[GitHub] [airflow] potiuk commented on a diff in pull request #27848: git-sync 3.6.1

[GitBox <gi...@apache.org>]

potiuk commented on code in PR #27848:
URL: https://github.com/apache/airflow/pull/27848#discussion_r1038644120


##########
chart/templates/_helpers.yaml:
##########
@@ -214,8 +214,10 @@ If release name contains chart name it will be used as a full name.
       value: "repo"
     - name: GIT_SYNC_ADD_USER
       value: "true"
-    - name: GIT_SYNC_WAIT
+    - name: GIT_SYNC_WAIT # Deprecated: use GIT_SYNC_PERIOD
       value: {{ .Values.dags.gitSync.wait | quote }}
+    - name: GIT_SYNC_PERIOD
+      value: {{ .Values.dags.gitSync.period | quote }}

Review Comment:
   Why do we need to change the parameter at all? Would not taht be enough (and I think @jedcunningham suggests it) use GIT_SYNC_PERIOD and pass the `.Values.dags.gitSync.wait` to it.
   
   While not super convenient, I do not see big value in changing the name. For most people who do not read the documentation but use autocomplete and such - it will be fine as they likely do not even look at git-sync docs. For those who read the documentation you can explain it..
   
   I think that should be enough?
   
   Or am I missing something?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org