-
Notifications
You must be signed in to change notification settings - Fork 15.1k
helm: add webserver_config.py to api server #50432
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
base: main
Are you sure you want to change the base?
helm: add webserver_config.py to api server #50432
Conversation
I think #50288 is related |
@@ -170,6 +170,9 @@ spec: | |||
resources: {{- toYaml .Values.apiServer.resources | nindent 12 }} | |||
volumeMounts: | |||
{{- include "airflow_config_mount" . | nindent 12 }} | |||
{{- if or .Values.apiServer.apiServerConfig .Values.apiServer.apiServerConfigConfigMapName }} |
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 think you can integrate this logic into the schedule, workers, and trigger deployment.
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.
example in schedule depoyment:
{{- if or .Values.apiServer.apiServerConfig .Values.apiServer.apiServerConfigConfigMapName (semverCompare ">=3.0.0" .Values.airflowVersion) }}
{{- include "airflow_api_server_config_mount" . | nindent 12 }}
{{- end }}
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.
But will it have an actual effect on the scheduler/worker/triggerer, or is it just making the chart unnecessarily more complex?
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.
In version 2.10.5, those are mounted also (here for example) but I don't think there are really needed.
Maybe you should not include them ?
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.
Yes, I checked gain, so it is unnecessary. Thanks!
Hi @GrumpyCat51! #50108 was just merged that added this to the api server deployment only, but could you rebase this PR on main to get it on the rest of the components? Thanks! |
This MR adds the
webserver_config.py
file to the api server, similar to how it was included to the webserver in version <3. I've had to add thewebserver_config.py
file when configuring OAuth in my local setup, where it worked as expected.Open question: In version <3, the
webserver_config.py
was mounted into all deployments (as well as the pod template for the KubernetesOperator) if defined. I didn't add it as I don't think it should be necessary, but please let me know if it has to be added