-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
fix(helm-chart): dags gitsync credentials #43264
base: main
Are you sure you want to change the base?
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add unit test for this?
I added them here: 2b70391 |
It was a really quick solution, great one! @tobimichael96 |
valueFrom: | ||
secretKeyRef: | ||
name: {{ .Values.dags.gitSync.credentialsSecret | quote }} | ||
key: GITSYNC_USERNAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We at least need to add a newsfragment that these will no longer work and it'll use GIT_SYNC_X
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. But I have no idea where and how to put that. Feel free to add/suggest something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a question, what you think about a solution like this
{{- $gitUsernameKey := if (hasKey .Values.dags.gitSync.credentialsSecret "GITSYNC_USERNAME") "GITSYNC_USERNAME" "GIT_SYNC_USERNAME" -}}
key: $gitUsernameKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, but it's in the secret, not in values so that won't work :(
@tobimichael96, you can read about how to do a newsfragment over in the contributing docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the fragment, let me know if that works for you.
Any updates on this here? From my perspective it could be merged. |
Looks good to me. |
…read only required variables according to the gitsync version. Signed-off-by: Tobias Effner <[email protected]>
Signed-off-by: Tobias Effner <[email protected]>
…ronment variables for gitSync Signed-off-by: Tobias Effner <[email protected]>
… set Signed-off-by: Tobias Effner <[email protected]>
… keys Signed-off-by: Tobias Effner <[email protected]>
Signed-off-by: Tobias Effner <[email protected]>
Signed-off-by: Tobias Effner <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but as discussed it, we should likely bundle it with a number of other backwards-incompatible changes to the helm chart and relase a new MAJOR release then.
closes: #43247
This adds a condition to the env vars being added to the gitsync sidecar. The expected keys are in sync with what is documentation here: https://github.com/romsharon98/airflow/blob/main/chart/values.yaml#L2738