Chart: Fix gitsync version handling #61027
Conversation
jedcunningham
requested review from
dstandish,
hussein-awala and
jscheffl
as code owners
We can't rely on the tag to determine the version. Instead, add a new `dags.gitSync.version` config.
jedcunningham
force-pushed
the
fixup_gitsync_version_handling
branch
from
58c7220 to
f920d7c
Compare
|
Why can we not rely on the tag? Which bug was in the previous code? For me it looked "proper". |
|
There was some reasoning presented in #52388 |
| Add ``dags.gitSync.version`` configuration | ||
|
|
||
| The chart now uses ``dags.gitSync.version`` to determine what version of git sync is used. The ``images.gitSync.tag`` value does not influence this. | ||
| The default ``dags.gitSync.version`` is ``4.4.2``. If you are using git sync v3, you must set ``dags.gitSync.version: 3.x.x`` explicitly. |
There was a problem hiding this comment.
If someone uses the 3rd version (I doubt that personally, but it is still supported), it will be a breaking change. Do we want to make this in that way (we are removing breaking change behaviour regarding the workers.celery section, so I think we should not introduce a different one in a different place)?
There was a problem hiding this comment.
Fair point. I agree it'd be very rare, but still breaking. I think celery is a very different situation just because of its popularity.
But, maybe we just revert and let it ride as-is for 1.19 and remove support for 3 in the next release completely, along with all the other cleanup we will be doing. Let me sleep on it.
There was a problem hiding this comment.
Then I'd favor the latter that we drop support for v3 at time when (soon) we drop support for Airflow 2 - to get the complexity out which is most probably not needed.
There was a problem hiding this comment.
Sounds good to me (if revert means reverting the #56245 PR)
Basically we can't assume the tag will be |
|
Closing in favor of #61098 |
4 participants
We can't rely on the tag to determine the version. Instead, add a new
dags.gitSync.versionconfig.Related: #56245
Was generative AI tooling used to co-author this PR?
Generated-by: Cursor (Claude Opus 4.5)