You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "N-ickJones (via GitHub)" <gi...@apache.org> on 2023/02/28 18:45:11 UTC
[GitHub] [airflow] N-ickJones opened a new pull request, #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
N-ickJones opened a new pull request, #29818:
URL: https://github.com/apache/airflow/pull/29818
closes: #29817
When deploying Helm chart 1.8.0 the scheduler is expecting the airflow.cfg file to contain a kubernetes_executor section but, instead contains a kubernetes section. This change was introduced during the Airflow release v2.5.0 # `Rename kubernetes config section to kubernetes_executor` [#26873](https://github.com/apache/airflow/pull/26873)
--
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 #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on PR #29818:
URL: https://github.com/apache/airflow/pull/29818#issuecomment-1474465867
Thanks @N-ickJones! Congrats on your first commit 🎉
--
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] N-ickJones commented on pull request #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "N-ickJones (via GitHub)" <gi...@apache.org>.
N-ickJones commented on PR #29818:
URL: https://github.com/apache/airflow/pull/29818#issuecomment-1448938010
> Thanks for the PR @N-ickJones!
>
> Unfortunately, it's not quite as simple as your proposed change. The latest version of the chart still supports older Airflow versions. Check out what was done for the [logging config move](https://github.com/apache/airflow/blob/4b36137a31241d0f502604213546b6bf677fea69/chart/values.yaml#L1784-L1789).
For backwards compatibility do my updated changes work. Similar to the logging config move, add a duplicate section.
--
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] boring-cyborg[bot] commented on pull request #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #29818:
URL: https://github.com/apache/airflow/pull/29818#issuecomment-1448685222
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] N-ickJones commented on a diff in pull request #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "N-ickJones (via GitHub)" <gi...@apache.org>.
N-ickJones commented on code in PR #29818:
URL: https://github.com/apache/airflow/pull/29818#discussion_r1140582928
##########
chart/values.yaml:
##########
@@ -1822,6 +1822,8 @@ config:
ccache: '{{ .Values.kerberos.ccacheMountPath }}/{{ .Values.kerberos.ccacheFileName }}'
celery_kubernetes_executor:
kubernetes_queue: 'kubernetes'
+ # The `kubernetes` section is deprecated in Airflow >= 2.5.0 due to a airflow.cfg scheme change.
+ # The `kubernetes` section can be removed once the helm chart no longer supports Airflow < 2.5.0.
kubernetes:
namespace: '{{ .Release.Namespace }}'
airflow_configmap: '{{ include "airflow_config" . }}'
Review Comment:
The recent force pushes are to squash commits, clean the commit message, and update the fork to the latest of airflow main.
--
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 #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29818:
URL: https://github.com/apache/airflow/pull/29818#discussion_r1140706209
##########
chart/values.yaml:
##########
@@ -1888,14 +1888,24 @@ config:
ccache: '{{ .Values.kerberos.ccacheMountPath }}/{{ .Values.kerberos.ccacheFileName }}'
celery_kubernetes_executor:
kubernetes_queue: 'kubernetes'
+ # The `kubernetes` section is deprecated in Airflow >= 2.5.0 due to an airflow.cfg schema change.
Review Comment:
```suggestion
# The `kubernetes` section is deprecated in Airflow >= 2.5.0 due to an airflow.cfg schema change.
```
--
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 pull request #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29818:
URL: https://github.com/apache/airflow/pull/29818#issuecomment-1454828431
I think what @jedcunningham refers to is to add a comment explaining why this section is duplicated and ideally add condition on when it shoudl be removed.
--
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] N-ickJones commented on a diff in pull request #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "N-ickJones (via GitHub)" <gi...@apache.org>.
N-ickJones commented on code in PR #29818:
URL: https://github.com/apache/airflow/pull/29818#discussion_r1140508551
##########
chart/values.yaml:
##########
@@ -1830,6 +1830,14 @@ config:
worker_container_repository: '{{ .Values.images.airflow.repository | default .Values.defaultAirflowRepository }}'
worker_container_tag: '{{ .Values.images.airflow.tag | default .Values.defaultAirflowTag }}'
multi_namespace_mode: '{{ ternary "True" "False" .Values.multiNamespaceMode }}'
+ kubernetes_executor:
+ namespace: '{{ .Release.Namespace }}'
+ airflow_configmap: '{{ include "airflow_config" . }}'
+ airflow_local_settings_configmap: '{{ include "airflow_config" . }}'
Review Comment:
I removes those 2 properties and added comments.
--
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] boring-cyborg[bot] commented on pull request #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #29818:
URL: https://github.com/apache/airflow/pull/29818#issuecomment-1474465490
Awesome work, congrats on your first merged pull request!
--
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 #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham merged PR #29818:
URL: https://github.com/apache/airflow/pull/29818
--
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] N-ickJones commented on a diff in pull request #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "N-ickJones (via GitHub)" <gi...@apache.org>.
N-ickJones commented on code in PR #29818:
URL: https://github.com/apache/airflow/pull/29818#discussion_r1140527042
##########
chart/values.yaml:
##########
@@ -1822,6 +1822,8 @@ config:
ccache: '{{ .Values.kerberos.ccacheMountPath }}/{{ .Values.kerberos.ccacheFileName }}'
celery_kubernetes_executor:
kubernetes_queue: 'kubernetes'
+ # The `kubernetes` section is deprecated in Airflow >= 2.5.0 due to a airflow.cfg scheme change.
+ # The `kubernetes` section can be removed once the helm chart no longer supports Airflow < 2.5.0.
kubernetes:
namespace: '{{ .Release.Namespace }}'
airflow_configmap: '{{ include "airflow_config" . }}'
Review Comment:
note added
--
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 #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29818:
URL: https://github.com/apache/airflow/pull/29818#discussion_r1140483052
##########
chart/values.yaml:
##########
@@ -1830,6 +1830,14 @@ config:
worker_container_repository: '{{ .Values.images.airflow.repository | default .Values.defaultAirflowRepository }}'
worker_container_tag: '{{ .Values.images.airflow.tag | default .Values.defaultAirflowTag }}'
multi_namespace_mode: '{{ ternary "True" "False" .Values.multiNamespaceMode }}'
+ kubernetes_executor:
+ namespace: '{{ .Release.Namespace }}'
+ airflow_configmap: '{{ include "airflow_config" . }}'
+ airflow_local_settings_configmap: '{{ include "airflow_config" . }}'
Review Comment:
```suggestion
```
These 2 are for 1.10 and earlier versions, yeah? We definitely don't need to duplicate those!
--
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] N-ickJones commented on pull request #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "N-ickJones (via GitHub)" <gi...@apache.org>.
N-ickJones commented on PR #29818:
URL: https://github.com/apache/airflow/pull/29818#issuecomment-1474478751
> Thanks @N-ickJones! Congrats on your first commit 🎉
Thanks @jedcunningham @potiuk. I learned a lot from this experience.
--
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 #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29818:
URL: https://github.com/apache/airflow/pull/29818#discussion_r1140520464
##########
chart/values.yaml:
##########
@@ -1822,6 +1822,8 @@ config:
ccache: '{{ .Values.kerberos.ccacheMountPath }}/{{ .Values.kerberos.ccacheFileName }}'
celery_kubernetes_executor:
kubernetes_queue: 'kubernetes'
+ # The `kubernetes` section is deprecated in Airflow >= 2.5.0 due to a airflow.cfg scheme change.
+ # The `kubernetes` section can be removed once the helm chart no longer supports Airflow < 2.5.0.
kubernetes:
namespace: '{{ .Release.Namespace }}'
airflow_configmap: '{{ include "airflow_config" . }}'
Review Comment:
```suggestion
# The following `airflow_` entries are for Airflow 1, and can be removed when it is no longer supported.
airflow_configmap: '{{ include "airflow_config" . }}'
```
Let's add a note about these as well.
##########
chart/values.yaml:
##########
@@ -1822,6 +1822,8 @@ config:
ccache: '{{ .Values.kerberos.ccacheMountPath }}/{{ .Values.kerberos.ccacheFileName }}'
celery_kubernetes_executor:
kubernetes_queue: 'kubernetes'
+ # The `kubernetes` section is deprecated in Airflow >= 2.5.0 due to a airflow.cfg scheme change.
Review Comment:
```suggestion
# The `kubernetes` section is deprecated in Airflow >= 2.5.0 due to an airflow.cfg scheme change.
```
--
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] N-ickJones commented on a diff in pull request #29818: updates values.yaml config.kubernetes to config.kubernetes_executor
Posted by "N-ickJones (via GitHub)" <gi...@apache.org>.
N-ickJones commented on code in PR #29818:
URL: https://github.com/apache/airflow/pull/29818#discussion_r1140510710
##########
chart/values.yaml:
##########
@@ -1830,6 +1830,14 @@ config:
worker_container_repository: '{{ .Values.images.airflow.repository | default .Values.defaultAirflowRepository }}'
worker_container_tag: '{{ .Values.images.airflow.tag | default .Values.defaultAirflowTag }}'
multi_namespace_mode: '{{ ternary "True" "False" .Values.multiNamespaceMode }}'
+ kubernetes_executor:
+ namespace: '{{ .Release.Namespace }}'
+ airflow_configmap: '{{ include "airflow_config" . }}'
+ airflow_local_settings_configmap: '{{ include "airflow_config" . }}'
Review Comment:
Thanks for the help. Hopefully, I updated the PR appropriately.
--
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