Remove "tags:" prefix from model config selectors #1387
Conversation
yoshimaru46
force-pushed
the
yoshimaru46/fix-tag-on-load-mode-custom
branch
from
807cd58 to
53f1083
Compare
dosubot
bot
added
size:XS
| @@ -654,6 +654,8 @@ def load_via_custom_parser(self) -> None: | |||
| for model_name, model in models: | |||
| config = {item.split(":")[0]: item.split(":")[-1] for item in model.config.config_selectors} | |||
| tags = [selector for selector in model.config.config_selectors if selector.startswith("tags:")] | |||
| # Remove the "tags:" prefix | |||
| tags = [tag.split(":")[-1] for tag in tags] | |||
There was a problem hiding this comment.
We should not represent tags in different ways depending on the parsing mode. Either we remove the prefix for all LoadModes that set this configuration (not only LoadMode.CUSTOM, but also LoadMode.DBT_MANIFEST), or we remove for none of them.
By changing this here, I'm afraid we may be breaking the behaviour ahead in the selector, for instance:
https://github.com/astronomer/astronomer-cosmos/blob/main/cosmos/dbt/selector.py#L177
The prefix "tag:" is currently being used in the selector module in at least four places via the TAG_SELECTOR constant: https://github.com/astronomer/astronomer-cosmos/blob/110fb0760cb1a21a2a21ca1a06370c44613ba085/cosmos/dbt/selector.py#L20C1-L20C13
Since the select_nodes is being used not only by RenderMode.CUSTOM, but also by RenderMode.DBT_MANIFEST, I believe this change will break the overall behaviour.
I'm surprised no unit tests broke with this change of interface.
There was a problem hiding this comment.
@yoshimaru46 thanks for identifying the bug and proposing a fix.
I have shared a concern in-line.
I agree the following line will likely fail in your use case:
astronomer-cosmos/cosmos/dbt/selector.py
Line 422 in 110fb07
I have two requests:
- Could you confirm it also fails if you use
LoadMode.DBT_MANIFEST - Would it make sense to make the change in this method itself, to avoid breaking other parts, as mentioned in my other comment?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1387 +/- ##
=======================================
Coverage 96.24% 96.24%
=======================================
Files 67 67
Lines 4051 4052 +1
=======================================
+ Hits 3899 3900 +1
Misses 152 152 ☔ View full report in Codecov by Sentry. |
Description
closes #1367 (comment)
Related Issue(s)
Breaking Change?
Checklist