-
Notifications
You must be signed in to change notification settings - Fork 177
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
adds profile_config for Docker #1347
adds profile_config for Docker #1347
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1347 +/- ##
=======================================
Coverage 96.28% 96.29%
=======================================
Files 68 68
Lines 4150 4152 +2
=======================================
+ Hits 3996 3998 +2
Misses 154 154 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a unit test to avoid regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @andrewhlui, thanks a lot for your contribution!
Some requests:
- Please add a test for this feature
- If possible, update the docs, mentioning any limitations of this feature (comment inline)
If you can address this before 20 December, we can release this feature as part of Cosmos 1.8
Added some tests, let me know if this works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a note about supporting profile_config
with profiles.yml
way and not ProfileMapping
way in the docs at https://astronomer.github.io/astronomer-cosmos/getting_started/execution-modes.html#docker
The source for the docs are located at https://github.com/astronomer/astronomer-cosmos/blob/main/docs/getting_started/execution-modes.rst#docker
IMO, we're good to merge once these suggestions are addressed
In the interest of time, I have tried to address my review comments in PR #1413. If those look good, perhaps we can merge this PR and rebase my PR cc: @tatiana @andrewhlui |
I won't have time to work on this for the next week -- so that approach sounds good to me. |
3629068
to
99672af
Compare
…tionMode.Docker (#1413) This PR addresses the open review comments on PR #1347 which adds profile_config to Docker execution mode. related: #1346 related: #1347 --------- Co-authored-by: Andrew Lui <[email protected]>
Description
Adds ProfileConfig to Docker operators so that people can use TaskGroups.
Related Issue(s)
Closes #1346
Breaking Change?
Checklist