-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Move the database configuration to a new section #22284
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)
|
This one will need much more. It's not enough to move the sections. And (unless I missed it) the change should handle deprecation - this is a very intrusive change and whoever has the current configurationm (either via config or environment variable) it should continue to work but raise a deprecation warning. |
Thanks @potiuk . Will make the necessary change. |
### Database configuration moved to new section | ||
|
||
The following configurations have been moved from `[core]` to the new `[database]` section. | ||
|
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.
Information that they can still be accessed via "core" section but will raise a deprecation warning would be useful to add here.
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.
This is NIT. And it is good without it - feel free ot add it if you want though.
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.
Cool. One nit
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
Need rebase unfortunately. |
chart/templates/_helpers.yaml
Outdated
- name: AIRFLOW__DATABASE__SQL_ALCHEMY_CONN | ||
valueFrom: |
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.
- name: AIRFLOW__DATABASE__SQL_ALCHEMY_CONN | |
valueFrom: | |
- name: AIRFLOW__DATABASE__SQL_ALCHEMY_CONN | |
valueFrom: |
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.
This should help.
Awesome work, congrats on your first merged pull request! |
@gitstart-airflow it looks like this pr has caused this bug #23679 |
@potiuk i am not quite sure about the expected behavior here for deprecating section. airflow/airflow/configuration.py Lines 518 to 528 in a7bccaa
for the deprecation_section and key, for the above ^^ method, for the current behavior for my case #23679 (comment) is |
Yes. I looked at it already and I could confirm that I am not the author of this part, but after looking there I think simply conf.as_dict has not been written with such deprecations in mind (and likely some users could have some problems before because of other deprecations). I think - "logically" speaking I think when the "new" value has a corresponding non-default "deprecated" value in the conf - it just should not be exported by the "as_dict()" (when it is taken from default) - but otherwise it should. i am looking at fixing it in an upcoming pr - but I have to learn how the system works first. |
If you want @pingzh you can try to learn it too, and come up with a good solution - maybe we can eventually compare ours and choose the better one. |
closes: 15930
This PR moves the following configurations from [core] to the new [database] section.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.