Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kyungjunleeme
Copy link
Contributor

@kyungjunleeme kyungjunleeme commented Jun 28, 2025

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:

  • v3 expects: GIT_SYNC_USERNAME, GIT_SYNC_PASSWORD
  • v4 expects: GITSYNC_USERNAME, GITSYNC_PASSWORD

Prior 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_credentials in _helpers.tpl
    to 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

  • Cleaner and version-specific environment variable rendering
  • Deterministic and clear testing logic per version
  • Easier to maintain or extend in the future (e.g. sshKey or knownHosts support)

^ 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.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Jun 28, 2025
@kyungjunleeme kyungjunleeme marked this pull request as draft June 28, 2025 06:55
@kyungjunleeme
Copy link
Contributor Author

I forgot to add test code

@kyungjunleeme kyungjunleeme force-pushed the feat/helm-gitsync-env-support branch 3 times, most recently from a295727 to 13a4caa Compare June 29, 2025 05:19
@kyungjunleeme kyungjunleeme marked this pull request as ready for review June 29, 2025 05:31
@kyungjunleeme kyungjunleeme force-pushed the feat/helm-gitsync-env-support branch 6 times, most recently from 4603be7 to 86ecd9e Compare June 29, 2025 22:49
@kyungjunleeme
Copy link
Contributor Author

@jedcunningham Hello, I'm not familiar with open source project. Colud I request review my pr ? Thanks in advance.

@eladkal eladkal requested a review from romsharon98 June 30, 2025 12:20
@kyungjunleeme kyungjunleeme force-pushed the feat/helm-gitsync-env-support branch 2 times, most recently from ca9883d to e7e311e Compare June 30, 2025 12:24
@kyungjunleeme kyungjunleeme force-pushed the feat/helm-gitsync-env-support branch from e7e311e to b87be66 Compare June 30, 2025 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant