From 140890c3c03479e513d867c1106bcce6dc847892 Mon Sep 17 00:00:00 2001 From: Stu Kilgore Date: Mon, 20 Feb 2023 12:42:53 -0600 Subject: [PATCH 1/5] Create method for env var deprecation --- .../Under the Hood-20230224-132811.yaml | 6 ++ core/dbt/cli/flags.py | 68 ++++++++++++++++--- core/dbt/cli/main.py | 2 + core/dbt/cli/params.py | 28 +++++--- core/dbt/cli/requires.py | 3 + core/dbt/deprecations.py | 15 ++++ core/dbt/events/proto_types.py | 14 ++++ core/dbt/events/types.py | 13 ++++ 8 files changed, 130 insertions(+), 19 deletions(-) create mode 100644 .changes/unreleased/Under the Hood-20230224-132811.yaml diff --git a/.changes/unreleased/Under the Hood-20230224-132811.yaml b/.changes/unreleased/Under the Hood-20230224-132811.yaml new file mode 100644 index 00000000000..56ad524164f --- /dev/null +++ b/.changes/unreleased/Under the Hood-20230224-132811.yaml @@ -0,0 +1,6 @@ +kind: Under the Hood +body: Add deprecation warning for DBT_NO_PRINT +time: 2023-02-24T13:28:11.295561-06:00 +custom: + Author: stu-k + Issue: "6960" diff --git a/core/dbt/cli/flags.py b/core/dbt/cli/flags.py index 6fe8a2c926b..4efd7c5d0ad 100644 --- a/core/dbt/cli/flags.py +++ b/core/dbt/cli/flags.py @@ -5,13 +5,14 @@ from importlib import import_module from multiprocessing import get_context from pprint import pformat as pf -from typing import Set, List +from typing import Callable, Dict, List, Set from click import Context, get_current_context, BadOptionUsage from click.core import ParameterSource, Command, Group from dbt.config.profile import read_user_config from dbt.contracts.project import UserConfig +from dbt.deprecations import renamed_env_var from dbt.helper_types import WarnErrorOptions from dbt.cli.resolvers import default_project_dir, default_log_path @@ -76,6 +77,11 @@ def args_to_context(args: List[str]) -> Context: return sub_command_ctx +DEPRECATED_PARAMS = { + "deprecated_print": "print", +} + + @dataclass(frozen=True) class Flags: def __init__(self, ctx: Context = None, user_config: UserConfig = None) -> None: @@ -87,7 +93,7 @@ def __init__(self, ctx: Context = None, user_config: UserConfig = None) -> None: if ctx is None: ctx = get_current_context() - def assign_params(ctx, params_assigned_from_default): + def assign_params(ctx, params_assigned_from_default, deprecated_env_vars): """Recursively adds all click params to flag object""" for param_name, param_value in ctx.params.items(): # TODO: this is to avoid duplicate params being defined in two places (version_check in run and cli) @@ -97,6 +103,10 @@ def assign_params(ctx, params_assigned_from_default): # when using frozen dataclasses. # https://docs.python.org/3/library/dataclasses.html#frozen-instances if hasattr(self, param_name.upper()): + if param_name in deprecated_env_vars: + # param already set via its deprecated but still respected env var + continue + if param_name not in EXPECTED_DUPLICATE_PARAMS: raise Exception( f"Duplicate flag names found in click command: {param_name}" @@ -107,15 +117,55 @@ def assign_params(ctx, params_assigned_from_default): if ctx.get_parameter_source(param_name) != ParameterSource.DEFAULT: object.__setattr__(self, param_name.upper(), param_value) else: - object.__setattr__(self, param_name.upper(), param_value) - if ctx.get_parameter_source(param_name) == ParameterSource.DEFAULT: - params_assigned_from_default.add(param_name) + # handle deprecated env vars while still respecting old values + # e.g. DBT_NO_PRINT -> DBT_PRINT if DBT_NO_PRINT is set, it is + # respected over DBT_PRINT or --print + if param_name in DEPRECATED_PARAMS: + + # deprecated env vars can only be set via env var + # we use the deprecated option in click to + param_source = ctx.get_parameter_source(param_name) + if param_source == ParameterSource.DEFAULT: + continue + elif param_source != ParameterSource.ENVIRONMENT: + raise BadOptionUsage( + "Deprecated parameters can only be set via environment variables" + ) + + # rename for clarity + dep_name = param_name + new_name = DEPRECATED_PARAMS.get(dep_name) + + # find param objects for their envvar name + try: + dep_opt = [x for x in ctx.command.params if x.name == dep_name][0] + new_opt = [x for x in ctx.command.params if x.name == new_name][0] + except IndexError: + raise Exception( + f"No deprecated param name match from {dep_name} to {new_name}" + ) + + # adding the deprecation warning function to the set + deprecated_env_vars[new_name] = renamed_env_var( + old_name=dep_opt.envvar, + new_name=new_opt.envvar, + ) + + object.__setattr__(self, new_name.upper(), param_value) + else: + object.__setattr__(self, param_name.upper(), param_value) + if ctx.get_parameter_source(param_name) == ParameterSource.DEFAULT: + params_assigned_from_default.add(param_name) if ctx.parent: - assign_params(ctx.parent, params_assigned_from_default) + assign_params(ctx.parent, params_assigned_from_default, deprecated_env_vars) params_assigned_from_default = set() # type: Set[str] - assign_params(ctx, params_assigned_from_default) + deprecated_env_vars: Dict[str, Callable] = {} + assign_params(ctx, params_assigned_from_default, deprecated_env_vars) + + # set deprecated_env_var_warnings to be fired later after events have been init + object.__setattr__(self, "deprecated_env_var_warnings", deprecated_env_vars.values()) # Get the invoked command flags invoked_subcommand_name = ( @@ -126,7 +176,9 @@ def assign_params(ctx, params_assigned_from_default): invoked_subcommand.allow_extra_args = True invoked_subcommand.ignore_unknown_options = True invoked_subcommand_ctx = invoked_subcommand.make_context(None, sys.argv) - assign_params(invoked_subcommand_ctx, params_assigned_from_default) + assign_params( + invoked_subcommand_ctx, params_assigned_from_default, deprecated_env_vars + ) if not user_config: profiles_dir = getattr(self, "PROFILES_DIR", None) diff --git a/core/dbt/cli/main.py b/core/dbt/cli/main.py index 4651cdbb1a9..e9a2ef9f424 100644 --- a/core/dbt/cli/main.py +++ b/core/dbt/cli/main.py @@ -2,6 +2,7 @@ from typing import List, Tuple, Optional import click + from dbt.cli import requires, params as p from dbt.config.project import Project from dbt.config.profile import Profile @@ -80,6 +81,7 @@ def invoke(self, args: List[str]) -> Tuple[Optional[List], bool]: @p.macro_debugging @p.partial_parse @p.print +@p.deprecated_print @p.printer_width @p.quiet @p.record_timing_info diff --git a/core/dbt/cli/params.py b/core/dbt/cli/params.py index bef6618eb75..5eb164c2327 100644 --- a/core/dbt/cli/params.py +++ b/core/dbt/cli/params.py @@ -6,13 +6,6 @@ from dbt.cli.resolvers import default_project_dir, default_profiles_dir from dbt.version import get_version_information -# TODO: Rename this to meet naming conventions (the word "send" is redundant) -send_anonymous_usage_stats = click.option( - "--send-anonymous-usage-stats/--no-send-anonymous-usage-stats", - envvar="DBT_SEND_ANONYMOUS_USAGE_STATS", - help="Send anonymous usage stats to dbt Labs.", - default=True, -) args = click.option( "--args", @@ -206,10 +199,7 @@ type=click.INT, ) -# TODO: The env var and name (reflected in flags) are corrections! -# The original name was `NO_PRINT` and used the env var `DBT_NO_PRINT`. -# 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! +# envvar was previously named DBT_NO_PRINT print = click.option( "--print/--no-print", envvar="DBT_PRINT", @@ -217,6 +207,15 @@ default=True, ) +deprecated_print = click.option( + "--deprecated-print/--deprecated-no-print", + envvar="DBT_NO_PRINT", + help="Internal flag for deprecating old env var.", + default=True, + hidden=True, + callback=lambda ctx, param, value: not value, +) + printer_width = click.option( "--printer-width", envvar="DBT_PRINTER_WIDTH", @@ -315,6 +314,13 @@ "--selector", envvar=None, help="The selector name to use, as defined in selectors.yml" ) +send_anonymous_usage_stats = click.option( + "--send-anonymous-usage-stats/--no-send-anonymous-usage-stats", + envvar="DBT_SEND_ANONYMOUS_USAGE_STATS", + help="Send anonymous usage stats to dbt Labs.", + default=True, +) + show = click.option( "--show", envvar=None, help="Show a sample of the loaded data in the terminal", is_flag=True ) diff --git a/core/dbt/cli/requires.py b/core/dbt/cli/requires.py index 10e1273214e..57f0df82795 100644 --- a/core/dbt/cli/requires.py +++ b/core/dbt/cli/requires.py @@ -40,6 +40,9 @@ def wrapper(*args, **kwargs): flags_dict_str = cast_dict_to_dict_of_strings(get_flag_dict()) fire_event(MainReportArgs(args=flags_dict_str)) + # Deprecation warnings + [dep_fn() for dep_fn in flags.deprecated_env_var_warnings] + if active_user is not None: # mypy appeasement, always true fire_event(MainTrackingUserState(user_state=active_user.state())) diff --git a/core/dbt/deprecations.py b/core/dbt/deprecations.py index f7cee59df5a..51545f28079 100644 --- a/core/dbt/deprecations.py +++ b/core/dbt/deprecations.py @@ -81,6 +81,21 @@ class ExposureNameDeprecation(DBTDeprecation): _event = "ExposureNameDeprecation" +def renamed_env_var(old_name: str, new_name: str): + class EnvironmentVariableRenamed(DBTDeprecation): + _name = f"environment-variable-renamed:{old_name}" + _event = "EnvironmentVariableRenamed" + + dep = EnvironmentVariableRenamed() + deprecations_list.append(dep) + deprecations[dep.name] = dep + + def cb(): + dep.show(old_name=old_name, new_name=new_name) + + return cb + + def warn(name, *args, **kwargs): if name not in deprecations: # this should (hopefully) never happen diff --git a/core/dbt/events/proto_types.py b/core/dbt/events/proto_types.py index c931feea5db..42b9015d16c 100644 --- a/core/dbt/events/proto_types.py +++ b/core/dbt/events/proto_types.py @@ -446,6 +446,20 @@ class InternalDeprecationMsg(betterproto.Message): data: "InternalDeprecation" = betterproto.message_field(2) +@dataclass +class EnvironmentVariableRenamed(betterproto.Message): + """D009""" + + old_name: str = betterproto.string_field(1) + new_name: str = betterproto.string_field(2) + + +@dataclass +class EnvironmentVariableRenamedMsg(betterproto.Message): + info: "EventInfo" = betterproto.message_field(1) + data: "EnvironmentVariableRenamed" = betterproto.message_field(2) + + @dataclass class AdapterEventDebug(betterproto.Message): """E001""" diff --git a/core/dbt/events/types.py b/core/dbt/events/types.py index b5e61e9007b..fe8640cee3e 100644 --- a/core/dbt/events/types.py +++ b/core/dbt/events/types.py @@ -393,6 +393,19 @@ def message(self): return warning_tag(msg) +@dataclass +class EnvironmentVariableRenamed(WarnLevel, pt.EnvironmentVariableRenamed): # noqa + def code(self): + return "D009" + + def message(self): + description = ( + f"The environment variable `{self.old_name}` has been renamed to `{self.new_name}`. " + f"Since `{self.old_name}` has been set, its value will be used instead of `{self.new_name}`." + ) + return line_wrap_message(warning_tag(f"Deprecated functionality\n\n{description}")) + + # ======================================================= # E - DB Adapter # ======================================================= From 48bedd948f84ea2d67bd0b9c0baff419bd7fc327 Mon Sep 17 00:00:00 2001 From: Github Build Bot Date: Fri, 24 Feb 2023 19:30:28 +0000 Subject: [PATCH 2/5] Add generated CLI API docs --- .../docs/build/doctrees/environment.pickle | Bin 207366 -> 207366 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/core/dbt/docs/build/doctrees/environment.pickle b/core/dbt/docs/build/doctrees/environment.pickle index 45eeb69e648060a873b75fb7b0fabd51ef3f11bd..0aea2be13d2398c36b54114d878e193a3790a742 100644 GIT binary patch delta 33 ncmZp>!qawzXG4=b+dmhP7e5v@cgVMQ$TI>l)AkN|<~l9_{zMIJ delta 33 ncmZp>!qawzXG4=b+uzTR?w|H Date: Mon, 27 Feb 2023 09:46:09 -0600 Subject: [PATCH 3/5] Tests, better warning message --- core/dbt/cli/flags.py | 5 +++-- core/dbt/events/types.py | 6 ++++-- tests/unit/test_cli.py | 4 +++- tests/unit/test_cli_flags.py | 6 ++++++ tests/unit/test_events.py | 1 + 5 files changed, 17 insertions(+), 5 deletions(-) diff --git a/core/dbt/cli/flags.py b/core/dbt/cli/flags.py index 4efd7c5d0ad..9e46200ebc5 100644 --- a/core/dbt/cli/flags.py +++ b/core/dbt/cli/flags.py @@ -122,8 +122,9 @@ def assign_params(ctx, params_assigned_from_default, deprecated_env_vars): # respected over DBT_PRINT or --print if param_name in DEPRECATED_PARAMS: - # deprecated env vars can only be set via env var - # we use the deprecated option in click to + # deprecated env vars can only be set via env var. + # we use the deprecated option in click to serialize the value + # from the env var string param_source = ctx.get_parameter_source(param_name) if param_source == ParameterSource.DEFAULT: continue diff --git a/core/dbt/events/types.py b/core/dbt/events/types.py index fe8640cee3e..f635e7fe1fd 100644 --- a/core/dbt/events/types.py +++ b/core/dbt/events/types.py @@ -400,8 +400,10 @@ def code(self): def message(self): description = ( - f"The environment variable `{self.old_name}` has been renamed to `{self.new_name}`. " - f"Since `{self.old_name}` has been set, its value will be used instead of `{self.new_name}`." + 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"Set `{self.new_name}` and unset `{self.old_name}` to avoid this deprecation warning and " + "ensure it works properly in a future release." ) return line_wrap_message(warning_tag(f"Deprecated functionality\n\n{description}")) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 4ed17583e69..e5d172f4977 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -32,7 +32,9 @@ def run_test(command): def test_param_names_match_envvars(self): def run_test(command): for param in command.params: - if param.envvar is not None: + # deprecated params are named "deprecated_x" and do not need to have + # a paralel name like "DBT_" + if param.envvar is not None and "deprecated_" not in param.name: assert "DBT_" + param.name.upper() == param.envvar if type(command) is click.Group: for command in command.commands.values(): diff --git a/tests/unit/test_cli_flags.py b/tests/unit/test_cli_flags.py index bf58703a1c0..12a5a6d649c 100644 --- a/tests/unit/test_cli_flags.py +++ b/tests/unit/test_cli_flags.py @@ -35,8 +35,14 @@ def test_mp_context(self, run_context): @pytest.mark.parametrize("param", cli.params) def test_cli_group_flags_from_params(self, run_context, param): flags = Flags(run_context) + + if "DEPRECATED_" in param.name.upper(): + assert not hasattr(flags, param.name.upper()) + return + if param.name.upper() in ("VERSION", "LOG_PATH"): return + assert hasattr(flags, param.name.upper()) assert getattr(flags, param.name.upper()) == run_context.params[param.name.lower()] diff --git a/tests/unit/test_events.py b/tests/unit/test_events.py index d111da2422f..7cc78a9dadc 100644 --- a/tests/unit/test_events.py +++ b/tests/unit/test_events.py @@ -126,6 +126,7 @@ def test_event_codes(self): types.MetricAttributesRenamed(metric_name=""), types.ExposureNameDeprecation(exposure=""), types.InternalDeprecation(name="", reason="", suggested_action="", version=""), + types.EnvironmentVariableRenamed(old_name="", new_name=""), # E - DB Adapter ====================== types.AdapterEventDebug(), types.AdapterEventInfo(), From 4134095e7055caf7c7e7cb4f1b72c0d257cdf18b Mon Sep 17 00:00:00 2001 From: Github Build Bot Date: Mon, 27 Feb 2023 15:48:04 +0000 Subject: [PATCH 4/5] Add generated CLI API docs --- .../docs/build/doctrees/environment.pickle | Bin 207366 -> 207366 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/core/dbt/docs/build/doctrees/environment.pickle b/core/dbt/docs/build/doctrees/environment.pickle index 0aea2be13d2398c36b54114d878e193a3790a742..5863d6cfe247a4486fd63be91a695cc9ca639686 100644 GIT binary patch delta 33 ncmZp>!qawzXG4=b+kX>Xw!5s&9rEoR@{B;tw7o-~xsD3};Aaco delta 33 ncmZp>!qawzXG4=b+dmhP7e5v@cgVMQ$TI>l)AkN|<~l9_{zMIJ From 727fd7163f7975f299af753be5bbe5878fb357bf Mon Sep 17 00:00:00 2001 From: Stu Kilgore Date: Mon, 27 Feb 2023 09:57:55 -0600 Subject: [PATCH 5/5] Typo --- tests/unit/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index e5d172f4977..0a122bef3bc 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -33,7 +33,7 @@ def test_param_names_match_envvars(self): def run_test(command): for param in command.params: # deprecated params are named "deprecated_x" and do not need to have - # a paralel name like "DBT_" + # a parallel name like "DBT_" if param.envvar is not None and "deprecated_" not in param.name: assert "DBT_" + param.name.upper() == param.envvar if type(command) is click.Group: