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

Added new profile mapping configuration for Teradata #1077

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

sc250072
Copy link
Contributor

@sc250072 sc250072 commented Jul 2, 2024

Description

Teradata has Provider in airflow and adapter in dbt. The cosmos library doesn't have profile configuration with mapping support. This PR address this issue.

Related Issue(s)

Closes #1053

Breaking Change?

No 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

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 2, 2024
Copy link

netlify bot commented Jul 2, 2024

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 2bb37a1
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/66850469680d0800080b3b39

@dosubot dosubot bot added the area:profile Related to ProfileConfig, like Athena, BigQuery, Clickhouse, Spark, Trino, etc label Jul 2, 2024
@sc250072 sc250072 mentioned this pull request Jul 2, 2024
1 task
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.

This looks great, @sc250072 , thank you very much for adding support to Teradata! I added some minor comments in-line

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.14%. Comparing base (30a7e5f) to head (2bb37a1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1077      +/-   ##
==========================================
+ Coverage   96.12%   96.14%   +0.02%     
==========================================
  Files          62       63       +1     
  Lines        3249     3266      +17     
==========================================
+ Hits         3123     3140      +17     
  Misses        126      126              

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

1. Removed mock_profile as it inherits the parent mock_profile
2. Removed TYPE_CHECKING
@sc250072
Copy link
Contributor Author

sc250072 commented Jul 2, 2024

This looks great, @sc250072 , thank you very much for adding support to Teradata! I added some minor comments in-line

Thank you @tatiana . Addressed the review comments.

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, @sc250072 for being so quick at addressing the comments. Happy for us to merge this PR once all the checks pass

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 2, 2024
@sc250072
Copy link
Contributor Author

sc250072 commented Jul 3, 2024

Thanks, @sc250072 for being so quick at addressing the comments. Happy for us to merge this PR once all the checks pass

Thank you for the quick response on the PR. Right now, I'm conducting integration testing by manually installing the .whl file on my local machine. Could you please provide any documentation or guidance on implementing integration tests that can be run through GitHub workflows?

@pankajastro
Copy link
Contributor

pankajastro commented Jul 3, 2024

We have test file https://github.com/astronomer/astronomer-cosmos/blob/main/tests/test_example_dags.py which run some dags of this repo https://github.com/astronomer/astronomer-cosmos/tree/main/dev/dags also you can find some tests decorated with @pytest.mark.integration runs as integration tests. But running integration tests from this repo may require some Teradata credentials so I'm not sure if you have those at the moment

Copy link
Contributor

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

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

lgtm

@tatiana
Copy link
Collaborator

tatiana commented Jul 3, 2024

We have test file https://github.com/astronomer/astronomer-cosmos/blob/main/tests/test_example_dags.py which run some dags of this repo https://github.com/astronomer/astronomer-cosmos/tree/main/dev/dags also you can find some tests decorated with @pytest.mark.integration runs as integration tests. But running integration tests from this repo may require some Teradata credentials, so I'm not sure if you have those at the moment.

@sc250072 complemeting @pankajastro 's message: #1077 (comment)

Could the Teradata team share specific credentials to run integration tests from the Cosmos repo to validate that these tests pass? In that case, we'd be happy to remove the community flag.

As @pankajastro mentioned, we could either have an example DAG with Teradata or other tests marked with integration.

Another alternative, similar to Airflow, is to run a Cosmos DAG using this profile in your own CI/CD against the Cosmos main branch and share a dashboard/page with us with the job status (if you could extend https://teradata.github.io/airflow/. It would be awesome). In this case, you wouldn't have to share credentials with us, and we could help guarantee the stability of this feature. In this case, we could remove the is_community flag again.

@tatiana
Copy link
Collaborator

tatiana commented Jul 3, 2024

@sc250072 this work looks great, we'll be merging this PR and we can address removing the is_community in a separate PR

@tatiana tatiana merged commit 1c9e1f5 into astronomer:main Jul 3, 2024
64 checks passed
@sc250072
Copy link
Contributor Author

sc250072 commented Jul 3, 2024

@sc250072 this work looks great, we'll be merging this PR and we can address removing the is_community in a separate PR

Thank you @tatiana .

@sc250072
Copy link
Contributor Author

sc250072 commented Jul 4, 2024

@pankajastro @tatiana any expected date of new version of astronomer-cosmos, which includes teradata support. https://github.com/astronomer/astronomer-cosmos/blob/main/CHANGELOG.rst

@tatiana tatiana added this to the Cosmos 1.6.0 milestone Jul 4, 2024
@tatiana
Copy link
Collaborator

tatiana commented Jul 4, 2024

@sc250072 it'll be released as part of 1.6, which is currently planned for the end of July.

Would you like us to create an alpha (e.g., 1.6.0a1) for you to validate the feature beforehand?

@sc250072
Copy link
Contributor Author

sc250072 commented Jul 4, 2024

@sc250072 it'll be released as part of 1.6, which is currently planned for the end of July.

Would you like us to create an alpha (e.g., 1.6.0a1) for you to validate the feature beforehand?

Yes that would be help to validate feature before release.

@tatiana tatiana mentioned this pull request Jul 5, 2024
@tatiana
Copy link
Collaborator

tatiana commented Jul 5, 2024

@sc250072
Copy link
Contributor Author

@tatiana I have tested Cosmos 1.6.0a1 and identified an issue with the Teradata mock profile, documented here: #1087. I have resolved this issue and submitted a pull request at #1088.

After addressing the issue, I validated the fix using a locally built .whl file and tested it with several Cosmos DAGs within the dbt-jaffle-shop project, utilizing the dbt-teradata adapter. Everything is functioning correctly now.

Could we proceed with another alpha release incorporating these changes?"

@tatiana
Copy link
Collaborator

tatiana commented Jul 10, 2024

Hi @sc250072 , thanks for all the work on this!

Yes, we'll be releasing a new alpha this week Nd we'll let you know

arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
Teradata has
[Provider](https://airflow.apache.org/docs/apache-airflow-providers-teradata/stable/index.html)
in airflow and [adapter](https://github.com/Teradata/dbt-teradata) in
dbt. The cosmos library doesn't have profile configuration with mapping
support. This PR address this issue.

Closes: astronomer#1053
@sc250072
Copy link
Contributor Author

Hi @sc250072 , thanks for all the work on this!

Yes, we'll be releasing a new alpha this week Nd we'll let you know

@tatiana any update on new alpha release

pankajkoti added a commit that referenced this pull request Aug 20, 2024
New Features

* Add support for loading manifest from cloud stores using Airflow
Object Storage by @pankajkoti in #1109
* Cache ``package-lock.yml`` file by @pankajastro in #1086
* Support persisting the ``LoadMode.VIRTUALENV`` directory by @tatiana
in #1079
* Add support to store and fetch ``dbt ls`` cache in remote stores by
@pankajkoti in #1147
* Add default source nodes rendering by @arojasb3 in #1107
* Add Teradata ``ProfileMapping`` by @sc250072 in #1077

Enhancements

* Add ``DatabricksOauthProfileMapping`` profile by @CorsettiS in #1091
* Use ``dbt ls`` as the default parser when ``profile_config`` is
provided by @pankajastro in #1101
* Add task owner to dbt operators by @wornjs in #1082
* Extend Cosmos custom selector to support + when using paths and tags
by @mvictoria in #1150
* Simplify logging by @dwreeves in #1108

Bug fixes

* Fix Teradata ``ProfileMapping`` target invalid issue by @sc250072 in
#1088
* Fix empty tag in case of custom parser by @pankajastro in #1100
* Fix ``dbt deps`` of ``LoadMode.DBT_LS`` should use
``ProjectConfig.dbt_vars`` by @tatiana in #1114
* Fix import handling by lazy loading hooks introduced in PR #1109 by
@dwreeves in #1132
* Fix Airflow 2.10 regression and add Airflow 2.10 in test matrix by
@pankajastro in #1162

Docs

* Fix typo in azure-container-instance docs by @pankajastro in #1106
* Use Airflow trademark as it has been registered by @pankajastro in
#1105

Others

* Run some example DAGs in Kubernetes execution mode in CI by
@pankajastro in #1127
* Install requirements.txt by default during dev env spin up by
@@CorsettiS in #1099
* Remove ``DbtGraph.current_version`` dead code by @tatiana in #1111
* Disable test for Airflow-2.5 and Python-3.11 combination in CI by
@pankajastro in #1124
* Pre-commit hook updates in #1074, #1113, #1125, #1144, #1154,  #1167

---------

Co-authored-by: Pankaj Koti <[email protected]>
Co-authored-by: Pankaj Singh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:profile Related to ProfileConfig, like Athena, BigQuery, Clickhouse, Spark, Trino, etc lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for dbt-teradata
4 participants