Skip to content

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

GrumpyCat51
Copy link
Contributor


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 the webserver_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

@eladkal eladkal requested a review from romsharon98 May 12, 2025 15:20
@GrumpyCat51
Copy link
Contributor Author

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 }}
Copy link

@nhuantho nhuantho May 14, 2025

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.

Copy link

@nhuantho nhuantho May 14, 2025

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 }}

Copy link
Contributor Author

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?

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 ?

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!

@jedcunningham
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants