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

Add support for env vars in RenderConfig for dbt ls parsing #690

Merged

Conversation

jbandoro
Copy link
Collaborator

@jbandoro jbandoro commented Nov 18, 2023

Description

Currently there is a workaround to have environment variables that are required when parsing a dbt project with the dbt ls load mode by setting them with os.environ in the DAG file.

This is what is currently done in the cosmos dev dag here since that env var is required for parsing with dbt ls. The problem with setting os.environ in that python file is that for the sqlite integration test it was enabling this test to unexpectedly pass (which also requires that env var).

This PR adds support for env_vars as an argument for RenderConfig and sets/unsets the environment variables in a context manager for the dbt ls graph parsing.

Related Issue(s)

closes #550 and closes #646

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

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

netlify bot commented Nov 18, 2023

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name Link
🔨 Latest commit 06c4b80
🔍 Latest deploy log https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/655e4aa9f97ec50008714605

@dosubot dosubot bot added area:config Related to configuration, like YAML files, environment variables, or executer configuration dbt:list Primarily related to dbt list command or functionality parsing:dbt_ls Issues, questions, or features related to dbt_ls parsing labels Nov 18, 2023
Copy link

codecov bot commented Nov 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (24aa38e) 92.93% compared to head (cb136c2) 92.97%.

❗ Current head cb136c2 differs from pull request most recent head 06c4b80. Consider uploading reports for the commit 06c4b80 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #690      +/-   ##
==========================================
+ Coverage   92.93%   92.97%   +0.04%     
==========================================
  Files          55       55              
  Lines        2292     2306      +14     
==========================================
+ Hits         2130     2144      +14     
  Misses        162      162              

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

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Nov 21, 2023
@tatiana tatiana added this to the 1.3.0 milestone Nov 22, 2023
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.

Hey @jbandoro this change looks great, thank you!

Moving forward, we need to find a way so users don't need to set this type of information both in the RenderConfig and in the ExecutionConfig - since, in most cases, they will want the same environment vars defined in both places. A related recent request was to set dbt --vars in a way they are reused both in RenderConfig and ExecutionConfig.

Would ProjectConfig be a more suitable place for this? Or should we have a new config object, altogether?

@jlaneve would love your thoughts on having a unified interface as well

@tatiana tatiana modified the milestones: 1.3.0, 1.2.5 Nov 22, 2023
@jbandoro
Copy link
Collaborator Author

Moving forward, we need to find a way so users don't need to set this type of information both in the RenderConfig and in the ExecutionConfig - since, in most cases, they will want the same environment vars defined in both places. A related recent request was to set dbt --vars in a way they are reused both in RenderConfig and ExecutionConfig.

Would ProjectConfig be a more suitable place for this? Or should we have a new config object, altogether?

I agree that having to specify env variables twice, once for parsing and again for execution is not ideal, and I can't think of a reason why they would be different? For example in the example cosmos source dev dag we have:

render_config = RenderConfig(
node_converters={
DbtResourceType("source"): convert_source, # known dbt node type to Cosmos (part of DbtResourceType)
DbtResourceType("exposure"): convert_exposure, # dbt node type new to Cosmos (will be added to DbtResourceType)
},
env_vars={"DBT_SQLITE_PATH": DBT_SQLITE_PATH},
)
example_cosmos_sources = DbtDag(
# dbt/cosmos-specific parameters
project_config=ProjectConfig(
DBT_ROOT_PATH / "simple",
),
profile_config=profile_config,
render_config=render_config,
operator_args={"env": {"DBT_SQLITE_PATH": DBT_SQLITE_PATH}},
# normal dag parameters
schedule_interval="@daily",
start_date=datetime(2023, 1, 1),
catchup=False,
dag_id="example_cosmos_sources",
)

I agree It would be cleaner and less confusing to have env vars specified in one place like ProjectConfig, and deprecate the env in the operator args and the RenderConfig here.

