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-configs-in-profile #769

Closed
wants to merge 29 commits into from
Closed

Conversation

ykuc
Copy link
Contributor

@ykuc ykuc commented Dec 15, 2023

Description

#724

Related Issue(s)

Breaking Change?

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

@ykuc ykuc requested a review from a team as a code owner December 15, 2023 13:41
@ykuc ykuc requested a review from a team December 15, 2023 13:41
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 15, 2023
Copy link

netlify bot commented Dec 15, 2023

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name Link
🔨 Latest commit 504257d
🔍 Latest deploy log https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/6590ca3c4b2d890008027d66

@dosubot dosubot bot added area:config Related to configuration, like YAML files, environment variables, or executer configuration dbt:run Primarily related to dbt run command or functionality status:awaiting-reviewer The issue/PR is awaiting for a reviewer input labels Dec 15, 2023
@tatiana tatiana requested a review from jbandoro December 15, 2023 15:16
@tatiana
Copy link
Collaborator

tatiana commented Dec 15, 2023

@ykuc are you planning to add some tests?
@jbandoro you've been working with this lately, could you help reviewing this change?

@tatiana tatiana closed this Dec 15, 2023
@tatiana tatiana reopened this Dec 15, 2023
@tatiana tatiana added the status:awaiting-author Issue/PR is under discussion and waiting for author's input label Dec 15, 2023
Copy link
Collaborator

@jbandoro jbandoro left a comment

Choose a reason for hiding this comment

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

@ykuc thanks for opening up this PR, being able to add configs to the mapped profile is really cool and would be great to support! There are some items that need to be addressed below:

  1. Did you see that Support disabling event tracking when using Cosmos profile mapping #768 was merged that adds an option for disabling dbt event tracking, and is the reason why there are merge conflicts? Since dbt_config_vars also allows users to configure anonymous event tracking, even though Support disabling event tracking when using Cosmos profile mapping #768 has not been released yet, we should discuss if we're going to support both or only use dbt_config_vars and then update the Cosmos docs.
  2. As @tatiana commented, unit tests need to be added.
  3. Cosmos docs also need to be updated. Specifically this one: https://github.com/astronomer/astronomer-cosmos/blob/main/docs/templates/index.rst.jinja2

cosmos/config.py Outdated Show resolved Hide resolved
cosmos/config.py Outdated Show resolved Hide resolved
@ykuc
Copy link
Contributor Author

ykuc commented Dec 16, 2023

Hi, @jbandoro

  1. Sorry but I did see it... Do you think will we need this pr?
  2. As @tatiana commented, unit tests need to be added. Thanks see it, will fix it soon
  3. will do it

@jbandoro
Copy link
Collaborator

  1. Sorry but I did see it... Do you think will we need this pr?

This is great, I just wanted to flag it for you since your branch had merge conflicts and wasn't sure if we were both working on it at the same time.

Copy link

codecov bot commented Dec 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (090116e) 93.28% compared to head (d453648) 93.33%.
Report is 15 commits behind head on main.

❗ Current head d453648 differs from pull request most recent head 4b08586. Consider uploading reports for the commit 4b08586 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #769      +/-   ##
==========================================
+ Coverage   93.28%   93.33%   +0.05%     
==========================================
  Files          55       55              
  Lines        2502     2522      +20     
==========================================
+ Hits         2334     2354      +20     
  Misses        168      168              

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

@ykuc
Copy link
Contributor Author

ykuc commented Dec 30, 2023

done:

  1. fix type check
  2. add description and exception if profiles_yml_filepath is used with dbt_config_vars
  3. check configuration add-configs-in-profile #769 (comment)
  4. add unit tests
  5. update docs. Specifically this one: https://github.com/astronomer/astronomer-cosmos/blob/main/docs/templates/index.rst.jinja2

to do:

  1. fix merge conflicts Support disabling event tracking when using Cosmos profile mapping #768. I think we can use the best sides of our solutions:
    move dbt_config_vars to profile mapping because I think it's part of it
    use data class for dbt_config_vars

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 30, 2023
Copy link

netlify bot commented Jan 4, 2024

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 4b08586
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/659f4e075c89120008705f6a

YuriyK90 and others added 16 commits January 11, 2024 02:27
<!--pre-commit.ci start-->
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.1.7 →
v0.1.8](astral-sh/ruff-pre-commit@v0.1.7...v0.1.8)
- [github.com/psf/black: 23.11.0 →
23.12.0](psf/black@23.11.0...23.12.0)
<!--pre-commit.ci end-->

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start-->
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.1.8 →
v0.1.9](astral-sh/ruff-pre-commit@v0.1.8...v0.1.9)
- [github.com/psf/black: 23.12.0 →
23.12.1](psf/black@23.12.0...23.12.1)
- [github.com/pre-commit/mirrors-mypy: v1.7.1 →
v1.8.0](pre-commit/mirrors-mypy@v1.7.1...v1.8.0)
<!--pre-commit.ci end-->

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
[Airflow's upgraded to Pydantic 2.0
(!!)](apache/airflow#35551), so this removes the
restriction on Cosmos to ensure it's compatible.
…astronomer#779)

Apache Airflow 2.8 was released on 18 December 2023:
https://pypi.org/project/apache-airflow/2.8.0/

Add Airflow 2.8 to Cosmos integration tests matrix.

Update Airflow & dbt dependencies conflict document to include the new
conflicts introduced by this release:

https://astronomer.github.io/astronomer-cosmos/getting_started/execution-modes-local-conflicts.html#execution-modes-local-conflicts
)

## Description

I thought that there were a few aspects regarding execution modes that
could require more clarification in the docs.

- The parsing methods docs mentions that only `LOCAL` execution mode is
supported for `DBT_LS`, but the reverse was not true (i.e. execution
mode docs made no mention of parsing methods), so I added notes about
that.
- GCC docs suggest using `VIRTUALENV` execution mode, but makes no
mention of the fact that the `DBT_LS` parsing method is not supported in
this execution mode. Naturally, in this case, users should be utilizing
the `DBT_MANIFEST` load mode, but that means that the docs are
incomplete since they don't include a `manifest_path=?` in the
`ProjectConfig`.
- Note that there are also discussions in the Airflow Slack regarding
issues users have had parsing the `DbtDag` in GCC that are fixable via
using a pre-compiled `manifest,json`, e.g.
https://apache-airflow.slack.com/archives/C059CC42E9W/p1696435273519979
    - Also see astronomer#520 for more discussion.
- Generally speaking when doing the `DBT_MANIFEST` load method, the
pattern is that you run `dbt deps && dbt compile` as part of your
deployment, and upload your full dbt project including these artifacts.
This deployment approach may be obvious to veteran users of Airflow
and/or dbt, but it may not be obvious to everyone, so I think adding a
couple sentences in `parsing-methods.rst` is beneficial.

## Related Issue(s)

Not explicitly related, but astronomer#520 discusses some issues encountered using
the default parsing method. (Specifically, running `dbt deps` from a
blank slate tends to slow everything down a lot.)

Part of my motivation for adding to the docs is to better advertise +
better document this alternate method of parsing the dbt DAG.

## Breaking Change?

n/a

## Checklist

n/a
…stronomer#776)

