You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/11/22 22:48:18 UTC
[GitHub] [airflow] danielhoherd opened a new pull request, #27848: Use git-sync 3.6.1. Handle deprecations.
danielhoherd opened a new pull request, #27848:
URL: https://github.com/apache/airflow/pull/27848
- Update to git-sync 3.6.1, which is currently the latest.
- Use newer GIT_SYNC_PERIOD env var since GIT_SYNC_WAIT is deprecated.
- Replace deprecated env var `GIT_SYNC_SYNC_TIMEOUT` inside of example text.
--
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
[GitHub] [airflow] danielhoherd commented on a diff in pull request #27848: git-sync 3.6.3
Posted by "danielhoherd (via GitHub)" <gi...@apache.org>.
danielhoherd commented on code in PR #27848:
URL: https://github.com/apache/airflow/pull/27848#discussion_r1083071160
##########
chart/values.yaml:
##########
@@ -93,7 +93,7 @@ images:
pullPolicy: IfNotPresent
gitSync:
repository: k8s.gcr.io/git-sync/git-sync
- tag: v3.4.0
+ tag: v3.6.1
Review Comment:
OK, I undid everything except the tag version.
--
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
[GitHub] [airflow] danielhoherd commented on a diff in pull request #27848: git-sync 3.6.1
Posted by GitBox <gi...@apache.org>.
danielhoherd commented on code in PR #27848:
URL: https://github.com/apache/airflow/pull/27848#discussion_r1029890017
##########
chart/values.schema.json:
##########
@@ -5254,6 +5254,11 @@
"default": "tests/dags"
},
"wait": {
+ "description": "Interval between git sync attempts in seconds. High values are more likely to cause DAGs to become out of sync between different components. Low values cause more traffic to the remote git repository. (Deprecated. Use 'period')",
+ "type": "integer",
+ "default": 5
Review Comment:
@jedcunningham We chatted a bit about potentially changing this value. Is that something we want to ride in on this PR?
--
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
[GitHub] [airflow] danielhoherd commented on a diff in pull request #27848: git-sync 3.6.1
Posted by GitBox <gi...@apache.org>.
danielhoherd commented on code in PR #27848:
URL: https://github.com/apache/airflow/pull/27848#discussion_r1030783942
##########
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:
What is the advantage of delaying support of the new flag? New deployments can start using it right away and not have to migrate to it at a later date. Existing deployments can migrate during deprecation period to avoid a straight cut-over where the version and the helm values have to all be changes at the same time.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #27848: git-sync 3.6.1
Posted by 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
[GitHub] [airflow] jedcunningham commented on pull request #27848: git-sync 3.6.3
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on PR #27848:
URL: https://github.com/apache/airflow/pull/27848#issuecomment-1399076067
@danielhoherd, can you add a newsfragement for this ([here is an example](https://github.com/apache/airflow/blob/main/chart/newsfragments/28074.significant.rst))?
--
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
[GitHub] [airflow] danielhoherd commented on a diff in pull request #27848: git-sync 3.6.1
Posted by GitBox <gi...@apache.org>.
danielhoherd commented on code in PR #27848:
URL: https://github.com/apache/airflow/pull/27848#discussion_r1030785910
##########
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:
Oh, but I do see a problem with my implementation... Maybe a better way to do it is to only include `period`, and just use the value from `wait` in that place if it is given.
--
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
[GitHub] [airflow] jedcunningham merged pull request #27848: git-sync 3.6.3
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham merged PR #27848:
URL: https://github.com/apache/airflow/pull/27848
--
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
[GitHub] [airflow] jedcunningham commented on a diff in pull request #27848: git-sync 3.6.1
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #27848:
URL: https://github.com/apache/airflow/pull/27848#discussion_r1038648326
##########
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:
Yeah, that's basically where I was going.
We have to set both env vars so it works regardless of gitsync version. And it'll prefer `WAIT`, so `PERIOD` is never used anyways. We could do some more complicated logic to support both, but I agree sticking with just `wait` in our values seems easiest.
--
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
[GitHub] [airflow] jedcunningham commented on a diff in pull request #27848: git-sync 3.6.1
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #27848:
URL: https://github.com/apache/airflow/pull/27848#discussion_r1029975838
##########
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:
Since `wait` is used when both are set, we might as well stick with just `wait` here as we can't remove it (yet).
##########
chart/values.schema.json:
##########
@@ -5254,6 +5254,11 @@
"default": "tests/dags"
},
"wait": {
+ "description": "Interval between git sync attempts in seconds. High values are more likely to cause DAGs to become out of sync between different components. Low values cause more traffic to the remote git repository. (Deprecated. Use 'period')",
+ "type": "integer",
+ "default": 5
Review Comment:
We [just reduced this from 60](https://github.com/apache/airflow/pull/27625) to reduce the risk of them being out of sync. If you want to pitch another value, definitely so that in another PR.
##########
chart/values.yaml:
##########
@@ -93,7 +93,7 @@ images:
pullPolicy: IfNotPresent
gitSync:
repository: k8s.gcr.io/git-sync/git-sync
- tag: v3.4.0
+ tag: v3.6.1
Review Comment:
This single line might be all we end up keeping?
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #27848: git-sync 3.6.1
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #27848:
URL: https://github.com/apache/airflow/pull/27848#issuecomment-1386268871
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 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