-
Notifications
You must be signed in to change notification settings - Fork 79
Snow 2306184 ng config support 3 #2638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ff33724
1fc26cf
c7a6cee
5450a81
8cb20fa
4f848c4
93c1b10
0562341
eabb59c
304a27a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,9 +10,9 @@ | |
| CLI_SECTION, | ||
| IGNORE_NEW_VERSION_WARNING_KEY, | ||
| get_config_bool_value, | ||
| get_config_manager, | ||
| ) | ||
| from snowflake.cli.api.secure_path import SecurePath | ||
| from snowflake.connector.config_manager import CONFIG_MANAGER | ||
|
|
||
| REPOSITORY_URL_PIP = "https://pypi.org/pypi/snowflake-cli/json" | ||
| REPOSITORY_URL_BREW = "https://formulae.brew.sh/api/formula/snowflake-cli.json" | ||
|
|
@@ -69,12 +69,14 @@ class _VersionCache: | |
| _last_time = "last_time_check" | ||
| _version = "version" | ||
| _last_time_shown = "last_time_shown" | ||
| _version_cache_file = SecurePath( | ||
| CONFIG_MANAGER.file_path.parent / ".cli_version.cache" | ||
| ) | ||
|
|
||
| @property | ||
| def _version_cache_file(self): | ||
| """Get version cache file path with lazy evaluation.""" | ||
| return SecurePath(get_config_manager().file_path.parent / ".cli_version.cache") | ||
|
|
||
| def __init__(self): | ||
| self._cache_file = _VersionCache._version_cache_file | ||
| self._cache_file = self._version_cache_file | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The property Consider modifying this approach to truly implement lazy loading: def __init__(self):
self._cache_file = None # Initialize as None
@property
def _cache_file(self):
if self.__cache_file is None:
self.__cache_file = self._version_cache_file
return self.__cache_file
@_cache_file.setter
def _cache_file(self, value):
self.__cache_file = valueThis way, the property is only evaluated when first accessed, not during initialization. Spotted by Diamond |
||
|
|
||
| def _save_latest_version(self, version: str): | ||
| data = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,12 +21,19 @@ | |
| from pathlib import Path | ||
| from typing import TYPE_CHECKING, Iterator | ||
|
|
||
| import tomlkit | ||
| from snowflake.cli.api.connections import ConnectionContext, OpenConnectionCache | ||
| from snowflake.cli.api.exceptions import MissingConfigurationError | ||
| from snowflake.cli.api.metrics import CLIMetrics | ||
| from snowflake.cli.api.output.formats import OutputFormat | ||
| from snowflake.cli.api.rendering.jinja import CONTEXT_KEY | ||
| from snowflake.connector import SnowflakeConnection | ||
| from snowflake.connector.config_manager import ( | ||
| ConfigManager, | ||
| ConfigSlice, | ||
| ConfigSliceOptions, | ||
| ) | ||
| from snowflake.connector.constants import CONFIG_FILE | ||
|
|
||
| if TYPE_CHECKING: | ||
| from snowflake.cli._plugins.sql.repl import Repl | ||
|
|
@@ -66,13 +73,19 @@ class _CliGlobalContextManager: | |
| _definition_manager: DefinitionManager | None = None | ||
| enhanced_exit_codes: bool = False | ||
|
|
||
| _config_manager: ConfigManager | None = None | ||
| config_file_override: Path | None = None | ||
| connections_file_override: Path | None = None | ||
|
|
||
| # which properties invalidate our current DefinitionManager? | ||
| DEFINITION_MANAGER_DEPENDENCIES = [ | ||
| "project_path_arg", | ||
| "project_is_optional", | ||
| "project_env_overrides_args", | ||
| ] | ||
|
|
||
| CONFIG_MANAGER_DEPENDENCIES = ["config_file_override", "connections_file_override"] | ||
|
|
||
| def reset(self): | ||
| self.__init__() | ||
|
|
||
|
|
@@ -88,6 +101,9 @@ def __setattr__(self, prop, val): | |
| if prop in self.DEFINITION_MANAGER_DEPENDENCIES: | ||
| self._clear_definition_manager() | ||
|
|
||
| if prop in self.CONFIG_MANAGER_DEPENDENCIES: | ||
| self._clear_config_manager() | ||
|
|
||
| super().__setattr__(prop, val) | ||
|
|
||
| @property | ||
|
|
@@ -144,6 +160,63 @@ def _clear_definition_manager(self): | |
| """ | ||
| self._definition_manager = None | ||
|
|
||
| @property | ||
| def config_manager(self) -> ConfigManager: | ||
| """ | ||
| Get or create the configuration manager instance. | ||
| Follows the same lazy initialization pattern as DefinitionManager. | ||
| """ | ||
| if self._config_manager is None: | ||
| self._config_manager = self._create_config_manager() | ||
| return self._config_manager | ||
|
|
||
| def _create_config_manager(self) -> ConfigManager: | ||
| """ | ||
| Factory method to create ConfigManager instance with CLI-specific options. | ||
| Replicates the behavior of the imported CONFIG_MANAGER singleton. | ||
| """ | ||
| from snowflake.cli.api.config import get_connections_file | ||
|
|
||
| connections_file = get_connections_file() | ||
|
|
||
| connections_slice = ConfigSlice( | ||
| path=connections_file, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually is that override even needed? We don't seem to be using it anywhere?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With connector based singleton this was created by default. As we are dropping the connector version of singleton we need to have our own support for connections.toml |
||
| options=ConfigSliceOptions(check_permissions=True, only_in_slice=False), | ||
| section="connections", | ||
| ) | ||
|
|
||
| manager = ConfigManager( | ||
| name="CONFIG_MANAGER", | ||
| file_path=self.config_file_override or CONFIG_FILE, | ||
| _slices=[connections_slice], | ||
| ) | ||
|
|
||
| manager.add_option( | ||
| name="connections", | ||
| parse_str=tomlkit.parse, | ||
| default=dict(), | ||
| ) | ||
|
|
||
| manager.add_option( | ||
| name="default_connection_name", parse_str=str, default="default" | ||
| ) | ||
|
|
||
| from snowflake.cli.api.config import CLI_SECTION | ||
|
|
||
| manager.add_option( | ||
| name=CLI_SECTION, | ||
| parse_str=tomlkit.parse, | ||
| default=dict(), | ||
| ) | ||
|
|
||
| return manager | ||
|
|
||
| def _clear_config_manager(self): | ||
| """ | ||
| Force re-creation of config manager when dependencies change. | ||
| """ | ||
| self._config_manager = None | ||
|
|
||
|
|
||
| class _CliGlobalContextAccess: | ||
| def __init__(self, manager: _CliGlobalContextManager): | ||
|
|
@@ -216,6 +289,21 @@ def repl(self) -> Repl | None: | |
| """Get the current REPL instance if running in REPL mode.""" | ||
| return self._manager.repl_instance | ||
|
|
||
| @property | ||
| def config_manager(self) -> ConfigManager: | ||
| """Get the current configuration manager.""" | ||
| return self._manager.config_manager | ||
|
|
||
| @property | ||
| def config_file_override(self) -> Path | None: | ||
| """Get the current config file override path.""" | ||
| return self._manager.config_file_override | ||
|
|
||
| @config_file_override.setter | ||
| def config_file_override(self, value: Path | None) -> None: | ||
| """Set the config file override path.""" | ||
| self._manager.config_file_override = value | ||
|
|
||
|
|
||
| _CLI_CONTEXT_MANAGER: ContextVar[_CliGlobalContextManager | None] = ContextVar( | ||
| "cli_context", default=None | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: local import