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.

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

Fix custom selector when select has a subset of tags of the models' tags #606

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

david-mag
Copy link
Contributor

Description

Tags filtering (select/exclude) was bugged, expecting a perfect match of all tags set (e.g. model1 with tags tag1 and tag2 would only be selected/excluded if the select/exclude statement in the RenderConfig is set to select=["tag:tag1,tag:tag2"], but not if it is set as select=["tag:tag1"] or select=["tag:tag2"] only.)

Tests were already existing, but the node-setup did not include intersections of tags. I've fixed that and added another test as well.

Checklist

  • I have made corresponding changes to the documentation (if required)
  • I have added tests that prove my fix is effective or that my feature works

- tags and config.tags to be identical (usually the case)
- add consistency with has_child and is_child tag for non-test nodes
@david-mag david-mag requested a review from a team as a code owner October 17, 2023 12:33
@david-mag david-mag requested a review from a team October 17, 2023 12:33
@netlify
Copy link

netlify bot commented Oct 17, 2023

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name Link
🔨 Latest commit e9f2b48
🔍 Latest deploy log https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/652e7f084e9ead00087530c8

@david-mag david-mag temporarily deployed to external October 17, 2023 12:33 — with GitHub Actions Inactive
Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

Thanks, @david-mag , it looks good - happy to merge once the checks pass!

@david-mag
Copy link
Contributor Author

@tatiana Not sure whats the issue exactly, but the error msg in the failed test returns this:

result = FullOutputSubprocessResult(exit_code=2, output='      Error during request to server: : Invalid access token.', full_o...time Error', '  Runtime Error', '    Database Error', '      Error during request to server: : Invalid access token.'])

So could it be that the integration test itself has an issue? I'm not aware of any changes regarding access tokens in my PR. 😅

@tatiana
Copy link
Collaborator

tatiana commented Oct 17, 2023

Hey @david-mag , yes I noticed this earlier today - it seems there is an issue with the token. I will look into this within a few hours!

@david-mag david-mag temporarily deployed to external October 17, 2023 21:04 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d61f120) 93.13% compared to head (e9f2b48) 93.13%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #606   +/-   ##
=======================================
  Coverage   93.13%   93.13%           
=======================================
  Files          51       51           
  Lines        2053     2053           
=======================================
  Hits         1912     1912           
  Misses        141      141           
Files Coverage Δ
cosmos/dbt/selector.py 97.34% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tatiana tatiana changed the title Selector tag bugfix Fix custom selector when select has a subset of tags of the models' tags Oct 17, 2023
@tatiana tatiana merged commit a433f15 into astronomer:main Oct 17, 2023
42 checks passed
@tatiana tatiana added this to the 1.2.1 milestone Oct 18, 2023
tatiana pushed a commit that referenced this pull request Oct 25, 2023
…ags (#606)

Before, when using Cosmos custom selector, it expected an exact match of all *tags*.

Example: `model1` with tags `tag1` and `tag2` would only be
selected/excluded if the select/exclude statement in the `RenderConfig`
was set to `select=["tag:tag1,tag:tag2"]`. It would not be selected if the `RenderConfig` had
`select=["tag:tag1"]` or `select=["tag:tag2"]` only.

(cherry picked from commit a433f15)
tatiana added a commit that referenced this pull request Oct 25, 2023
Bug fixes

* Resolve errors occurring when `dbt_project_path` is str and partial support `dbt_project_path=None` by @MrBones757 in #605
* Fix running dbt tests that depend on multiple models (support `--indirect-selection` buildable) by @david-mag in #613
* Add tests to sources, snapshots and seeds when using `TestBehavior.AFTER_EACH` by @tatiana in #599
* Fix custom selector when select has a subset of tags of the models' tags by @david-mag in #606

Others

* Update contributing guide docs by @raffifu in #591
* Fix running test that validates manifest-based DAGs by @tatiana in #619
* pre-commit updates in #604 and #621
tatiana added a commit that referenced this pull request Oct 25, 2023
Bug fixes

* Resolve errors occurring when `dbt_project_path` is str and partial support `dbt_project_path=None` by @MrBones757 in #605
* Fix running dbt tests that depend on multiple models (support `--indirect-selection` buildable) by @david-mag in #613
* Add tests to sources, snapshots and seeds when using `TestBehavior.AFTER_EACH` by @tatiana in #599
* Fix custom selector when select has a subset of tags of the models' tags by @david-mag in #606

Others

* Update contributing guide docs by @raffifu in #591
* Fix running test that validates manifest-based DAGs by @tatiana in #619
* pre-commit updates in #604 and #621

(cherry picked from commit 52c34a2)
tatiana added a commit that referenced this pull request Oct 25, 2023
Bug fixes

* Resolve errors occurring when `dbt_project_path` is str and partial support `dbt_project_path=None` by @MrBones757 in #605
* Fix running dbt tests that depend on multiple models (support `--indirect-selection` buildable) by @david-mag in #613
* Add tests to sources, snapshots and seeds when using `TestBehavior.AFTER_EACH` by @tatiana in #599
* Fix custom selector when select has a subset of tags of the models' tags by @david-mag in #606
* Fix `LoadMode.AUTOMATIC` behaviour to use `LoadMode.DBT_LS` when `ProfileMapping` is used by @tatiana in #625
* Fix failure if `openlineage-common` raises a jinja exception by @tatiana in #626

Others

* Update contributing guide docs by @raffifu in #591
* Remove unnecessary stack trace from Cosmos initialization by @tatiana in #624
* Fix running test that validates manifest-based DAGs by @tatiana in #619
* pre-commit updates in #604 and #621
tatiana added a commit that referenced this pull request Oct 25, 2023
Bug fixes

* Resolve errors occurring when `dbt_project_path` is str and partial support `dbt_project_path=None` by @MrBones757 in #605
* Fix running dbt tests that depend on multiple models (support `--indirect-selection` buildable) by @david-mag in #613
* Add tests to sources, snapshots and seeds when using `TestBehavior.AFTER_EACH` by @tatiana in #599
* Fix custom selector when select has a subset of tags of the models' tags by @david-mag in #606
* Fix `LoadMode.AUTOMATIC` behaviour to use `LoadMode.DBT_LS` when `ProfileMapping` is used by @tatiana in #625
* Fix failure if `openlineage-common` raises a jinja exception by @tatiana in #626

Others

* Update contributing guide docs by @raffifu in #591
* Remove unnecessary stack trace from Cosmos initialization by @tatiana in #624
* Fix running test that validates manifest-based DAGs by @tatiana in #619
* pre-commit updates in #604 and #621

(cherry picked from commit 635fb7b)
@tatiana tatiana mentioned this pull request Oct 25, 2023
tatiana added a commit that referenced this pull request Oct 25, 2023
Bug fixes

* Resolve errors occurring when `dbt_project_path` is str and partial
support `dbt_project_path=None` by @MrBones757 in #605
* Fix running dbt tests that depend on multiple models (support
`--indirect-selection` buildable) by @david-mag in #613
* Add tests to sources, snapshots and seeds when using
`TestBehavior.AFTER_EACH` by @tatiana in #599
* Fix custom selector when select has a subset of tags of the models'
tags by @david-mag in #606
* Fix `LoadMode.AUTOMATIC` behaviour to use `LoadMode.DBT_LS` when
`ProfileMapping` is used by @tatiana in #625
* Fix failure if `openlineage-common` raises a jinja exception by
@tatiana in #626

Others

* Update contributing guide docs by @raffifu in #591
* Remove unnecessary stack trace from Cosmos initialization by @tatiana
in #624
* Fix running test that validates manifest-based DAGs by @tatiana in
#619
* pre-commit updates in #604 and #621

(cherry picked from commit 635fb7b)
@tatiana
Copy link
Collaborator

tatiana commented Oct 25, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants