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
| @@ -216,6 +216,34 @@ If release name contains chart name it will be used as a full name. | |||
| defaultMode: 288 | |||
| {{- end }} | |||
|
|
|||
| {{/* 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.
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.
@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!
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.