From a57266aa4f37602dd7476ccb1ec74e7b202aa713 Mon Sep 17 00:00:00 2001 From: Stu Kilgore Date: Wed, 15 Mar 2023 12:26:31 -0500 Subject: [PATCH 1/2] Deprecate more env vars --- .../Under the Hood-20230315-122723.yaml | 6 ++ core/dbt/cli/flags.py | 13 +++- core/dbt/cli/main.py | 21 +++++++ core/dbt/cli/params.py | 38 +++++++++--- core/dbt/cli/requires.py | 1 + core/dbt/events/types.py | 2 +- .../cli/test_env_var_deprecations.py | 60 +++++++++++++++++++ 7 files changed, 130 insertions(+), 11 deletions(-) create mode 100644 .changes/unreleased/Under the Hood-20230315-122723.yaml create mode 100644 tests/functional/cli/test_env_var_deprecations.py diff --git a/.changes/unreleased/Under the Hood-20230315-122723.yaml b/.changes/unreleased/Under the Hood-20230315-122723.yaml new file mode 100644 index 00000000000..d99b7133b3b --- /dev/null +++ b/.changes/unreleased/Under the Hood-20230315-122723.yaml @@ -0,0 +1,6 @@ +kind: Under the Hood +body: Deprecate additional environment variables +time: 2023-03-15T12:27:23.194686-05:00 +custom: + Author: stu-k + Issue: "6903" diff --git a/core/dbt/cli/flags.py b/core/dbt/cli/flags.py index c0cae646688..ef2f50f2032 100644 --- a/core/dbt/cli/flags.py +++ b/core/dbt/cli/flags.py @@ -80,7 +80,10 @@ def args_to_context(args: List[str]) -> Context: DEPRECATED_PARAMS = { + "deprecated_defer": "defer", + "deprecated_favor_state": "favor_state", "deprecated_print": "print", + "deprecated_state": "state", } @@ -132,7 +135,8 @@ def assign_params(ctx, params_assigned_from_default, deprecated_env_vars): continue elif param_source != ParameterSource.ENVIRONMENT: raise BadOptionUsage( - "Deprecated parameters can only be set via environment variables" + param_name, + "Deprecated parameters can only be set via environment variables", ) # rename for clarity @@ -148,6 +152,10 @@ def assign_params(ctx, params_assigned_from_default, deprecated_env_vars): f"No deprecated param name match from {dep_name} to {new_name}" ) + # remove from defaulted set + if new_name in params_assigned_from_default: + params_assigned_from_default.remove(new_name) + # adding the deprecation warning function to the set deprecated_env_vars[new_name] = renamed_env_var( old_name=dep_opt.envvar, @@ -259,3 +267,6 @@ def _assert_mutually_exclusive( ) elif flag_set_by_user: set_flag = flag + + def clear_deprecations(self): + object.__delattr__(self, "deprecated_env_var_warnings") diff --git a/core/dbt/cli/main.py b/core/dbt/cli/main.py index dbe24205d4b..26ad48c613f 100644 --- a/core/dbt/cli/main.py +++ b/core/dbt/cli/main.py @@ -104,9 +104,11 @@ def cli(ctx, **kwargs): @cli.command("build") @click.pass_context @p.defer +@p.deprecated_defer @p.exclude @p.fail_fast @p.favor_state +@p.deprecated_favor_state @p.full_refresh @p.indirect_selection @p.profile @@ -117,6 +119,7 @@ def cli(ctx, **kwargs): @p.selector @p.show @p.state +@p.deprecated_state @p.store_failures @p.target @p.target_path @@ -173,14 +176,17 @@ def docs(ctx, **kwargs): @click.pass_context @p.compile_docs @p.defer +@p.deprecated_defer @p.exclude @p.favor_state +@p.deprecated_favor_state @p.profile @p.profiles_dir @p.project_dir @p.select @p.selector @p.state +@p.deprecated_state @p.target @p.target_path @p.threads @@ -236,8 +242,10 @@ def docs_serve(ctx, **kwargs): @cli.command("compile") @click.pass_context @p.defer +@p.deprecated_defer @p.exclude @p.favor_state +@p.deprecated_favor_state @p.full_refresh @p.indirect_selection @p.introspect @@ -249,6 +257,7 @@ def docs_serve(ctx, **kwargs): @p.selector @p.inline @p.state +@p.deprecated_state @p.target @p.target_path @p.threads @@ -351,6 +360,7 @@ def init(ctx, **kwargs): @p.raw_select @p.selector @p.state +@p.deprecated_state @p.target @p.vars @requires.preflight @@ -405,7 +415,9 @@ def parse(ctx, **kwargs): @cli.command("run") @click.pass_context @p.defer +@p.deprecated_defer @p.favor_state +@p.deprecated_favor_state @p.exclude @p.fail_fast @p.full_refresh @@ -415,6 +427,7 @@ def parse(ctx, **kwargs): @p.select @p.selector @p.state +@p.deprecated_state @p.target @p.target_path @p.threads @@ -478,6 +491,7 @@ def run_operation(ctx, **kwargs): @p.selector @p.show @p.state +@p.deprecated_state @p.target @p.target_path @p.threads @@ -504,14 +518,17 @@ def seed(ctx, **kwargs): @cli.command("snapshot") @click.pass_context @p.defer +@p.deprecated_defer @p.exclude @p.favor_state +@p.deprecated_favor_state @p.profile @p.profiles_dir @p.project_dir @p.select @p.selector @p.state +@p.deprecated_state @p.target @p.threads @p.vars @@ -551,6 +568,7 @@ def source(ctx, **kwargs): @p.select @p.selector @p.state +@p.deprecated_state @p.target @p.threads @p.vars @@ -582,9 +600,11 @@ def freshness(ctx, **kwargs): @cli.command("test") @click.pass_context @p.defer +@p.deprecated_defer @p.exclude @p.fail_fast @p.favor_state +@p.deprecated_favor_state @p.indirect_selection @p.profile @p.profiles_dir @@ -592,6 +612,7 @@ def freshness(ctx, **kwargs): @p.select @p.selector @p.state +@p.deprecated_state @p.store_failures @p.target @p.target_path diff --git a/core/dbt/cli/params.py b/core/dbt/cli/params.py index 8c7b95293ef..2e148e4121f 100644 --- a/core/dbt/cli/params.py +++ b/core/dbt/cli/params.py @@ -62,16 +62,21 @@ help="Display debug logging during dbt execution. Useful for debugging and making bug reports.", ) -# TODO: The env var and name (reflected in flags) are corrections! -# The original name was `DEFER_MODE` and used an env var called "DBT_DEFER_TO_STATE" -# Both of which break existing naming conventions. -# This will need to be fixed before use in the main codebase and communicated as a change to the community! +# flag was previously named DEFER_MODE defer = click.option( "--defer/--no-defer", envvar="DBT_DEFER", help="If set, defer to the state variable for resolving unselected nodes.", ) +deprecated_defer = click.option( + "--deprecated-defer", + envvar="DBT_DEFER_TO_STATE", + help="Internal flag for deprecating old env var.", + default=False, + hidden=True, +) + enable_legacy_logger = click.option( "--enable-legacy-logger/--no-enable-legacy-logger", envvar="DBT_ENABLE_LEGACY_LOGGER", @@ -95,6 +100,12 @@ help="If set, defer to the argument provided to the state flag for resolving unselected nodes, even if the node(s) exist as a database object in the current environment.", ) +deprecated_favor_state = click.option( + "--deprecated-favor-state", + envvar="DBT_FAVOR_STATE_MODE", + help="Internal flag for deprecating old env var.", +) + full_refresh = click.option( "--full-refresh", "-f", @@ -214,7 +225,6 @@ type=click.INT, ) -# envvar was previously named DBT_NO_PRINT print = click.option( "--print/--no-print", envvar="DBT_PRINT", @@ -359,10 +369,6 @@ "--skip-profile-setup", "-s", envvar=None, help="Skip interactive profile setup.", is_flag=True ) -# TODO: The env var and name (reflected in flags) are corrections! -# The original name was `ARTIFACT_STATE_PATH` and used the env var `DBT_ARTIFACT_STATE_PATH`. -# Both of which break existing naming conventions. -# This will need to be fixed before use in the main codebase and communicated as a change to the community! state = click.option( "--state", envvar="DBT_STATE", @@ -376,6 +382,20 @@ ), ) +deprecated_state = click.option( + "--deprecated-state", + envvar="DBT_ARTIFACT_STATE_PATH", + help="Internal flag for deprecating old env var.", + hidden=True, + type=click.Path( + dir_okay=True, + file_okay=False, + readable=True, + resolve_path=True, + path_type=Path, + ), +) + static_parser = click.option( "--static-parser/--no-static-parser", envvar="DBT_STATIC_PARSER", diff --git a/core/dbt/cli/requires.py b/core/dbt/cli/requires.py index 57f0df82795..49f35ef7a9f 100644 --- a/core/dbt/cli/requires.py +++ b/core/dbt/cli/requires.py @@ -42,6 +42,7 @@ def wrapper(*args, **kwargs): # Deprecation warnings [dep_fn() for dep_fn in flags.deprecated_env_var_warnings] + flags.clear_deprecations() if active_user is not None: # mypy appeasement, always true fire_event(MainTrackingUserState(user_state=active_user.state())) diff --git a/core/dbt/events/types.py b/core/dbt/events/types.py index 9ff34f84b52..9060807d68c 100644 --- a/core/dbt/events/types.py +++ b/core/dbt/events/types.py @@ -401,7 +401,7 @@ def code(self): def message(self): description = ( f"The environment variable `{self.old_name}` has been renamed as `{self.new_name}`.\n" - f"If `{self.old_name}` is currently set, its value will be used instead of `{self.old_name}`.\n" + f"If `{self.old_name}` is currently set, its value will be used instead of `{self.new_name}`.\n" f"Set `{self.new_name}` and unset `{self.old_name}` to avoid this deprecation warning and " "ensure it works properly in a future release." ) diff --git a/tests/functional/cli/test_env_var_deprecations.py b/tests/functional/cli/test_env_var_deprecations.py new file mode 100644 index 00000000000..6880cc6890d --- /dev/null +++ b/tests/functional/cli/test_env_var_deprecations.py @@ -0,0 +1,60 @@ +import pytest +import os + +from dbt.tests.util import read_file, run_dbt + + +model_one_sql = """ + select 1 as fun +""" + + +class TestDeprecatedEnvVars: + @pytest.fixture(scope="class") + def models(self): + return {"model_one.sql": model_one_sql} + + def test_defer(self, project, logs_dir): + self.assert_deprecated( + logs_dir, + "DBT_DEFER_TO_STATE", + "DBT_DEFER", + ) + + def test_favor_state(self, project, logs_dir): + self.assert_deprecated( + logs_dir, + "DBT_FAVOR_STATE_MODE", + "DBT_FAVOR_STATE", + command="build", + ) + + def test_print(self, project, logs_dir): + self.assert_deprecated( + logs_dir, + "DBT_NO_PRINT", + "DBT_PRINT", + ) + + def test_state(self, project, logs_dir): + self.assert_deprecated( + logs_dir, + "DBT_ARTIFACT_STATE_PATH", + "DBT_STATE", + old_val=".", + ) + + def assert_deprecated(self, logs_dir, old_env_var, new_env_var, command="run", old_val="0"): + os.environ[old_env_var] = old_val + run_dbt([command]) + + # replacing new lines with spaces accounts for text wrapping + log_file = read_file(logs_dir, "dbt.log").replace("\n", " ").replace("\\n", " ") + dep_str = f"The environment variable `{old_env_var}` has been renamed as `{new_env_var}`" + + try: + assert dep_str in log_file + except Exception as e: + del os.environ[old_env_var] + raise e + del os.environ[old_env_var] From 733d4729c64ff1cfaf1c7f2de311e0f0d041cc2b Mon Sep 17 00:00:00 2001 From: Stu Kilgore Date: Mon, 20 Mar 2023 10:48:02 -0500 Subject: [PATCH 2/2] Better comments, combine firing into class method --- core/dbt/cli/flags.py | 8 ++++++-- core/dbt/cli/requires.py | 3 +-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/core/dbt/cli/flags.py b/core/dbt/cli/flags.py index ef2f50f2032..b338959b02d 100644 --- a/core/dbt/cli/flags.py +++ b/core/dbt/cli/flags.py @@ -152,7 +152,8 @@ def assign_params(ctx, params_assigned_from_default, deprecated_env_vars): f"No deprecated param name match from {dep_name} to {new_name}" ) - # remove from defaulted set + # remove param from defaulted set since the deprecated + # value is not set from default, but from an env var if new_name in params_assigned_from_default: params_assigned_from_default.remove(new_name) @@ -268,5 +269,8 @@ def _assert_mutually_exclusive( elif flag_set_by_user: set_flag = flag - def clear_deprecations(self): + def fire_deprecations(self): + [dep_fn() for dep_fn in self.deprecated_env_var_warnings] + # it is necessary to remove this attr from the class so it does + # not get pickled when written to disk as json object.__delattr__(self, "deprecated_env_var_warnings") diff --git a/core/dbt/cli/requires.py b/core/dbt/cli/requires.py index 49f35ef7a9f..adeeb0b485a 100644 --- a/core/dbt/cli/requires.py +++ b/core/dbt/cli/requires.py @@ -41,8 +41,7 @@ def wrapper(*args, **kwargs): fire_event(MainReportArgs(args=flags_dict_str)) # Deprecation warnings - [dep_fn() for dep_fn in flags.deprecated_env_var_warnings] - flags.clear_deprecations() + flags.fire_deprecations() if active_user is not None: # mypy appeasement, always true fire_event(MainTrackingUserState(user_state=active_user.state()))