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

Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution #634

Merged
merged 10 commits into from
Nov 3, 2023
Merged

Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution #634

merged 10 commits into from
Nov 3, 2023

Conversation

MrBones757
Copy link
Contributor

@MrBones757 MrBones757 commented Oct 28, 2023

Description

This MR finishes the work that was started in #605 to add full support for ProjectConfig.dbt_project_path = None, and implements #568.

Within this PR, several things have been updated:

1 - Added project_path fields to RenderConfig and ExecutionConfig
2 - Simplified the consumption of RenderConfig in the dbtGraph class
3 - added option to configure different dbt executable for Rendering vs Execution.

Related Issue(s)

Closes: #568

Breaking Change?

None

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

@MrBones757 MrBones757 requested a review from a team as a code owner October 28, 2023 09:47
@MrBones757 MrBones757 requested a review from a team October 28, 2023 09:47
@netlify
Copy link

netlify bot commented Oct 28, 2023

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name Link
🔨 Latest commit 111cea5
🔍 Latest deploy log https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/65450bc6147c25000800f2a2

@MrBones757 MrBones757 temporarily deployed to external October 28, 2023 09:50 — with GitHub Actions Inactive
@MrBones757
Copy link
Contributor Author

Docs changes still need to be completed, though all the core logic and baseline tests in its current state has been implemented.

@tatiana could you please have a scan over the changes in this MR and let me know if there are any other changes you'd like to see before i finalize the docs / tests?

@MrBones757 MrBones757 temporarily deployed to external October 30, 2023 01:14 — 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.

@MrBones757, this looks great, thank you!

I left two minor comments - I'll review them again after the checks are passed.

cosmos/dbt/graph.py Outdated Show resolved Hide resolved
cosmos/config.py Show resolved Hide resolved
@MrBones757 MrBones757 temporarily deployed to external October 31, 2023 02:35 — with GitHub Actions Inactive
@MrBones757 MrBones757 temporarily deployed to external October 31, 2023 03:01 — with GitHub Actions Inactive
@MrBones757 MrBones757 temporarily deployed to external October 31, 2023 03:45 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (14b3090) 93.41% compared to head (111cea5) 92.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #634      +/-   ##
==========================================
- Coverage   93.41%   92.66%   -0.76%     
==========================================
  Files          53       53              
  Lines        2158     2168      +10     
==========================================
- Hits         2016     2009       -7     
- Misses        142      159      +17     
Files Coverage Δ
cosmos/config.py 97.08% <100.00%> (+0.27%) ⬆️
cosmos/converter.py 96.87% <100.00%> (+0.04%) ⬆️
cosmos/dbt/graph.py 98.80% <92.00%> (-0.60%) ⬇️

... and 1 file with indirect coverage changes

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

@MrBones757 MrBones757 temporarily deployed to external October 31, 2023 05:09 — with GitHub Actions Inactive
@MrBones757
Copy link
Contributor Author

I've added some additional tests to captures some more edge cases i found and add additional coverage.
Additionally, I've updated some docs, and docstrings, to reflect changes that have occurred in some Config Classes both within this PR and others.

All unit tests are running fine locally, and all integration tests other than databricks are too (no token for this)
I'll verify these in CI and once thats complete its looking just about complete.

@MrBones757 MrBones757 temporarily deployed to external October 31, 2023 09:57 — with GitHub Actions Inactive
@tatiana tatiana added this to the 1.2.2 milestone Oct 31, 2023
@MrBones757
Copy link
Contributor Author

Since the initial integration tests look good, is there anything else that needs to be done here :)

@tatiana tatiana temporarily deployed to external November 1, 2023 10:36 — with GitHub Actions Inactive
@tatiana tatiana temporarily deployed to external November 1, 2023 17:17 — with GitHub Actions Inactive
@tatiana tatiana temporarily deployed to external November 1, 2023 21:54 — with GitHub Actions Inactive
…er for new Fields. Simplified Graph interaction to consume RenderConfig natively. Updates tests to reflect changes
…onfig.dbt_project_path or ProjectConfig.manifest_path as we now support defining dbt_project_path in ExecutionConfig and RenderConfig
@MrBones757 MrBones757 temporarily deployed to external November 2, 2023 12:59 — with GitHub Actions Inactive
@MrBones757 MrBones757 temporarily deployed to external November 2, 2023 14:45 — with GitHub Actions Inactive
@MrBones757 MrBones757 temporarily deployed to external November 2, 2023 16:05 — with GitHub Actions Inactive
@tatiana tatiana changed the title Finish implementation of ProjectConfig.dbt_project_path = None & Allow Different paths for Rendering and Execution Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution Nov 3, 2023
@tatiana tatiana merged commit b64eb9a into astronomer:main Nov 3, 2023
39 of 40 checks passed
tatiana pushed a commit that referenced this pull request Nov 6, 2023
… Rendering and Execution (#634)

This MR finishes the work that was started in #605 to add full support
for ProjectConfig.dbt_project_path = None, and implements #568.

Within this PR, several things have been updated:
1 - Added project_path fields to RenderConfig and ExecutionConfig
2 - Simplified the consumption of RenderConfig in the dbtGraph class
3 - added option to configure different dbt executables for Rendering vs
Execution.

Closes: #568
(cherry picked from commit b64eb9a)
tatiana added a commit that referenced this pull request Nov 6, 2023
Bug fixes

* Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution by @MrBones757 in #634
* Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM by @edgga in #615

Others

* Add pre-commit hook for McCabe max complexity check and fix errors by @jbandoro in #629
* Update contributing docs for running integration tests by @jbandoro in #638
* Fix CI issue running integration tests by @tatiana in #640 and #644
* pre-commit updates in #637
@tatiana tatiana mentioned this pull request Nov 6, 2023
tatiana pushed a commit that referenced this pull request Nov 6, 2023
… Rendering and Execution (#634)

This MR finishes the work that was started in #605 to add full support
for ProjectConfig.dbt_project_path = None, and implements #568.

Within this PR, several things have been updated:
1 - Added project_path fields to RenderConfig and ExecutionConfig
2 - Simplified the consumption of RenderConfig in the dbtGraph class
3 - added option to configure different dbt executables for Rendering vs
Execution.

Closes: #568
(cherry picked from commit b64eb9a)
tatiana added a commit that referenced this pull request Nov 6, 2023
Bug fixes

* Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution by @MrBones757 in #634
* Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM by @edgga in #615

Others

* Add pre-commit hook for McCabe max complexity check and fix errors by @jbandoro in #629
* Update contributing docs for running integration tests by @jbandoro in #638
* Fix CI issue running integration tests by @tatiana in #640 and #644
* pre-commit updates in #637
tatiana added a commit that referenced this pull request Nov 7, 2023
Bug fixes

* Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution by @MrBones757 in #634
* Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM by @edgga in #615

Others

* Add pre-commit hook for McCabe max complexity check and fix errors by @jbandoro in #629
* Update contributing docs for running integration tests by @jbandoro in #638
* Fix CI issue running integration tests by @tatiana in #640 and #644
* pre-commit updates in #637

(cherry picked from commit fa0620a)
LennartKloppenburg pushed a commit to LennartKloppenburg/astronomer-cosmos that referenced this pull request Nov 17, 2023
… Rendering and Execution (astronomer#634)

This MR finishes the work that was started in astronomer#605 to add full support
for ProjectConfig.dbt_project_path = None, and implements astronomer#568.

Within this PR, several things have been updated:
1 - Added project_path fields to RenderConfig and ExecutionConfig
2 - Simplified the consumption of RenderConfig in the dbtGraph class
3 - added option to configure different dbt executables for Rendering vs
Execution.

Closes: astronomer#568
LennartKloppenburg pushed a commit to LennartKloppenburg/astronomer-cosmos that referenced this pull request Dec 17, 2023
… Rendering and Execution (astronomer#634)

This MR finishes the work that was started in astronomer#605 to add full support
for ProjectConfig.dbt_project_path = None, and implements astronomer#568.

Within this PR, several things have been updated:
1 - Added project_path fields to RenderConfig and ExecutionConfig
2 - Simplified the consumption of RenderConfig in the dbtGraph class
3 - added option to configure different dbt executables for Rendering vs
Execution.

Closes: astronomer#568
tatiana pushed a commit that referenced this pull request Jul 4, 2024
… Rendering and Execution (#634)

This MR finishes the work that was started in #605 to add full support
for ProjectConfig.dbt_project_path = None, and implements #568.

Within this PR, several things have been updated:
1 - Added project_path fields to RenderConfig and ExecutionConfig
2 - Simplified the consumption of RenderConfig in the dbtGraph class
3 - added option to configure different dbt executables for Rendering vs
Execution.

Closes: #568
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
Bug fixes

* Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution by @MrBones757 in astronomer#634
* Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM by @edgga in astronomer#615

Others

* Add pre-commit hook for McCabe max complexity check and fix errors by @jbandoro in astronomer#629
* Update contributing docs for running integration tests by @jbandoro in astronomer#638
* Fix CI issue running integration tests by @tatiana in astronomer#640 and astronomer#644
* pre-commit updates in astronomer#637

(cherry picked from commit fa0620a)
tatiana pushed a commit that referenced this pull request Jul 29, 2024
… Rendering and Execution (#634)

This MR finishes the work that was started in #605 to add full support
for ProjectConfig.dbt_project_path = None, and implements #568.

Within this PR, several things have been updated:
1 - Added project_path fields to RenderConfig and ExecutionConfig
2 - Simplified the consumption of RenderConfig in the dbtGraph class
3 - added option to configure different dbt executables for Rendering vs
Execution.

Closes: #568
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.

Support using different dbt paths for DAG rendering and dbt execution
2 participants