-
Notifications
You must be signed in to change notification settings - Fork 16.4k
feat(helm): Conditionally render GIT_SYNC_* vs GITSYNC_* env vars based on Git Sync version #52388
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
feat(helm): Conditionally render GIT_SYNC_* vs GITSYNC_* env vars based on Git Sync version #52388
Conversation
|
I forgot to add test code |
a295727 to
13a4caa
Compare
4603be7 to
86ecd9e
Compare
|
@jedcunningham Hello, I'm not familiar with open source project. Colud I request review my pr ? Thanks in advance. |
ca9883d to
e7e311e
Compare
e7e311e to
b87be66
Compare
|
|
||
| {{/* Helper to render git-sync credentials envs for v3/v4 */}} | ||
| {{- define "git_sync.env_credentials" }} | ||
| {{- $tag := .Values.images.gitSync.tag | default "v4" }} |
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 can't rely on the tag for this - we need to support folks who have mirrored the image and changed the tag names. If we want to add conditional logic, we will need to add a new version field for gitsync.
Also, we will want a significant newsfragment for this - this is bordering a breaking change and will want to make sure we message it appropriately.
Maybe we default the version to none and add both in that case, and only the right ones if the version is set? That's non breaking?
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.
@jedcunningham
Thank you so much for pointing this out — I had completely overlooked all of these important aspects.
Regarding the following part:
Also, we will want a significant newsfragment for this - this is bordering a breaking change and will want to make sure we message it appropriately.
I'm not very familiar with how newsfragments work. Would you mind explaining this in a bit more detail? I want to make sure I follow the correct process, especially since this change is close to being a breaking one.
Thanks again for your guidance!
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.
You can read more about it here:
| * Consider adding a newsfragment to your PR so you can add an entry in the release notes. |
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.
Thank you so much
|
I’m currently reproducing the test environment and running tests. I’ll finish it as soon as possible. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
This PR addresses Issue #52320 by introducing conditional logic in the Helm Chart to support both Git Sync v3 and v4 credential environment variable formats.
Background
In Git Sync:
GIT_SYNC_USERNAME,GIT_SYNC_PASSWORDGITSYNC_USERNAME,GITSYNC_PASSWORDPrior to this change, the Airflow Helm Chart rendered all four variables, regardless of which version of Git Sync was being used. This led to confusion and potential misconfigurations.
What this PR changes
Added a helper function
git_sync.env_credentialsin_helpers.tplto conditionally render environment variables for git-sync v3 (
GIT_SYNC_*) and v4 (GITSYNC_*) based on image tag.Replaced hardcoded credential environment variable blocks in Helm templates
with the new
include-based helper for better maintainability and correctness.Added two new parameterized unit tests:
test_git_sync_credentials_env_vars(for pod-template-file)test_scheduler_git_sync_env_vars(for scheduler-deployment)Both test the expected env vars for v3 and v4 using
@pytest.mark.parametrize.Motivation
Previously, git-sync credential environment variables were rendered unconditionally for both v3 and v4,
causing redundancy and potential YAML issues. Also, tests for those were hard to maintain and unclear
when failures occurred due to overlapping expectations.
Benefits
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.