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

Display only the running configuration in configurations view #28892

Merged
merged 5 commits into from
Jan 13, 2023

Conversation

BasPH
Copy link
Contributor

@BasPH BasPH commented Jan 12, 2023

This PR displays only the running configuration in the configurations view. The (very confusing) default configuration is not displayed anymore. I regularly encounter confusion among Airflow users on this page about what is actually the running value of a config option, because it's not obvious you first have to scroll past the default configuration.

This follows up on #14641, which went stale.

There's an undocumented /configuration?raw=true endpoint to download the config. Users should use the /config endpoint in the REST API instead. I can't imagine anybody using this raw=true endpoint, but added a Deprecation header to the response to indicate the upcoming removal. If there's a better way to indicate deprecation, please let me know.

After:
AIRFLOW__WEBSERVER__EXPOSE_CONFIG=True:
image

AIRFLOW__WEBSERVER__EXPOSE_CONFIG=False:
image

Before:
AIRFLOW__WEBSERVER__EXPOSE_CONFIG=True:
image

AIRFLOW__WEBSERVER__EXPOSE_CONFIG=False:
image


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jan 12, 2023
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Looks good

default configuration is not displayed anymore.

I think we should still display defaults because if user is not aware of them he needs to look them up.

Maybe a toggle could solve this as it will allow both states in the same view?

@BasPH
Copy link
Contributor Author

BasPH commented Jan 13, 2023

I think we should still display defaults because if user is not aware of them he needs to look them up.

Why?

I encounter confusion about this page very regularly, people expect this page to show the running configuration. They do not expect the default configuration and are also not interested in knowing the default value when viewing this page, so I don't see the point in showing that.

@eladkal
Copy link
Contributor

eladkal commented Jan 13, 2023

I encounter confusion about this page very regularly, people expect this page to show the running configuration. They do not expect the default configuration and are also not interested in knowing the default value when viewing this page, so I don't see the point in showing that.

I agree that now its confusing.
What I meant is to show all configurations that applied to the specific Airflow instance. If user override then show the override value if user didn't then show the default value

@BasPH
Copy link
Contributor Author

BasPH commented Jan 13, 2023

The table is showing exactly that? It shows the active values, plus from where they're configured:

image

@eladkal
Copy link
Contributor

eladkal commented Jan 13, 2023

The table is showing exactly that? It shows the active values, plus from where they're configured

Wasnt sure about that.
LGTM then :)
Very useful improvement!

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.

Looks cool. very useful to not show defaults and super helpful to see the source of the values.

I think it would be useful but more difficult to show the same values for other components (especially if they differ from the one that webserve sees). This is also an often source of confusion for users especially those who set up a self-managed versions where the values might differ. But this is much more difficult to avhieve via GUI, so this is another story (and discussed/implemented elsewhere).

@BasPH
Copy link
Contributor Author

BasPH commented Jan 13, 2023

Agreed! Also saw failures happening at projects where the configuration wasn't applied consistently to all components, which was difficult to debug because the webserver showed the correct value. It would be cool to build some sort of "configuration-drift-detection", but I'll leave that for another time ;-)

@BasPH BasPH merged commit 185faab into apache:main Jan 13, 2023
@BasPH BasPH deleted the remove-default-config-take2 branch January 13, 2023 21:07
@eladkal eladkal added this to the Airflow 2.5.2 milestone Jan 13, 2023
@pierrejeambrun pierrejeambrun added the type:misc/internal Changelog: Misc changes that should appear in change log label Feb 27, 2023
@pierrejeambrun pierrejeambrun added type:improvement Changelog: Improvements and removed type:misc/internal Changelog: Misc changes that should appear in change log labels Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants