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 ExecutionConfig.dbt_executable_path to use default_factory #678

Merged

Conversation

jbandoro
Copy link
Collaborator

@jbandoro jbandoro commented Nov 16, 2023

Description

I was seeing unit tests failing locally for dbt.test_graph.test_load_via_dbt_ls_with_invalid_dbt_path because I have dbt installed on my system and after the fix in #666 this test will fail since it fallbacks to the system dbt if it exists instead of "dbt" which the test is expecting.

This PR changes ExecutionConfig.dbt_executable_path to use a dataclass default factory for get_system_dbt so it is not called when ExecutionConfig is defined and allows us to more easily patch the shutil.which in the test so it won't fail if there is a system dbt installation.

Related Issue(s)

None

Breaking Change?

None

@jbandoro jbandoro requested a review from a team as a code owner November 16, 2023 00:22
@jbandoro jbandoro requested a review from a team November 16, 2023 00:22
Copy link

netlify bot commented Nov 16, 2023

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name Link
🔨 Latest commit 0192d4d
🔍 Latest deploy log https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/655560b33c5ce60008803e67

@dosubot dosubot bot added area:config Related to configuration, like YAML files, environment variables, or executer configuration dbt:test Primarily related to dbt test command or functionality execution:local Related to Local execution environment labels Nov 16, 2023
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (773fa27) 92.76% compared to head (0192d4d) 92.76%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #678   +/-   ##
=======================================
  Coverage   92.76%   92.76%           
=======================================
  Files          55       55           
  Lines        2238     2238           
=======================================
  Hits         2076     2076           
  Misses        162      162           

☔ 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.

Thanks for this fix, @jbandoro !

@tatiana tatiana merged commit 709ba44 into astronomer:main Nov 16, 2023
42 checks passed
@tatiana tatiana added this to the 1.2.5 milestone Nov 23, 2023
This was referenced Nov 23, 2023
@tatiana
Copy link
Collaborator

tatiana commented Nov 23, 2023

pull bot pushed a commit to pgoslatara/astronomer-cosmos that referenced this pull request Nov 24, 2023
Bug fixes

* Fix running models that use alias while supporting dbt versions by
@binhnq94 in astronomer#662
* Make profiles_yml_path optional for ExecutionMode.DOCKER and
KUBERNETES by @MrBones757 in astronomer#681
* Prevent overriding dbt profile fields with profile args of type or
method by @jbandoro in astronomer#702
* Fix LoadMode.DBT_LS fail when dbt outputs WarnErrorOptions by
@adammarples in astronomer#692
* Add support for env vars in RenderConfig for dbt ls parsing by
@jbandoro in astronomer#690
* Add support for Kubernetes on_warning_callback by @david-mag in astronomer#673
* Fix ExecutionConfig.dbt_executable_path to use ``default_factory`` by
@jbandoro in astronomer#678

Others

* Docs fix: example DAG in the README and docs/index by @tatiana in astronomer#705
* Docs improvement: highlight DAG examples in README by @iancmoritz and
@jlaneve in astronomer#695

(cherry picked from commit 2878d6a)
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
Bug fixes

* Fix running models that use alias while supporting dbt versions by
@binhnq94 in astronomer#662
* Make profiles_yml_path optional for ExecutionMode.DOCKER and
KUBERNETES by @MrBones757 in astronomer#681
* Prevent overriding dbt profile fields with profile args of type or
method by @jbandoro in astronomer#702
* Fix LoadMode.DBT_LS fail when dbt outputs WarnErrorOptions by
@adammarples in astronomer#692
* Add support for env vars in RenderConfig for dbt ls parsing by
@jbandoro in astronomer#690
* Add support for Kubernetes on_warning_callback by @david-mag in astronomer#673
* Fix ExecutionConfig.dbt_executable_path to use ``default_factory`` by
@jbandoro in astronomer#678

Others

* Docs fix: example DAG in the README and docs/index by @tatiana in astronomer#705
* Docs improvement: highlight DAG examples in README by @iancmoritz and
@jlaneve in astronomer#695

(cherry picked from commit 2878d6a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config Related to configuration, like YAML files, environment variables, or executer configuration dbt:test Primarily related to dbt test command or functionality execution:local Related to Local execution environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants