helm: extend webserver_config.py to all deployments #51923
Conversation
Move and adapt changes from https://github.com/apache/airflow/pull/50432/files#
|
I guess that one is needed (and rc3?) @jedcunningham ? |
|
Not sure if this is in scope for this MR, but should I add a checksum for this configmap to the annotations setting? |
| @@ -218,7 +218,7 @@ spec: | |||
| {{- include "custom_airflow_environment" . | indent 10 }} | |||
| {{- include "standard_airflow_environment" . | indent 10 }} | |||
| {{- include "container_extra_envs" (list . .Values.apiServer.env) | indent 10 }} | |||
| {{- if and (.Values.dags.gitSync.enabled) (not .Values.dags.persistence.enabled) (semverCompare "<2.0.0" .Values.airflowVersion) }} | |||
| {{- if and (.Values.dags.gitSync.enabled) (not .Values.dags.persistence.enabled) }} | |||
There was a problem hiding this comment.
Given that the previous implementation is basically if false as the chart is only enabled for airflowVersion>=3.0.0, should this actually be removed or left with the changes in this MR?
There was a problem hiding this comment.
Technically the chart still supports Airflow 1, so we can't cut this now.
There was a problem hiding this comment.
But the whole api-server-deployment is only active for Airflow >= 3.0.0 (
), so this if statement can never be true. So I'd either remove it completely, or fix it to its intended behaviour (not quite sure what is the expected behaviour in this regard, hence the question)| @@ -218,7 +218,7 @@ spec: | |||
| {{- include "custom_airflow_environment" . | indent 10 }} | |||
| {{- include "standard_airflow_environment" . | indent 10 }} | |||
| {{- include "container_extra_envs" (list . .Values.apiServer.env) | indent 10 }} | |||
| {{- if and (.Values.dags.gitSync.enabled) (not .Values.dags.persistence.enabled) (semverCompare "<2.0.0" .Values.airflowVersion) }} | |||
| {{- if and (.Values.dags.gitSync.enabled) (not .Values.dags.persistence.enabled) }} | |||
There was a problem hiding this comment.
Technically the chart still supports Airflow 1, so we can't cut this now.
| @@ -79,8 +79,10 @@ spec: | |||
| {{- if .Values.workers.extraVolumeMounts }} | |||
| {{- tpl (toYaml .Values.workers.extraVolumeMounts) . | nindent 8 }} | |||
| {{- end }} | |||
| {{- if or .Values.webserver.webserverConfig .Values.webserver.webserverConfigConfigMapName }} | |||
| {{- if and (semverCompare "<3.0.0" .Values.airflowVersion) (or .Values.webserver.webserverConfig .Values.webserver.webserverConfigConfigMapName) }} | |||
There was a problem hiding this comment.
This is a bit more nuanced I believe - webserver config is still necessary for the FAB provider right?
There was a problem hiding this comment.
Yes but when it's airflow >=3.0.0 we can use apiServerConfig or apiServerConfigConfigMapName.
Here an example
There was a problem hiding this comment.
But for Airflow >= 3.0.0, the webserver config used is defined within the api-server settings, while previously it was defined in the webserver settings. That's why this MR adds all the if - else if clauses (or maybe I just miss something completely).
Move and adapt changes from https://github.com/apache/airflow/pull/50432/files#.
Also fixes an error from #50108 where the old webserverConfig is referenced: https://github.com/humit0/airflow/blob/844c9ebcb355b8a3558ab325f3057cef7de76f03/chart/templates/_helpers.yaml#L586
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.