Making webserver config customisable by amoghrajesh · Pull Request #29926 · apache/airflow

amoghrajesh · 2023-03-05T06:29:54Z

The webserver_config.py is hard coded and is not customisable at the moment. It would be nice to read this config from a property in the configs instead of hardcoding it. Added support for reading the webserver_config.py through [webserver] config_file
closes: #10206

^ 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.

uranusjr · 2023-03-05T07:01:13Z

This looks like a work in progress? A lot of leftover and incorrectly formatted code.

amoghrajesh · 2023-03-05T07:05:45Z

@uranusjr the unit test is a WIP. The application code is up to date. I do not see formatting issues in that file

potiuk · 2023-03-05T10:37:09Z


+        if in_airflow_home:
+            log.warning(
+                "webserver_config.py is still present in %s, this will be deprecated and moved to %s",


Suggested change

"webserver_config.py is still present in %s, this will be deprecated and moved to %s",

"webserver_config.py is still present in %s, this locaation is deprecated. You should move your webserver_config.py to %s",

Good idea. Let me make this change

potiuk

I think generating the webserver_config.py to the new location by default should be also part of this PR. Otherwise - if we forget about it, we will end up when you install airflow for the first time - with deprecation warning out-of-the-box.

(and we need to update documentation too).

amoghrajesh · 2023-03-05T11:02:05Z

@potiuk I realised that the last version of this PR was very confusing. Re worked it to address your concerns too.

potiuk · 2023-03-05T15:12:27Z

Tests need fixing too.

amoghrajesh · 2023-03-05T15:41:31Z

@potiuk fixed the code. There are lot of occurrences in the docs. Should we handle it in a separate Github issue and PR?

potiuk · 2023-03-05T20:24:40Z

@potiuk fixed the code. There are lot of occurrences in the docs. Should we handle it in a separate Github issue and PR?

No. We should update docs in the same PR that changes the code. This should be done together, then it is better cherry-pickable .

…w into webServerConfigurable

amoghrajesh · 2023-03-12T04:55:13Z

@potiuk @uranusjr can you take another look at the PR?

@uranusjr I commented on your review comments - maybe I didn't understand them correctly

@potiuk handled the review comments - added changes in the docs too. Did I include all the files?

amoghrajesh

@jedcunningham @potiuk reworked the PR as per our slack discussion.Honestly, this PR looks much cleaner than what we had previously. Thanks!

potiuk · 2023-03-21T16:46:13Z

LGTM - @jedcunningham ?

…w into webServerConfigurable

amoghrajesh · 2023-03-24T18:57:55Z

@jedcunningham can you take a look when possible?

jedcunningham · 2023-03-27T21:57:18Z

+    conf = initialize_config()
+    conf.validate()
+
+    assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")


Why does this pass? Shouldn't it be your new name instead?

I might be wrong but as per my understanding and running the debugger: Initially, the default path is taken. Once we reach lines: https://github.com/apache/airflow/pull/29926/files#diff-2ab05df659c739ff335a407c4a6cddc5f177e9c402aca296991f53fa341f4dfeR1537-R1542, it generates the file to the new path instead.

It's good that the assert down below passes (meaning Airflow does create it in the right place). However, we should also test that WEBSERVER_CONFIG gets set to the file in the config. You might want to import it here after running initialize?

This also has the potential of side effects. You should switch back to using mock.patch.dict, and you should also try and clean up the file as well with a try/finally.

You might want to import it here after running initialize

Not sure how I can do that. Can i get some help here?

I think the issue is that the local_conf.get("webserver", "config_file") is unable to return the right value of the webserver config file here. This is because the mock.patch.dict doesn't seem to add these values correctly for the unit test. Could i get some help here @jedcunningham ?

Adding a log statement to print the env variable set and it seems to reflect it there.

PASSED [100%][2023-05-10T14:10:06.620+0530] {test_views.py:68} WARNING - /tmp/my_custom_webserver_config.py

Hmm, maybe try with conf_vars like this instead:

airflow/tests/operators/test_email.py

Line 59 in d6051fd

with conf_vars({("email", "email_backend"): "tests.operators.test_email.send_email_test"}):

Not helpful. Wondering if we can get the main feature in here and then I can create a follow up PR to investigate this in more detail and fix it. @jedcunningham thoughts?

I pushed a commit into your branch that fixes the test. Took me a bit of time to get it actually working, but that exercises the config option and doesn't have any side effects (like leaving WEBSERVER_CONFIG with the custom config from this test).

amoghrajesh · 2023-04-24T11:06:23Z

I will try to look into this based on your suggestion @jedcunningham
Thanks for the feedback

…w into webServerConfigurable

potiuk · 2023-05-17T18:00:20Z

-    global WEBSERVER_CONFIG
-    WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
-
+    WEBSERVER_CONFIG = local_conf.get("webserver", "config_file")


Needs formatting, but I think we need:

Suggested change

WEBSERVER_CONFIG = local_conf.get("webserver", "config_file")

WEBSERVER_CONFIG = local_conf.get("webserver", "config_file",

fallback= AIRFLOW_HOME + "/webserver_config.py")

Otherwise it will fail for those who have the old config file

(or is it aautomaticaly handled with the default?

It's automatically handled. Most fallbacks can go honestly, much more useful in providers than core.

Yeah I vaguely recall that - I wanted to clean up the code doing it once and failed miserably :)

Thanks for clarifying this. Was not so aware about this

Making webserver config customizable

a6c5ee9

boring-cyborg Bot added the area:webserver Webserver related Issues label Mar 5, 2023

amoghrajesh mentioned this pull request Mar 5, 2023

Make webserver_config.py location customizable #10206

Closed

Amogh added 3 commits March 5, 2023 12:36

Merge branch 'main' into webServerConfigurable

77802f7

Adding unit tests

26fc78e

Fixing staticcheck issues

b85cd0c

potiuk reviewed Mar 5, 2023

View reviewed changes

potiuk previously requested changes Mar 5, 2023

View reviewed changes

Adding deprecation warning

4ce74b9

amoghrajesh requested a review from potiuk March 5, 2023 11:01

Fixing failing tests

5b73b18

Amogh added 5 commits March 5, 2023 21:24

Making webserver config customizable

2152dea

Adding unit tests

5edfd1d

Fixing staticcheck issues

68e422b

Adding deprecation warning

ba81ae6

Fixing failing tests

0f6b784

potiuk force-pushed the webServerConfigurable branch from 5b73b18 to 0f6b784 Compare March 5, 2023 20:24

uranusjr reviewed Mar 6, 2023

View reviewed changes

Comment thread airflow/configuration.py Outdated

uranusjr reviewed Mar 6, 2023

View reviewed changes

Comment thread airflow/configuration.py Outdated

Amogh added 2 commits March 12, 2023 10:13

Merge branch 'webServerConfigurable' of github.com:amoghrajesh/airflo…

cddd581

…w into webServerConfigurable

Adding documentation changes

acd291e

Correcting bug in the code

13f1e9c

amoghrajesh removed the request for review from potiuk March 13, 2023 15:32

amoghrajesh commented Mar 20, 2023

View reviewed changes

Comment thread tests/www/views/test_views.py Outdated

amoghrajesh requested review from jedcunningham and removed request for potiuk March 20, 2023 07:50

Merge branch 'main' into webServerConfigurable

02685c8

potiuk approved these changes Mar 21, 2023

View reviewed changes

jedcunningham reviewed Mar 22, 2023

View reviewed changes

Comment thread airflow/config_templates/config.yml Outdated

Comment thread airflow/configuration.py Outdated

Comment thread tests/www/views/test_views.py Outdated

Amogh added 2 commits March 23, 2023 19:46

Handling review comments from Jed

98ace93

Merge branch 'webServerConfigurable' of github.com:amoghrajesh/airflo…

2b271f6

…w into webServerConfigurable

amoghrajesh requested a review from jedcunningham March 23, 2023 14:18

jedcunningham reviewed Mar 26, 2023

View reviewed changes

Comment thread tests/www/views/test_views.py Outdated

Comment thread tests/www/views/test_views.py Outdated

Amogh added 2 commits March 27, 2023 16:29

Handling review comments from Jed

3be8ca0

removing unwanted import

86048bd

jedcunningham reviewed Mar 27, 2023

View reviewed changes

Amogh and others added 3 commits May 2, 2023 09:27

Addressing review comments from Jed

4305a48

Merge branch 'main' into webServerConfigurable

4b0ca76

Fix test

41052e3

jedcunningham reviewed May 17, 2023

View reviewed changes

Comment thread airflow/config_templates/config.yml Outdated

Comment thread airflow/configuration.py Outdated

Amogh added 2 commits May 17, 2023 13:07

nits from Jed

5184664

Merge branch 'webServerConfigurable' of github.com:amoghrajesh/airflo…

2993bff

…w into webServerConfigurable

amoghrajesh requested a review from jedcunningham May 17, 2023 07:38

jedcunningham approved these changes May 17, 2023

View reviewed changes

jedcunningham requested a review from potiuk May 17, 2023 17:31

potiuk reviewed May 17, 2023

View reviewed changes

potiuk approved these changes May 17, 2023

View reviewed changes

potiuk merged commit 30e93ff into apache:main May 18, 2023

jedcunningham added this to the Airflow 2.7.0 milestone May 30, 2023

jedcunningham added the type:new-feature Changelog: New Features label May 30, 2023

	"webserver_config.py is still present in %s, this will be deprecated and moved to %s",
	"webserver_config.py is still present in %s, this locaation is deprecated. You should move your webserver_config.py to %s",

	WEBSERVER_CONFIG = local_conf.get("webserver", "config_file")
	WEBSERVER_CONFIG = local_conf.get("webserver", "config_file",
	fallback= AIRFLOW_HOME + "/webserver_config.py")

Uh oh!

Conversation

amoghrajesh commented Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uranusjr commented Mar 5, 2023

Uh oh!

amoghrajesh commented Mar 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amoghrajesh commented Mar 5, 2023

Uh oh!

potiuk commented Mar 5, 2023

Uh oh!

amoghrajesh commented Mar 5, 2023

Uh oh!

potiuk commented Mar 5, 2023

Uh oh!

Uh oh!

Uh oh!

amoghrajesh commented Mar 12, 2023

Uh oh!

amoghrajesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

potiuk commented Mar 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amoghrajesh commented Mar 24, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jedcunningham May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amoghrajesh commented Apr 24, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

amoghrajesh commented Mar 5, 2023 •

edited

Loading

potiuk left a comment •

edited

Loading

jedcunningham May 17, 2023 •

edited

Loading