Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "tags:" prefix from model config selectors #1387

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove "tags:" prefix from model config selectors #1387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cosmos/dbt/graph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Collaborator

@tatiana tatiana Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

node = DbtNode(
unique_id=f"{model.type.value}.{self.project.project_name}.{model_name}",
resource_type=DbtResourceType(model.type.value),
Expand Down
2 changes: 1 addition & 1 deletion tests/dbt/test_graph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ def test_load_via_load_via_custom_parser_select_rendering_config():
assert model_name == "customers"
filter_node = dbt_graph.filtered_nodes.get(model_name)
assert filter_node.file_path == DBT_PROJECTS_ROOT_DIR / "jaffle_shop/models/customers.sql"
assert filter_node.tags == ["tags:customers"]
assert filter_node.tags == ["customers"]


@patch("cosmos.dbt.graph.DbtGraph.update_node_dependency", return_value=None)
Expand Down
Loading