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: missed condition in adding vars args #1543

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlexandrKhabarov
Copy link
Contributor

Description

DbtToAirflowConverter can pass dbt_vars to DbtGraph with help of ProjectConfig or operator_args. If operator_args is used in DbtToAirflowConverter then it will lead to the issue with absence of dbt_vars in dbt ls command (faced rendering issue during usage of cosmos with project level variables)

Related Issue(s)

Breaking Change?

No breaking changes

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

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Feb 17, 2025
@dosubot dosubot bot added area:rendering Related to rendering, like Jinja, Airflow tasks, etc area:testing Related to testing, like unit tests, integration tests, etc labels Feb 17, 2025
Copy link

netlify bot commented Feb 17, 2025

Deploy Preview for sunny-pastelito-5ecb04 canceled.

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

@AlexandrKhabarov
Copy link
Contributor Author

@tatiana can you take a look, please?

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.30%. Comparing base (968b80e) to head (cf45616).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1543   +/-   ##
=======================================
  Coverage   97.30%   97.30%           
=======================================
  Files          80       80           
  Lines        4854     4856    +2     
=======================================
+ Hits         4723     4725    +2     
  Misses        131      131           

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

Comment on lines 379 to +382
if self.project.dbt_vars:
cmd_args.extend(["--vars", json.dumps(self.project.dbt_vars, sort_keys=True)])
elif self.dbt_vars:
cmd_args.extend(["--vars", json.dumps(self.dbt_vars, sort_keys=True)])
Copy link
Contributor

@pankajkoti pankajkoti Feb 19, 2025

Choose a reason for hiding this comment

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

Suggested change
if self.project.dbt_vars:
cmd_args.extend(["--vars", json.dumps(self.project.dbt_vars, sort_keys=True)])
elif self.dbt_vars:
cmd_args.extend(["--vars", json.dumps(self.dbt_vars, sort_keys=True)])
if self.dbt_vars:
cmd_args.extend(["--vars", json.dumps(self.dbt_vars, sort_keys=True)])

This should yield the same result, right?

@tatiana Was there a specific reason we checked for dbt_vars only in self.project instead of directly using self.dbt_vars while working on #1114?

@AlexandrKhabarov While Tatiana(she is Out of Office this week) provides insights on this, logically speaking, wouldn’t it make more sense to use the variables set at the project level during parsing/graph building rather than the ones set for the operator(I would incline to think that operator args would be used for each of the operator tasks during execution)? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I saw this part of the code and decided that project level variables have higher priority than operator vars and here can be two different approaches to pass variables to dbt (we are passing dbt variables via operator_args)

And this PR was intended to follow to the same logic in listing resource of dbt

Also, project level variables can be the same as variables of operator, right? Listing resource of dbt will lead to duplicated logic on client side (I will need to pass the same variable in two places)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rendering Related to rendering, like Jinja, Airflow tasks, etc area:testing Related to testing, like unit tests, integration tests, etc size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants