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 LoadMode.AUTOMATIC behaviour to use LoadMode.DBT_LS when ProfileMapping is used #625

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

tatiana
Copy link
Collaborator

@tatiana tatiana commented Oct 25, 2023

Since #489 was merged, the behavior of LoadMode.AUTOMATIC changed to generate a profiles.yml file if the file didn't exist. However, we forgot to remove the previously necessary condition for being able to run LoadMode.DBT_LS (having the profiles.yml file).

This leads to inconsistent behaviour in Cosmos when using LoadMode.AUTOMATIC and the manifest.json was not available:

  1. If the user used a ProfileConfig with profiles_yml_filepath, it would use LoadMode.DBT_LS
  2. If the user used a ProfileConfig with a ProfileMapping class, it would unnecessarily use LoadMode.CUSTOM

This PR fixes the behaviour to attempt to use LoadMode.DBT_LS regardless of how the ProfileConfig was set.

@netlify
Copy link

netlify bot commented Oct 25, 2023

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name Link
🔨 Latest commit 5160170
🔍 Latest deploy log https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/653959b689beb90008c1f2b8

@tatiana tatiana temporarily deployed to internal October 25, 2023 15:20 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a4aa5cc) 93.37% compared to head (af09a3e) 93.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #625      +/-   ##
==========================================
- Coverage   93.37%   93.33%   -0.05%     
==========================================
  Files          53       53              
  Lines        2114     2114              
==========================================
- Hits         1974     1973       -1     
- Misses        140      141       +1     
Files Coverage Δ
cosmos/dbt/graph.py 98.77% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@tatiana tatiana temporarily deployed to internal October 25, 2023 16:09 — with GitHub Actions Inactive
@tatiana tatiana marked this pull request as ready for review October 25, 2023 16:13
@tatiana tatiana requested a review from a team as a code owner October 25, 2023 16:13
@tatiana tatiana requested a review from a team October 25, 2023 16:13
@tatiana tatiana temporarily deployed to internal October 25, 2023 16:28 — with GitHub Actions Inactive
Copy link
Contributor

@harels harels left a comment

Choose a reason for hiding this comment

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

lgtm, pending tests passing

@tatiana tatiana temporarily deployed to internal October 25, 2023 17:22 — with GitHub Actions Inactive
@tatiana tatiana force-pushed the fix-dbt-ls-by-default branch from af09a3e to 5160170 Compare October 25, 2023 18:08
@tatiana tatiana temporarily deployed to internal October 25, 2023 18:08 — with GitHub Actions Inactive
@tatiana tatiana merged commit ad7dcf0 into main Oct 25, 2023
37 checks passed
@tatiana tatiana deleted the fix-dbt-ls-by-default branch October 25, 2023 18:10
tatiana added a commit that referenced this pull request Oct 25, 2023
…fileMapping` is used (#625)

Since #489 was merged, the behavior of `LoadMode.AUTOMATIC` changed to
generate a `profiles.yml` file if the file didn't exist. However, we
forgot to remove the previously necessary condition for being able to
run `LoadMode.DBT_LS` (having the `profiles.yml` file).

This leads to inconsistent behaviour in Cosmos when using
`LoadMode.AUTOMATIC` and the `manifest.json` was not available:
1. If the user used a `ProfileConfig` with `profiles_yml_filepath`, it
would use `LoadMode.DBT_LS`
2. If the user used a `ProfileConfig` with a ProfileMapping class, it
would unnecessarily use `LoadMode.CUSTOM`

This PR fixes the behaviour to attempt to use `LoadMode.DBT_LS`
regardless of how the `ProfileConfig` was set.

(cherry picked from commit ad7dcf0)
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 tatiana added this to the 1.2.1 milestone 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)
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