Skip to content

Operator argument fix#1648

Merged
tatiana merged 60 commits into
astronomer:mainfrom
johnhoran:metaclass
Apr 29, 2025
Merged

Operator argument fix#1648
tatiana merged 60 commits into
astronomer:mainfrom
johnhoran:metaclass

Conversation

@johnhoran
Copy link
Copy Markdown
Contributor

@johnhoran johnhoran commented Mar 28, 2025

Description

Cosmos 1.9.0 introduced a change such that AbstractDbtBase no longer inherits from airflow BaseOperator and I believe this introduced two bugs.

  • In the past you could give arguments using airflows default_args and these would end up being passed to cosmos. So for example we were specifying the project_dir as a default arg, and in 1.9.0 these projects were broken because AbstractDbtBase was not longer getting a project_dir specified. So the first change is to check both kwargs and default_args for the values AbstractDbtBase requires. I've only made this change for the kubernetes operator, but I'd argue it should be done for the local a docker ones too.
  • The existing code tries to inspect KubernetesPodOperator for the args that it requires, but notably this approach misses arguments that are required by BaseOperator, e.g. the task_group.
  • I've made an effort to restore the convention in airflow that all arguments must be consumed by the operator. So an argument must either be consumed by AbstractDbtBase or it will be passed along to the operator, it may also be consumed by both classes.

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

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 28, 2025

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 248be16
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/68109ba3a854af0008183c6d

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. execution:kubernetes Related to Kubernetes execution environment labels Mar 28, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.58%. Comparing base (441f21a) to head (248be16).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1648      +/-   ##
==========================================
+ Coverage   90.70%   97.58%   +6.88%     
==========================================
  Files          83       83              
  Lines        5174     5174              
==========================================
+ Hits         4693     5049     +356     
+ Misses        481      125     -356     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 31, 2025
@johnhoran johnhoran marked this pull request as draft April 4, 2025 11:33
@johnhoran johnhoran marked this pull request as ready for review April 4, 2025 19:13
@dosubot dosubot Bot added the area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc label Apr 4, 2025
@johnhoran johnhoran marked this pull request as draft April 23, 2025 15:22
@pankajkoti
Copy link
Copy Markdown
Contributor

hi @johnhoran thanks for sticking through this PR. Looks like the integration test is failing in the CI. Would you be able to take a look at it?

We are aiming to release Cosmos 1.10 in the upcoming week and would be really nice to see if we can have this fix in and included in the release.

@johnhoran
Copy link
Copy Markdown
Contributor Author

Hi @pankajkoti sorry this has gone around so many times. The test is passing on my end now with airflow2.4, so hopefully this is the last run.

@pankajkoti
Copy link
Copy Markdown
Contributor

Hi @pankajkoti sorry this has gone around so many times. The test is passing on my end now with airflow2.4, so hopefully this is the last run.

thanks @johnhoran . I re-triggered the CI for running integration tests. We can definitely re-trigger more if need be. And please don't be sorry :), all you're doing is helping the community with the fix and also investing your time here, that's much appreciated 🙇🏽

Copy link
Copy Markdown
Contributor

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot @johnhoran for fixing these 🙇🏽 👏🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc area:testing Related to testing, like unit tests, integration tests, etc execution:kubernetes Related to Kubernetes execution environment 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.

4 participants