From 4954c494d523220482a7b46229a5e10c8563ca83 Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Wed, 8 Feb 2023 14:36:47 +0100 Subject: [PATCH 1/6] Fix test_builtin_invocation_args_dict_function --- .../context_methods/test_builtin_functions.py | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/tests/functional/context_methods/test_builtin_functions.py b/tests/functional/context_methods/test_builtin_functions.py index 562118f946f..841e96a84b9 100644 --- a/tests/functional/context_methods/test_builtin_functions.py +++ b/tests/functional/context_methods/test_builtin_functions.py @@ -61,7 +61,11 @@ def parse_json_logs(json_log_output): def find_result_in_parsed_logs(parsed_logs, result_name): return next( - (item for item in parsed_logs if result_name in item["info"].get("msg", "msg")), + ( + item["data"]["msg"] + for item in parsed_logs + if result_name in item["data"].get("msg", "msg") + ), False, ) @@ -105,26 +109,18 @@ def test_builtin_invocation_args_dict_function(self, project): parsed_logs = parse_json_logs(log_output) result = find_result_in_parsed_logs(parsed_logs, "invocation_result") - assert result - - # Result is checked in two parts because profiles_dir is unique each test run - expected = "invocation_result: {'debug': True, 'log_format': 'json', 'write_json': True, 'use_colors': True, 'printer_width': 80, 'version_check': True, 'partial_parse': True, 'static_parser': True, 'profiles_dir': " - assert expected in str(result) - + # The result should include a dictionary of all flags with default values that aren't None expected = ( - "'send_anonymous_usage_stats': False", "'quiet': False", - "'no_print': False", + "'print': True", "'cache_selected_only': False", "'macro': 'validate_invocation'", - "'args': '{my_variable: test_variable}'", + "'args': {'my_variable': 'test_variable'}", "'which': 'run-operation'", - "'rpc_method': 'run-operation'", "'indirect_selection': 'eager'", ) - for element in expected: - assert element in str(result) + assert all(element in result for element in expected) def test_builtin_dbt_metadata_envs_function(self, project, monkeypatch): envs = { From cb05818e5f19511e17d58bf2a997759fe19f9686 Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Wed, 8 Feb 2023 14:46:16 +0100 Subject: [PATCH 2/6] Check a specific value, too --- .../functional/context_methods/test_builtin_functions.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/functional/context_methods/test_builtin_functions.py b/tests/functional/context_methods/test_builtin_functions.py index 841e96a84b9..f437dcb1aac 100644 --- a/tests/functional/context_methods/test_builtin_functions.py +++ b/tests/functional/context_methods/test_builtin_functions.py @@ -27,6 +27,9 @@ macros__validate_invocation_sql = """ {% macro validate_invocation(my_variable) %} + -- check a specific value + {{ log("use_colors: "~ invocation_args_dict['use_colors']) }} + -- whole dictionary (as string) {{ log("invocation_result: "~ invocation_args_dict) }} {% endmacro %} """ @@ -108,7 +111,9 @@ def test_builtin_invocation_args_dict_function(self, project): ) parsed_logs = parse_json_logs(log_output) - result = find_result_in_parsed_logs(parsed_logs, "invocation_result") + use_colors = result = find_result_in_parsed_logs(parsed_logs, "use_colors") + assert use_colors == "use_colors: True" + invocation_dict = find_result_in_parsed_logs(parsed_logs, "invocation_result") assert result # The result should include a dictionary of all flags with default values that aren't None expected = ( @@ -120,7 +125,7 @@ def test_builtin_invocation_args_dict_function(self, project): "'which': 'run-operation'", "'indirect_selection': 'eager'", ) - assert all(element in result for element in expected) + assert all(element in invocation_dict for element in expected) def test_builtin_dbt_metadata_envs_function(self, project, monkeypatch): envs = { From 3464ba79884c149a8b3d6149926d32cc767adee6 Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Wed, 8 Feb 2023 23:25:48 +0100 Subject: [PATCH 3/6] Break the test, again, for now --- tests/functional/context_methods/test_builtin_functions.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/functional/context_methods/test_builtin_functions.py b/tests/functional/context_methods/test_builtin_functions.py index f437dcb1aac..3acaf25170c 100644 --- a/tests/functional/context_methods/test_builtin_functions.py +++ b/tests/functional/context_methods/test_builtin_functions.py @@ -115,8 +115,9 @@ def test_builtin_invocation_args_dict_function(self, project): assert use_colors == "use_colors: True" invocation_dict = find_result_in_parsed_logs(parsed_logs, "invocation_result") assert result - # The result should include a dictionary of all flags with default values that aren't None + # The result should include a dictionary of all flags with values that aren't None expected = ( + "'send_anonymous_usage_stats': False", "'quiet': False", "'print': True", "'cache_selected_only': False", From 74dde4494f554c1c51dfb12e15a60e0a5495f163 Mon Sep 17 00:00:00 2001 From: Ian Knox Date: Wed, 8 Feb 2023 16:30:43 -0600 Subject: [PATCH 4/6] fixed the issue in DO_NOT_TRACK code --- core/dbt/cli/flags.py | 10 +++------- core/dbt/cli/main.py | 2 +- core/dbt/cli/params.py | 11 ++++------- core/dbt/cli/requires.py | 2 +- core/dbt/contracts/graph/manifest.py | 2 +- core/dbt/flags.py | 1 - core/dbt/main.py | 2 +- core/dbt/tracking.py | 4 ++-- test/unit/test_manifest.py | 4 ++-- .../context_methods/test_builtin_functions.py | 1 + tests/unit/test_cli_flags.py | 2 +- 11 files changed, 17 insertions(+), 24 deletions(-) diff --git a/core/dbt/cli/flags.py b/core/dbt/cli/flags.py index e6d15ced5ab..e7f8f06d036 100644 --- a/core/dbt/cli/flags.py +++ b/core/dbt/cli/flags.py @@ -151,13 +151,9 @@ def assign_params(ctx, params_assigned_from_default): object.__setattr__(self, "LOG_PATH", log_path) # Support console DO NOT TRACK initiave - object.__setattr__( - self, - "ANONYMOUS_USAGE_STATS", - False - if os.getenv("DO_NOT_TRACK", "").lower() in ("1", "t", "true", "y", "yes") - else True, - ) + if os.getenv("DO_NOT_TRACK", "").lower() in ("1", "t", "true", "y", "yes"): + object.__setattr__(self, "SEND_ANONYMOUS_USAGE_STATS", False) + # Check mutual exclusivity once all flags are set self._assert_mutually_exclusive( params_assigned_from_default, ["WARN_ERROR", "WARN_ERROR_OPTIONS"] diff --git a/core/dbt/cli/main.py b/core/dbt/cli/main.py index df41c7421f0..f7d7f8ef60c 100644 --- a/core/dbt/cli/main.py +++ b/core/dbt/cli/main.py @@ -66,7 +66,7 @@ def invoke(self, args: List[str]) -> Tuple[Optional[List], bool]: epilog="Specify one of these sub-commands and you can find more help from there.", ) @click.pass_context -@p.anonymous_usage_stats +@p.send_anonymous_usage_stats @p.cache_selected_only @p.debug @p.enable_legacy_logger diff --git a/core/dbt/cli/params.py b/core/dbt/cli/params.py index 32e5663d47a..983dea12c80 100644 --- a/core/dbt/cli/params.py +++ b/core/dbt/cli/params.py @@ -6,13 +6,10 @@ from dbt.cli.resolvers import default_project_dir, default_profiles_dir from dbt.version import get_version_information -# TODO: The name (reflected in flags) is a correction! -# The original name was `SEND_ANONYMOUS_USAGE_STATS` and used an env var called "DBT_SEND_ANONYMOUS_USAGE_STATS" -# Both of which break existing naming conventions (doesn't match param flag). -# This will need to be fixed before use in the main codebase and communicated as a change to the community! -anonymous_usage_stats = click.option( - "--anonymous-usage-stats/--no-anonymous-usage-stats", - envvar="DBT_ANONYMOUS_USAGE_STATS", +# 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, ) diff --git a/core/dbt/cli/requires.py b/core/dbt/cli/requires.py index eb45374db35..e301d21732c 100644 --- a/core/dbt/cli/requires.py +++ b/core/dbt/cli/requires.py @@ -25,7 +25,7 @@ def wrapper(*args, **kwargs): set_flags(flags) # Tracking - initialize_from_flags(flags.ANONYMOUS_USAGE_STATS, flags.PROFILES_DIR) + initialize_from_flags(flags.SEND_ANONYMOUS_USAGE_STATS, flags.PROFILES_DIR) ctx.with_resource(track_run(run_command=flags.WHICH)) # Logging diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index 881a2b34435..dbd2f6e7246 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -303,7 +303,7 @@ def __post_init__(self): self.user_id = tracking.active_user.id if self.send_anonymous_usage_stats is None: - self.send_anonymous_usage_stats = get_flags().ANONYMOUS_USAGE_STATS + self.send_anonymous_usage_stats = get_flags().SEND_ANONYMOUS_USAGE_STATS @classmethod def default(cls): diff --git a/core/dbt/flags.py b/core/dbt/flags.py index 9eacefc49d7..473378ba7bd 100644 --- a/core/dbt/flags.py +++ b/core/dbt/flags.py @@ -73,7 +73,6 @@ def get_flag_dict(): "version_check", "fail_fast", "send_anonymous_usage_stats", - "anonymous_usage_stats", "printer_width", "indirect_selection", "log_cache_events", diff --git a/core/dbt/main.py b/core/dbt/main.py index e186142c937..3ea3521868e 100644 --- a/core/dbt/main.py +++ b/core/dbt/main.py @@ -172,7 +172,7 @@ def handle_and_check(args): # Set flags from args, user config, and env vars user_config = read_user_config(flags.PROFILES_DIR) # This is read again later flags.set_from_args(parsed, user_config) - dbt.tracking.initialize_from_flags(flags.ANONYMOUS_USAGE_STATS, flags.PROFILES_DIR) + dbt.tracking.initialize_from_flags(flags.SEND_ANONYMOUS_USAGE_STATS, flags.PROFILES_DIR) # Set log_format from flags parsed.cls.set_log_format() diff --git a/core/dbt/tracking.py b/core/dbt/tracking.py index bb1b615ead7..f488babe002 100644 --- a/core/dbt/tracking.py +++ b/core/dbt/tracking.py @@ -428,10 +428,10 @@ def process(self, record): ) -def initialize_from_flags(anonymous_usage_stats, profiles_dir): +def initialize_from_flags(send_anonymous_usage_stats, profiles_dir): # Setting these used to be in UserConfig, but had to be moved here global active_user - if anonymous_usage_stats: + if send_anonymous_usage_stats: active_user = User(profiles_dir) try: active_user.initialize() diff --git a/test/unit/test_manifest.py b/test/unit/test_manifest.py index a5262d0f91f..30555b8db7b 100644 --- a/test/unit/test_manifest.py +++ b/test/unit/test_manifest.py @@ -427,7 +427,7 @@ def test__build_flat_graph(self): def test_metadata(self, mock_user): mock_user.id = 'cfc9500f-dc7f-4c83-9ea7-2c581c1b38cf' dbt.events.functions.EVENT_MANAGER.invocation_id = '01234567-0123-0123-0123-0123456789ab' - set_from_args(Namespace(ANONYMOUS_USAGE_STATS=False), None) + set_from_args(Namespace(SEND_ANONYMOUS_USAGE_STATS=False), None) now = datetime.utcnow() self.assertEqual( ManifestMetadata( @@ -450,7 +450,7 @@ def test_metadata(self, mock_user): def test_no_nodes_with_metadata(self, mock_user): mock_user.id = 'cfc9500f-dc7f-4c83-9ea7-2c581c1b38cf' dbt.events.functions.EVENT_MANAGER.invocation_id = '01234567-0123-0123-0123-0123456789ab' - set_from_args(Namespace(ANONYMOUS_USAGE_STATS=False), None) + set_from_args(Namespace(SEND_ANONYMOUS_USAGE_STATS=False), None) metadata = ManifestMetadata( project_id='098f6bcd4621d373cade4e832627b4f6', adapter_type='postgres', diff --git a/tests/functional/context_methods/test_builtin_functions.py b/tests/functional/context_methods/test_builtin_functions.py index f437dcb1aac..a1de5a91998 100644 --- a/tests/functional/context_methods/test_builtin_functions.py +++ b/tests/functional/context_methods/test_builtin_functions.py @@ -117,6 +117,7 @@ def test_builtin_invocation_args_dict_function(self, project): assert result # The result should include a dictionary of all flags with default values that aren't None expected = ( + "'send_anonymous_usage_stats': False", "'quiet': False", "'print': True", "'cache_selected_only': False", diff --git a/tests/unit/test_cli_flags.py b/tests/unit/test_cli_flags.py index 24730b02544..13d6a85c4e3 100644 --- a/tests/unit/test_cli_flags.py +++ b/tests/unit/test_cli_flags.py @@ -63,7 +63,7 @@ def test_anonymous_usage_state( monkeypatch.setenv("DO_NOT_TRACK", do_not_track) flags = Flags(run_context) - assert flags.ANONYMOUS_USAGE_STATS == expected_anonymous_usage_stats + assert flags.SEND_ANONYMOUS_USAGE_STATS == expected_anonymous_usage_stats def test_empty_user_config_uses_default(self, run_context, user_config): flags = Flags(run_context, user_config) From 7922969075008b0525a260a3828cbc6b4de7b13a Mon Sep 17 00:00:00 2001 From: Ian Knox Date: Wed, 8 Feb 2023 17:57:42 -0600 Subject: [PATCH 5/6] Beefed up the tests to catch future clobbering --- tests/unit/test_cli_flags.py | 53 ++++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/tests/unit/test_cli_flags.py b/tests/unit/test_cli_flags.py index 13d6a85c4e3..b1e27fe3be1 100644 --- a/tests/unit/test_cli_flags.py +++ b/tests/unit/test_cli_flags.py @@ -45,25 +45,54 @@ def test_log_path_default(self, run_context): assert getattr(flags, "LOG_PATH") == "logs" @pytest.mark.parametrize( - "do_not_track,expected_anonymous_usage_stats", + "set_stats_param,do_not_track,expected_anonymous_usage_stats", [ - ("1", False), - ("t", False), - ("true", False), - ("y", False), - ("yes", False), - ("false", True), - ("anything", True), - ("2", True), + # set_stats_param = default, DNT = True, expected = False + ("default", "1", False), + ("default", "t", False), + ("default", "true", False), + ("default", "y", False), + ("default", "yes", False), + # set_stats_param = default, DNT = false, expected = True + ("default", "false", True), + ("default", "anything", True), + # set_stats_param = True, DNT = True, expected = False + (True, "1", False), + (True, "t", False), + (True, "true", False), + (True, "y", False), + (True, "yes", False), + # set_stats_param = True, DNT = false, expected = True + (True, "false", True), + (True, "anything", True), + (True, "2", True), + # set_stats_param = False, DNT = True, expected = False + (False, "1", False), + (False, "t", False), + (False, "true", False), + (False, "y", False), + (False, "yes", False), + # set_stats_param = False, DNT = False, expected = False + (False, "false", False), + (False, "anything", False), + (False, "2", False), ], ) def test_anonymous_usage_state( - self, monkeypatch, run_context, do_not_track, expected_anonymous_usage_stats + self, + monkeypatch, + run_context, + set_stats_param, + do_not_track, + expected_anonymous_usage_stats, ): + # patch the env monkeypatch.setenv("DO_NOT_TRACK", do_not_track) - + # set the param + if set_stats_param != "default": + run_context.params["send_anonymous_usage_stats"] = set_stats_param flags = Flags(run_context) - assert flags.SEND_ANONYMOUS_USAGE_STATS == expected_anonymous_usage_stats + assert flags.SEND_ANONYMOUS_USAGE_STATS is expected_anonymous_usage_stats def test_empty_user_config_uses_default(self, run_context, user_config): flags = Flags(run_context, user_config) From 255e7159bcb718ab7d3cd40003170ae2a5867708 Mon Sep 17 00:00:00 2001 From: Ian Knox Date: Wed, 8 Feb 2023 18:33:52 -0600 Subject: [PATCH 6/6] cleaned up test a smidge --- tests/unit/test_cli_flags.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/test_cli_flags.py b/tests/unit/test_cli_flags.py index b1e27fe3be1..5b10b3f6f08 100644 --- a/tests/unit/test_cli_flags.py +++ b/tests/unit/test_cli_flags.py @@ -86,13 +86,11 @@ def test_anonymous_usage_state( do_not_track, expected_anonymous_usage_stats, ): - # patch the env monkeypatch.setenv("DO_NOT_TRACK", do_not_track) - # set the param if set_stats_param != "default": run_context.params["send_anonymous_usage_stats"] = set_stats_param flags = Flags(run_context) - assert flags.SEND_ANONYMOUS_USAGE_STATS is expected_anonymous_usage_stats + assert flags.SEND_ANONYMOUS_USAGE_STATS == expected_anonymous_usage_stats def test_empty_user_config_uses_default(self, run_context, user_config): flags = Flags(run_context, user_config)