@tatiana tatiana merged commit 1734365 into astronomer:main Nov 23, 2023
39 checks passed
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)
tatiana added a commit that referenced this pull request Dec 5, 2023
[Justin Bandoro](https://www.linkedin.com/in/justin-bandoro-592b14a7/)
(@jbandoro) is a Data Engineer at Kevala Inc. He's based in San
Francisco (USA) and has been an early adopter of Cosmos, using it
regularly at his company.

Not only has he been using Cosmos since the early stages, but he has
consistently improved Cosmos since January 2023:
![Screenshot 2023-12-04 at 16 28
29](https://github.com/astronomer/astronomer-cosmos/assets/272048/43197938-d1ab-431f-b101-b6026e5cd3ab)

Some of his contributions include new features, code quality,
documentation and overall improvements. Some examples:
* Speed up integration tests in 67% #732 
* Prevent override of dbt profile fields #702
* Add support for env vars in `RenderConfig` in #690 
* Use symbolic links to run local tasks, avoiding to copy potentially
huge dbt project folders in #660
* Improve documentation in #638
* Automated and improved the code complexity checks in #629
* Added `DbtDocsGCSOperator` in #616 
* Added support for Python 3.7 in #88 and #214

Additionally, he has been interacting with users in the #airflow-dbt
Slack channel in a very collaborative and supportive way.

We want to promote him as a Cosmos committer and maintainer for all
these, recognising his constant efforts and achievements towards our
community. Thank you very much, @jbandoro !
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
…er#690)

Currently, there is a workaround to have environment variables that are
required when parsing a dbt project with the dbt ls load mode by setting
them with `os.environ` in the DAG file.

This is what is currently done in the cosmos dev dag
[here](https://github.com/astronomer/astronomer-cosmos/blob/e23a445b30ca391842dae870260cc7ce799d4d5c/dev/dags/example_cosmos_sources.py#L29)
since that env var is required for parsing with dbt ls. The problem with
setting `os.environ` in that python file is that for the sqlite
integration test it was enabling this
[test](https://github.com/astronomer/astronomer-cosmos/blob/e23a445b30ca391842dae870260cc7ce799d4d5c/tests/dbt/test_graph.py#L388)
to unexpectedly pass (which also requires that env var).

This PR adds support for `env_vars` as an argument for `RenderConfig`
and sets/unsets the environment variables in a context manager for the
dbt ls graph parsing.

Closes: astronomer#5
Closes: astronomer#646
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)
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
[Justin Bandoro](https://www.linkedin.com/in/justin-bandoro-592b14a7/)
(@jbandoro) is a Data Engineer at Kevala Inc. He's based in San
Francisco (USA) and has been an early adopter of Cosmos, using it
regularly at his company.

Not only has he been using Cosmos since the early stages, but he has
consistently improved Cosmos since January 2023:
![Screenshot 2023-12-04 at 16 28
29](https://github.com/astronomer/astronomer-cosmos/assets/272048/43197938-d1ab-431f-b101-b6026e5cd3ab)

Some of his contributions include new features, code quality,
documentation and overall improvements. Some examples:
* Speed up integration tests in 67% astronomer#732 
* Prevent override of dbt profile fields astronomer#702
* Add support for env vars in `RenderConfig` in astronomer#690 
* Use symbolic links to run local tasks, avoiding to copy potentially
huge dbt project folders in astronomer#660
* Improve documentation in astronomer#638
* Automated and improved the code complexity checks in astronomer#629
* Added `DbtDocsGCSOperator` in astronomer#616 
* Added support for Python 3.7 in astronomer#88 and astronomer#214

Additionally, he has been interacting with users in the #airflow-dbt
Slack channel in a very collaborative and supportive way.

We want to promote him as a Cosmos committer and maintainer for all
these, recognising his constant efforts and achievements towards our
community. Thank you very much, @jbandoro !
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:list Primarily related to dbt list command or functionality parsing:dbt_ls Issues, questions, or features related to dbt_ls parsing size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
2 participants