The link to the quickstart doesn't actually take users to any kind of
quick start. So, instead, I've updated the link to point to the [getting
started
guide](https://astronomer.github.io/astronomer-cosmos/getting_started/astro.html)
**Features**

* Add new parsing method ``LoadMode.DBT_LS_FILE`` by @woogakoki in astronomer#733
([documentation](https://astronomer.github.io/astronomer-cosmos/configuration/parsing-methods.html#dbt-ls-file)).
* Add support to select using (some) graph operators when using
``LoadMode.CUSTOM`` and ``LoadMode.DBT_MANIFEST`` by @tatiana in astronomer#728
([documentation](https://astronomer.github.io/astronomer-cosmos/configuration/selecting-excluding.html#using-select-and-exclude))
* Add support for dbt ``selector`` arg for DAG parsing by @jbandoro in
astronomer#755,
([documentation](https://astronomer.github.io/astronomer-cosmos/configuration/render-config.html#render-config)).
* Add ``ProfileMapping`` for Vertica by @perttus in astronomer#540, astronomer#688 and astronomer#741,
as
([documentation](https://astronomer.github.io/astronomer-cosmos/profiles/VerticaUserPassword.html)).
* Add ``ProfileMapping`` for Snowflake encrypted private key path by
@ivanstillfront in astronomer#608, as ([documentation](
https://astronomer.github.io/astronomer-cosmos/profiles/SnowflakeEncryptedPrivateKeyFilePem.html)).
* Add support for Snowflake encrypted private key environment variable
by @DanMawdsleyBA in astronomer#649
* Add ``DbtDocsGCSOperator`` for uploading dbt docs to GCS by @jbandoro
in astronomer#616,
([documentation](https://astronomer.github.io/astronomer-cosmos/configuration/generating-docs.html#upload-to-gcs)).
* Add cosmos/propagate_logs Airflow config support for disabling log
propagation by @agreenburg in astronomer#648,
([documentation](https://astronomer.github.io/astronomer-cosmos/configuration/logging.html)).
* Add operator_args ``full_refresh`` as a templated field by @joppevos
in astronomer#623
* Expose environment variables and dbt variables in ``ProjectConfig`` by
@jbandoro in astronomer#735
([documentation](https://astronomer.github.io/astronomer-cosmos/configuration/project-config.html#project-config-example)).
* Support disabling event tracking when using Cosmos profile mapping by
@jbandoro in astronomer#768,
([documentation](https://astronomer.github.io/astronomer-cosmos/profiles/index.html#disabling-dbt-event-tracking)).

**Enhancements**

* Make Pydantic an optional dependency by @pixie79 in astronomer#736
* Create a symbolic link to ``dbt_packages`` when ``dbt_deps`` is False
when using ``LoadMode.DBT_LS`` by @DanMawdsleyBA in astronomer#730
* Add ``aws_session_token`` for Athena mapping by @benjamin-awd in astronomer#663
* Retrieve temporary credentials from ``conn_id`` for Athena by @octiva
in astronomer#758
* Extend ``DbtDocsLocalOperator`` with static flag by @joppevos  in astronomer#759

**Bug fixes**

* Remove Pydantic upper version restriction so Cosmos can be used with
Airflow 2.8 by @jlaneve in astronomer#772

**Others**

* Replace flake8 for Ruff by @joppevos in astronomer#743
* Reduce code complexity to 8 by @joppevos in astronomer#738
* Speed up integration tests by @jbandoro in astronomer#732
* Fix README quickstart link in by @RNHTTR in astronomer#776
* Add package location to work with hatchling 1.19.0 by @jbandoro in
astronomer#761
* Fix type check error in ``DbtKubernetesBaseOperator.build_env_args``
by @jbandoro in astronomer#766
* Improve ``DBT_MANIFEST`` documentation by @dwreeves in astronomer#757
* Update conflict matrix between Airflow and dbt versions by @tatiana in
astronomer#731 and astronomer#779
* pre-commit updates in astronomer#775, astronomer#770, astronomer#762
@ykuc ykuc marked this pull request as draft January 11, 2024 02:10
@ykuc
Copy link
Contributor Author

ykuc commented Jan 11, 2024

finish pr in #794

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:run Primarily related to dbt run command or functionality size:L This PR changes 100-499 lines, ignoring generated files. status:awaiting-author Issue/PR is under discussion and waiting for author's input status:awaiting-reviewer The issue/PR is awaiting for a reviewer input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants