Remove the default config from the Configuration View, show only running configuration #14641
Conversation
airflow/www/views.py
Outdated
| return self.render_template( | ||
| 'airflow/config.html', | ||
| title=title, | ||
| pre_subtitle=settings.HEADER + " v" + airflow.__version__, |
There was a problem hiding this comment.
I'm wondering if we need to render the header? It look good in cli but showing it in web UI does not seem a good experience. WDYT?
There was a problem hiding this comment.
IMO it doesn't add anything relevant, so I'd remove it. Shall I add that to this PR?
There was a problem hiding this comment.
Removed pre_subtitle altogether, the version is also displayed in the footer.
airflow/www/views.py
Outdated
| "configuration, most likely for security reasons." | ||
|
|
||
| if request.args.get('raw') == "true": | ||
| return Response(response=json.dumps(table), status=200, mimetype="application/json") |
There was a problem hiding this comment.
Is there a reason why to use json? Returning plain text allowed users do the copy past of configuration. I see not use case for json.
There was a problem hiding this comment.
If we're going to return json we should just content negotiation (i.e. Accept headers)
There was a problem hiding this comment.
To be consistent, I figured the raw=true argument should also return the rendered configuration. The airflow.cfg content was loaded as lines of text, but the rendered configuration is loaded as a list of tuples. So, I had to decide on an output format and figured JSON would make sense.
Thinking about it, the new REST API does exactly what we want (content negotiation, (https://github.com/apache/airflow/blob/master/airflow/api_connexion/endpoints/config_endpoint.py#L66-L87). We could also simply remove the raw=true argument and tell people to use the REST API, instead of maintaining this? I've never seen anybody using it?
There was a problem hiding this comment.
Went digging for possible usage, the raw=true code was first added 5 years ago: d12dc18. Seems like "first iteration" was permanent after all😄
Shall I remove the raw=true argument and direct people to the REST API in UPDATING.md?
There was a problem hiding this comment.
Went ahead and removed raw=true
There was a problem hiding this comment.
If I correctly understand removing the raw option marks this PR automatically as Airflow 3.0, right? Should we do some type of depreciation warning first and remove it with 3.0?
I know this is probably rarely used feature but we decided to use semver.
|
@ashb @turbaszek I went ahead and removed the Also, I removed the subtitle in the Configuration view:
WDYT? |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
|
@BasPH any plans to bring this in? |
github-actions
bot
removed
the
stale
|
I had also been bothered by having to scroll down the page and had created an alternative solution to this (that I have yet to PR). This would essentially keep the same content and present it in a tabbed interface:
Is there value in retaining access to the defaults as well? |
|
@ryanahamilton Sometimes, yes. Perhaps a check box for "include default config", (Or similar) so by default it only shows the changed settings, but that it is possible to show all running config. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
|
@ryanahamilton I would like to revive this PR since it continues to lead to confusion. I quite like your solution, any plans to make a PR for it? |
This PR removes airflow.cfg from the Airflow Configuration view, and shows only the rendered view, with the goal to remove confusion about which configuration is currently active.
My motivation is that I often encounter people not knowing that you can scroll down to see the active configuration, thinking the airflow.cfg (which is displayed first) is the configuration currently active (which are defaults that might be overridden by env vars).
Summary of changes:
AIRFLOW__WEBSERVER__EXPOSE_CONFIG=Falseraw=true, this can be substituted by the Config endpoint of the REST APIpre_subtitle(the ascii art thing + version)Screenshots of old & new behaviours:
AIRFLOW__WEBSERVER__EXPOSE_CONFIG=FalseAIRFLOW__WEBSERVER__EXPOSE_CONFIG=True^ 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.