[Bugfix][Selection] yaml selector do not obey default overwrite #10009
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
1 similar comment
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10009 +/- ##
==========================================
- Coverage 88.16% 88.12% -0.04%
==========================================
Files 181 181
Lines 22593 22613 +20
==========================================
+ Hits 19918 19928 +10
- Misses 2675 2685 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -121,7 +122,7 @@ def selection_criteria_from_dict( | |||
|
|
|||
| # If defined field in selector, override CLI flag | |||
| indirect_selection = IndirectSelection( | |||
| dct.get("indirect_selection", None) or indirect_selection | |||
| dct.get("indirect_selection", get_flags().INDIRECT_SELECTION) | |||
There was a problem hiding this comment.
This is the most important change that leads to resolving the bug and also removes all of the variable pass-through.
There was a problem hiding this comment.
What is the motivation for removing the variable pass-through here? I think it could be preferable to call get_flags().INDIRECT_SELECTION in runnable.py to avoid lower-level methods accessing global state.
There was a problem hiding this comment.
I looked up all call paths using this variable and all of them are referring to flags. INDIRECT_SELECTION. We can either pass it all the way everywhere or call it once in this lower level function(since this is where the selector being created.).
I think calling once at the lower level is a cleaner implementation overall. Since dbt is built on the assumption of flags is the first thing being constructed during an invocation.
There was a problem hiding this comment.
Okay, that's fair. I recall from our 'de-globalization' conversations in the past that flags was more acceptable to access because its never mutated and initialized at the very beginning of an invocation, as you mentioned.
| "indirect_selection_value,expected_value", | ||
| [(v, v) for v in IndirectSelection], | ||
| ) | ||
| def test_selection_criteria_default_indirect_value(indirect_selection_value, expected_value): |
There was a problem hiding this comment.
New unit test added for this
| @@ -92,3 +95,5 @@ def test_setup_event_logger_specify_max_bytes(mocker): | |||
| patched_file_handler.assert_called_once_with( | |||
| filename="logs/dbt.log", encoding="utf8", maxBytes=1234567, backupCount=5 | |||
| ) | |||
| # XXX if we do not clean up event logger here we are going to affect other tests. | |||
There was a problem hiding this comment.
This is something interesting that I noticed.
|
I think there are still some issues with selectors and indirect-selection flags in |
resolves #9776 resolves #7673
Problem
If user define a YAML selector and does not define indirect selection field on it. We would always use the default value
eagerinstead of using the resolved default value of the CLI flag and env vars.(meaning the user cannot overwrite it)See more detail in #7673
Solution
This patch would cause a selector to default to the resolved value of CLI, env var, specified by user.
This patch also removed the technical debt of always passing an indirect selection field across multiple layers where we should really just do one lookup when a selector is created.
Checklist