Skip to content
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

make conf.set case insensitive #33452

Merged
merged 6 commits into from
Aug 21, 2023
Merged

make conf.set case insensitive #33452

merged 6 commits into from
Aug 21, 2023

Conversation

vandonr-amz
Copy link
Contributor

conf.get is insensitive (it converts section and key to lower case), but set is not, which can lead to surprising behavior (see the test, which is not passing without the fix).
I suggest that we override set as well to fix that. Any value that was set before with upper case was unreacheable.

`conf.get` is insensitive (it converts section and key to lower case)
but set is not, which can lead to surprising behavior
(see the test, which is not passing without the fix).
I suggest that we override set as well to fix that.
Any value that was set before with upper case was unreacheable.
airflow/configuration.py Outdated Show resolved Hide resolved
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change. I remember I encountered such a gotcha recently. But have the same questions/suggestions as TP

airflow/configuration.py Outdated Show resolved Hide resolved
airflow/configuration.py Outdated Show resolved Hide resolved
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after resolving the discussions with the other reviewers.

airflow/configuration.py Show resolved Hide resolved
@hussein-awala hussein-awala added this to the Airflow 2.7.1 milestone Aug 17, 2023
@hussein-awala hussein-awala added the type:bug-fix Changelog: Bug Fixes label Aug 17, 2023
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as @hussein-awala. OK for me with a few small comments

@Taragolis Taragolis merged commit abbd567 into apache:main Aug 21, 2023
@vandonr-amz vandonr-amz deleted the vandonr/fix branch August 21, 2023 21:24
ephraimbuddy pushed a commit that referenced this pull request Aug 28, 2023
* make `conf.set` case insensitive

`conf.get` is insensitive (it converts section and key to lower case)
but set is not, which can lead to surprising behavior
(see the test, which is not passing without the fix).
I suggest that we override set as well to fix that.
Any value that was set before with upper case was unreacheable.

* fix remove_option as well

* away with the str()

* add significant change newsfragment

(cherry picked from commit abbd567)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants