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

Extend Cosmos custom selector to support + when using paths and tags #1150

Merged

Conversation

mvictoria
Copy link
Contributor

@mvictoria mvictoria commented Aug 12, 2024

Description

This PR resolves #1141 where dbt projects loaded with DBT_MANIFEST were unable to parse using the GraphSelector.

Changes:

  • updated GraphSelector to take into account path and tag dbt selector methods
  • update SelectorConfig to use regex to parse selection statement to handle graph and path statements

ToDo:

  • add tests covering change
  • enable config dbt selector method
  • refactor

Related Issue(s)

closes #1141
closes #823

Breaking Change? No

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

updated GraphSelector to take into account path and tag dbt selector methods

update SelectorConfig to use regex to parse selection statement to handle graph and path statements
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Aug 12, 2024
Copy link

netlify bot commented Aug 12, 2024

Deploy Preview for sunny-pastelito-5ecb04 ready!

Name Link
🔨 Latest commit e1a8364
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/66b9788b16dc7a0008f045fa
😎 Deploy Preview https://deploy-preview-1150--sunny-pastelito-5ecb04.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dosubot dosubot bot added area:selector Related to selector, like DAG selector, DBT selector, etc parsing:dbt_manifest Issues, questions, or features related to dbt_manifest parsing labels Aug 12, 2024
Copy link

netlify bot commented Aug 12, 2024

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 67cb56f
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/66bcc7c521db99000864a153

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Aug 12, 2024
combine added tests into single parameterized test
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 97.77778% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.53%. Comparing base (e847f19) to head (67cb56f).
Report is 3 commits behind head on main.

Files Patch % Lines
cosmos/dbt/selector.py 97.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1150      +/-   ##
==========================================
- Coverage   96.53%   96.53%   -0.01%     
==========================================
  Files          64       64              
  Lines        3374     3402      +28     
==========================================
+ Hits         3257     3284      +27     
- Misses        117      118       +1     

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

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.

Hi @mvictoria, this is looking great, thank you for covering these use-cases that were not previously covered in Cosmos' custom selector.

Please, could you update the description adding also:
Closes: #823

Also, could we rename this PR to something more representative, such as:
"Extend Cosmos custom selector to support + when using paths and tags"

I left some minor comments inline, would love to hear your thoughts on those. Depending on how quickly we iterate on this, we may be able to release it in Cosmos 1.6.0, planned for this week.

added tests covering config
@mvictoria mvictoria changed the title update GraphSelector and SelectorConfig Extend Cosmos custom selector to support + when using paths and tags Aug 14, 2024
update GraphSelector doc string to include selectors
@mvictoria
Copy link
Contributor Author

Hey @tatiana - thanks for the review, I've updated tests for coverage and finished adding ability to use the "config" dbt selector with graph operators.

Please let me know if you have any more feedback!

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 a lot for the contribution, @mvictoria , we'll release this change as part of Cosmos 1.6.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 15, 2024
@tatiana tatiana merged commit f7354e5 into astronomer:main Aug 15, 2024
61 of 62 checks passed
@pankajkoti pankajkoti mentioned this pull request Aug 16, 2024
pankajkoti added a commit that referenced this pull request Aug 20, 2024
New Features

* Add support for loading manifest from cloud stores using Airflow
Object Storage by @pankajkoti in #1109
* Cache ``package-lock.yml`` file by @pankajastro in #1086
* Support persisting the ``LoadMode.VIRTUALENV`` directory by @tatiana
in #1079
* Add support to store and fetch ``dbt ls`` cache in remote stores by
@pankajkoti in #1147
* Add default source nodes rendering by @arojasb3 in #1107
* Add Teradata ``ProfileMapping`` by @sc250072 in #1077

Enhancements

* Add ``DatabricksOauthProfileMapping`` profile by @CorsettiS in #1091
* Use ``dbt ls`` as the default parser when ``profile_config`` is
provided by @pankajastro in #1101
* Add task owner to dbt operators by @wornjs in #1082
* Extend Cosmos custom selector to support + when using paths and tags
by @mvictoria in #1150
* Simplify logging by @dwreeves in #1108

Bug fixes

* Fix Teradata ``ProfileMapping`` target invalid issue by @sc250072 in
#1088
* Fix empty tag in case of custom parser by @pankajastro in #1100
* Fix ``dbt deps`` of ``LoadMode.DBT_LS`` should use
``ProjectConfig.dbt_vars`` by @tatiana in #1114
* Fix import handling by lazy loading hooks introduced in PR #1109 by
@dwreeves in #1132
* Fix Airflow 2.10 regression and add Airflow 2.10 in test matrix by
@pankajastro in #1162

Docs

* Fix typo in azure-container-instance docs by @pankajastro in #1106
* Use Airflow trademark as it has been registered by @pankajastro in
#1105

Others

* Run some example DAGs in Kubernetes execution mode in CI by
@pankajastro in #1127
* Install requirements.txt by default during dev env spin up by
@@CorsettiS in #1099
* Remove ``DbtGraph.current_version`` dead code by @tatiana in #1111
* Disable test for Airflow-2.5 and Python-3.11 combination in CI by
@pankajastro in #1124
* Pre-commit hook updates in #1074, #1113, #1125, #1144, #1154,  #1167

---------

Co-authored-by: Pankaj Koti <[email protected]>
Co-authored-by: Pankaj Singh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:selector Related to selector, like DAG selector, DBT selector, etc lgtm This PR has been approved by a maintainer parsing:dbt_manifest Issues, questions, or features related to dbt_manifest parsing size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
3 participants