From 27fa159bc2cf4bebb0ad57a686a59abac4b1d390 Mon Sep 17 00:00:00 2001 From: Tomasz Urbaszek Date: Fri, 8 Mar 2024 14:04:57 +0100 Subject: [PATCH 1/2] Add config based feature flags (#772) --- src/snowflake/cli/api/commands/snow_typer.py | 5 +- src/snowflake/cli/api/config.py | 34 ++++++++-- src/snowflake/cli/api/feature_flags.py | 36 ++++++++++ .../cli/plugins/streamlit/manager.py | 6 +- .../__snapshots__/test_snow_typer.ambr | 32 +++++++++ tests/api/commands/test_snow_typer.py | 23 +++++++ tests/api/test_feature_flags.py | 65 +++++++++++++++++++ .../test_override_by_external_plugins.ambr | 19 ++++++ 8 files changed, 212 insertions(+), 8 deletions(-) create mode 100644 src/snowflake/cli/api/feature_flags.py create mode 100644 tests/api/test_feature_flags.py create mode 100644 tests/plugin/__snapshots__/test_override_by_external_plugins.ambr diff --git a/src/snowflake/cli/api/commands/snow_typer.py b/src/snowflake/cli/api/commands/snow_typer.py index f0cf14afb1..dfd70b5f99 100644 --- a/src/snowflake/cli/api/commands/snow_typer.py +++ b/src/snowflake/cli/api/commands/snow_typer.py @@ -2,7 +2,7 @@ import logging from functools import wraps -from typing import Optional +from typing import Callable, Optional import typer from snowflake.cli.api.commands.decorators import ( @@ -30,6 +30,7 @@ def command( name: Optional[str] = None, requires_global_options: bool = True, requires_connection: bool = False, + is_enabled: Callable[[], bool] | None = None, **kwargs, ): """ @@ -37,6 +38,8 @@ def command( logic before and after execution as well as process the result and act on possible errors. """ + if is_enabled is not None and not is_enabled(): + return lambda func: func def custom_command(command_callable): """Custom command wrapper similar to Typer.command.""" diff --git a/src/snowflake/cli/api/config.py b/src/snowflake/cli/api/config.py index 67859815ab..5d844f3173 100644 --- a/src/snowflake/cli/api/config.py +++ b/src/snowflake/cli/api/config.py @@ -7,6 +7,7 @@ from typing import Any, Dict, Optional, Union import tomlkit +from click import ClickException from snowflake.cli.api.exceptions import ( ConfigFileTooWidePermissionsError, MissingConfiguration, @@ -37,6 +38,7 @@ class Empty: LOGS_SECTION_PATH = [CLI_SECTION, LOGS_SECTION] PLUGINS_SECTION_PATH = [CLI_SECTION, PLUGINS_SECTION] +FEATURE_FLAGS_SECTION_PATH = [CLI_SECTION, "features"] CONFIG_MANAGER.add_option( name=CLI_SECTION, @@ -147,7 +149,7 @@ def get_config_section(*path) -> dict: def get_config_value(*path, key: str, default: Optional[Any] = Empty) -> Any: """Looks for given key under nested path in toml file.""" - env_variable = _get_env_value(*path, key=key) + env_variable = get_env_value(*path, key=key) if env_variable: return env_variable try: @@ -158,6 +160,25 @@ def get_config_value(*path, key: str, default: Optional[Any] = Empty) -> Any: raise +def get_config_bool_value(*path, key: str, default: Optional[Any] = Empty) -> bool: + value = get_config_value(*path, key=key, default=default) + # If we get bool then we can return + if isinstance(value, bool): + return value + + # Now if value is not string then cast it to str. Simplifies logic for 1 and 0 + if not isinstance(value, str): + value = str(value) + + know_booleans_mapping = {"true": True, "false": False, "1": True, "0": False} + + if value.lower() not in know_booleans_mapping: + raise ClickException( + f"Expected boolean value for {'.'.join((*path, key))} option." + ) + return know_booleans_mapping[value.lower()] + + def _initialise_config(config_file: Path) -> None: config_file = SecurePath(config_file) config_file.parent.mkdir(parents=True, exist_ok=True) @@ -166,11 +187,12 @@ def _initialise_config(config_file: Path) -> None: log.info("Created Snowflake configuration file at %s", CONFIG_MANAGER.file_path) -def _get_env_value(*path, key: str) -> str | None: - env_variable_name = ( - "SNOWFLAKE_" + "_".join(p.upper() for p in path) + f"_{key.upper()}" - ) - return os.environ.get(env_variable_name) +def get_env_variable_name(*path, key: str) -> str: + return "SNOWFLAKE_" + "_".join(p.upper() for p in path) + f"_{key.upper()}" + + +def get_env_value(*path, key: str) -> str | None: + return os.environ.get(get_env_variable_name(*path, key=key)) def _find_section(*path) -> TOMLDocument: diff --git a/src/snowflake/cli/api/feature_flags.py b/src/snowflake/cli/api/feature_flags.py new file mode 100644 index 0000000000..fc525f66c6 --- /dev/null +++ b/src/snowflake/cli/api/feature_flags.py @@ -0,0 +1,36 @@ +from enum import Enum, unique +from typing import NamedTuple + +from snowflake.cli.api.config import ( + FEATURE_FLAGS_SECTION_PATH, + get_config_bool_value, + get_env_variable_name, +) + + +class BooleanFlag(NamedTuple): + name: str + default: bool = False + + +@unique +class FeatureFlagMixin(Enum): + def is_enabled(self) -> bool: + return get_config_bool_value( + *FEATURE_FLAGS_SECTION_PATH, + key=self.value.name.lower(), + default=self.value.default, + ) + + def is_disabled(self): + return not self.is_enabled() + + def env_variable(self): + return get_env_variable_name(*FEATURE_FLAGS_SECTION_PATH, key=self.value.name) + + +@unique +class FeatureFlag(FeatureFlagMixin): + ENABLE_STREAMLIT_EMBEDDED_STAGE = BooleanFlag( + "ENABLE_STREAMLIT_EMBEDDED_STAGE", False + ) diff --git a/src/snowflake/cli/plugins/streamlit/manager.py b/src/snowflake/cli/plugins/streamlit/manager.py index 3beb317a39..0bc728b0d8 100644 --- a/src/snowflake/cli/plugins/streamlit/manager.py +++ b/src/snowflake/cli/plugins/streamlit/manager.py @@ -7,6 +7,7 @@ from snowflake.cli.api.commands.experimental_behaviour import ( experimental_behaviour_enabled, ) +from snowflake.cli.api.feature_flags import FeatureFlag from snowflake.cli.api.project.util import unquote_identifier from snowflake.cli.api.sql_execution import SqlExecutionMixin from snowflake.cli.plugins.connection.util import ( @@ -106,7 +107,10 @@ def deploy( ) fully_qualified_name = stage_manager.to_fully_qualified_name(streamlit_name) streamlit_name = self.get_name_from_fully_qualified_name(fully_qualified_name) - if experimental_behaviour_enabled(): + if ( + experimental_behaviour_enabled() + or FeatureFlag.ENABLE_STREAMLIT_EMBEDDED_STAGE.is_enabled() + ): """ 1. Create streamlit object 2. Upload files to embedded stage diff --git a/tests/api/commands/__snapshots__/test_snow_typer.ambr b/tests/api/commands/__snapshots__/test_snow_typer.ambr index 919844c93e..5d79558542 100644 --- a/tests/api/commands/__snapshots__/test_snow_typer.ambr +++ b/tests/api/commands/__snapshots__/test_snow_typer.ambr @@ -98,5 +98,37 @@ ╰──────────────────────────────────────────────────────────────────────────────╯ + ''' +# --- +# name: test_enabled_command_is_not_visible + ''' + Usage: snow [OPTIONS] COMMAND [ARGS]... + Try 'snow --help' for help. + ╭─ Error ──────────────────────────────────────────────────────────────────────╮ + │ No such command 'switchable_cmd'. │ + ╰──────────────────────────────────────────────────────────────────────────────╯ + + ''' +# --- +# name: test_enabled_command_is_visible + ''' + + Usage: snow switchable_cmd [OPTIONS] + + ╭─ Options ────────────────────────────────────────────────────────────────────╮ + │ --help -h Show this message and exit. │ + ╰──────────────────────────────────────────────────────────────────────────────╯ + ╭─ Global configuration ───────────────────────────────────────────────────────╮ + │ --format [TABLE|JSON] Specifies the output format. │ + │ [default: TABLE] │ + │ --verbose -v Displays log entries for log levels `info` │ + │ and higher. │ + │ --debug Displays log entries for log levels `debug` │ + │ and higher; debug logs contains additional │ + │ information. │ + │ --silent Turns off intermediate output to console. │ + ╰──────────────────────────────────────────────────────────────────────────────╯ + + ''' # --- diff --git a/tests/api/commands/test_snow_typer.py b/tests/api/commands/test_snow_typer.py index 08bb095dc3..2083b4f1c4 100644 --- a/tests/api/commands/test_snow_typer.py +++ b/tests/api/commands/test_snow_typer.py @@ -39,6 +39,9 @@ def exception_handler(err): return _CustomTyper +_ENABLED_FLAG = False + + def app_factory(typer_cls): app = typer_cls(name="snow") @@ -62,6 +65,10 @@ def cmd_with_global_options(name: str = typer.Argument()): def cmd_with_connection_options(name: str = typer.Argument()): return MessageResult(f"hello {name}") + @app.command("switchable_cmd", is_enabled=lambda: _ENABLED_FLAG) + def cmd_witch_enabled_switch(): + return MessageResult("Enabled") + return app @@ -147,6 +154,22 @@ def test_command_with_connection_options(cli, snapshot): assert result.output == snapshot +def test_enabled_command_is_visible(cli, snapshot): + global _ENABLED_FLAG + _ENABLED_FLAG = True + result = cli(app_factory(SnowTyper))(["switchable_cmd", "--help"]) + assert result.exit_code == 0 + assert result.output == snapshot + + +def test_enabled_command_is_not_visible(cli, snapshot): + global _ENABLED_FLAG + _ENABLED_FLAG = False + result = cli(app_factory(SnowTyper))(["switchable_cmd", "--help"]) + assert result.exit_code == 2 + assert result.output == snapshot + + @mock.patch("snowflake.cli.app.telemetry.log_command_usage") def test_snow_typer_pre_execute_sends_telemetry(mock_log_command_usage, cli): result = cli(app_factory(SnowTyper))(["simple_cmd", "Norma"]) diff --git a/tests/api/test_feature_flags.py b/tests/api/test_feature_flags.py new file mode 100644 index 0000000000..78349b8881 --- /dev/null +++ b/tests/api/test_feature_flags.py @@ -0,0 +1,65 @@ +from unittest import mock + +import pytest +from click import ClickException +from snowflake.cli.api.feature_flags import BooleanFlag, FeatureFlagMixin + + +class _TestFlags(FeatureFlagMixin): + # Intentional inconsistency between constant and the enum name to make sure there's no strict relation + ENABLED_BY_DEFAULT = BooleanFlag("ENABLED_DEFAULT", True) + DISABLED_BY_DEFAULT = BooleanFlag("DISABLED_DEFAULT", False) + NON_BOOLEAN = BooleanFlag("NON_BOOLEAN", "xys") # type: ignore + + +def test_flag_value_has_to_be_boolean(): + with pytest.raises(ClickException): + _TestFlags.NON_BOOLEAN.is_enabled() + + +def test_flag_is_enabled(): + assert _TestFlags.ENABLED_BY_DEFAULT.is_enabled() is True + assert _TestFlags.ENABLED_BY_DEFAULT.is_disabled() is False + + +def test_flag_is_disabled(): + assert _TestFlags.DISABLED_BY_DEFAULT.is_enabled() is False + assert _TestFlags.DISABLED_BY_DEFAULT.is_disabled() is True + + +def test_flag_env_variable_value(): + assert ( + _TestFlags.ENABLED_BY_DEFAULT.env_variable() + == "SNOWFLAKE_CLI_FEATURES_ENABLED_DEFAULT" + ) + assert ( + _TestFlags.DISABLED_BY_DEFAULT.env_variable() + == "SNOWFLAKE_CLI_FEATURES_DISABLED_DEFAULT" + ) + + +@mock.patch("snowflake.cli.api.config.get_config_value") +@pytest.mark.parametrize("value_from_config", [True, False]) +def test_flag_from_config_file(mock_get_config_value, value_from_config): + mock_get_config_value.return_value = value_from_config + + assert _TestFlags.DISABLED_BY_DEFAULT.is_enabled() is value_from_config + mock_get_config_value.assert_called_once_with( + "cli", "features", key="disabled_default", default=False + ) + + +@pytest.mark.parametrize("value_from_env", ["1", "true", "True", "TRUE", "TruE"]) +def test_flag_is_enabled_from_env_var(value_from_env): + with mock.patch.dict( + "os.environ", {"SNOWFLAKE_CLI_FEATURES_DISABLED_DEFAULT": value_from_env} + ): + assert _TestFlags.DISABLED_BY_DEFAULT.is_enabled() is True + + +@pytest.mark.parametrize("value_from_env", ["0", "false", "False", "FALSE", "FaLse"]) +def test_flag_is_disabled_from_env_var(value_from_env): + with mock.patch.dict( + "os.environ", {"SNOWFLAKE_CLI_FEATURES_ENABLED_DEFAULT": value_from_env} + ): + assert _TestFlags.ENABLED_BY_DEFAULT.is_enabled() is False diff --git a/tests/plugin/__snapshots__/test_override_by_external_plugins.ambr b/tests/plugin/__snapshots__/test_override_by_external_plugins.ambr new file mode 100644 index 0000000000..24d29ecdc6 --- /dev/null +++ b/tests/plugin/__snapshots__/test_override_by_external_plugins.ambr @@ -0,0 +1,19 @@ +# serializer version: 1 +# name: test_corrupted_config_raises_human_friendly_error[[corrupted0] + ''' + ╭─ Error ──────────────────────────────────────────────────────────────────────╮ + │ Configuration file seems to be corrupted. Unexpected end of file at line 1 │ + │ col 10 │ + ╰──────────────────────────────────────────────────────────────────────────────╯ + + ''' +# --- +# name: test_corrupted_config_raises_human_friendly_error[[corrupted1] + ''' + ╭─ Error ──────────────────────────────────────────────────────────────────────╮ + │ Configuration file seems to be corrupted. Unexpected end of file at line 1 │ + │ col 10 │ + ╰──────────────────────────────────────────────────────────────────────────────╯ + + ''' +# --- From ed437e49b225cbd3b969a812e2cb2021031a1284 Mon Sep 17 00:00:00 2001 From: Tomasz Urbaszek Date: Fri, 8 Mar 2024 14:27:57 +0100 Subject: [PATCH 2/2] Use @snowflakedb/snowcli in codeowners (#884) --- .github/CODEOWNERS | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 78117f4c78..692889bf80 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,4 +1,4 @@ -* @sfc-gh-turbaszek @sfc-gh-pjob @sfc-gh-jsikorski @sfc-gh-astus @sfc-gh-mraba @sfc-gh-pczajka +* @snowflakedb/snowcli # Native Apps Owners src/snowflake/cli/plugins/nativeapp/ @snowflakedb/nade @@ -7,4 +7,4 @@ tests_integration/test_nativeapp.py @snowflakedb/nade # Project Definition Owners src/snowflake/cli/api/project/schemas/native_app.py @snowflakedb/nade -tests/project/ @sfc-gh-turbaszek @sfc-gh-pjob @sfc-gh-jsikorski @sfc-gh-astus @sfc-gh-mraba @sfc-gh-pczajka @snowflakedb/nade +tests/project/ @snowflakedb/snowcli @snowflakedb/nade