-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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 support for using SHA digest of Docker images. #30214
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
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.
We should also add some tests.
Co-authored-by: Jed Cunningham <[email protected]>
Is there anything else to do for this PR? |
We should add some test coverage to make sure the default vs "normal" image stuff still works with a digest. |
@jedcunningham Care to provide an example for a Helm chart test? |
The chart tests are here: https://github.com/apache/airflow/tree/main/tests/charts and they are testing if the templates render properly when you provide some config values, so it should be rather straightforward. |
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.
Also missing coverage that defaultAirflowDigest
works right (hint: I don't think it does).
Co-authored-by: Jed Cunningham <[email protected]>
@jedcunningham Sorry, care to clarify what do you mean by that? |
I'm saying that if you set a non-default It's might be worth having a new test case for it, instead of making that existing parametrized one more complex, see how it goes when you get in there. |
Hi @jedcunningham, care to take another look? |
@elongl, this looks good. Can you resolve the conflict when you get a chance? |
Hi @jedcunningham, thanks for addressing. |
Co-authored-by: Jed Cunningham <[email protected]>
Co-authored-by: Jed Cunningham <[email protected]>
@eladkal @jedcunningham Updated accordingly to the new format. |
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Thanks @elongl! Congrats on your first commit 🎉 If you'd like, adding support for the digest for the rest of the images would be a good contribution. |
@jedcunningham is there a specific reason to include digest notation for airflow only, not for all the other potential images used? statsd, git-sync, and so on? |
No - and feel free to add it. |
Closes: #30200.