-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
updates values.yaml config.kubernetes to config.kubernetes_executor #29818
updates values.yaml config.kubernetes to config.kubernetes_executor #29818
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.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.
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.
For backwards compatibility do my updated changes work. Similar to the logging config move, add a duplicate section. |
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. |
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.
Overall, looks good. A couple comments and we should be good to merge this!
6200d20
to
93b73ca
Compare
For backwards compatibility of Airflow <= 2.5.0 keeps the existing values.yaml `config.kubernetes` and add a new `config.kubernetes_executor` section. values.yaml `config.kubernetes` deprecation in Airflow >= 2.5.0. values.yaml `config.kubernetes` deprecation of `airflow_` entries.
93b73ca
to
2f18ada
Compare
Awesome work, congrats on your first merged pull request! |
Thanks @N-ickJones! Congrats on your first commit 🎉 |
Thanks @jedcunningham @potiuk. I learned a lot from this experience. |
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