You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2023/03/05 06:29:56 UTC
[GitHub] [airflow] amoghrajesh opened a new pull request, #29926: Making webserver config customisable
amoghrajesh opened a new pull request, #29926:
URL: https://github.com/apache/airflow/pull/29926
<!--
Thank you for contribuhard codedase make sure that your code changes
are covered with tests. And in case of new features or big changes
remember the to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
The webserver_config.py is hard coded and is not customisable at the moment. It would be nice to read this config from the config directory instead of the root too. This PR adds support to prioritise the webserver_config.py present in the config directory over the root one and uses that as default location too.
closes: #10206
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1141611145
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,13 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER_CONFIG_FILE": "{AIRFLOW_HOME}/config/webserver_config.py"})
Review Comment:
@jedcunningham I was trying to get a thorough UT here where I override the default location of the webserver config file. However, the mock.patch here doesn't seem to get honored. Need some help here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1134218260
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
Review Comment:
Do not use `+` to join path components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1146765657
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,21 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/webserver_config.py"})
Review Comment:
How about we use something other than `webserver_config.py`, so its more obvious we are testing the arbitrary config path?
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,21 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(get_airflow_home(), "webserver_config.py")
+
+ config_path = os.path.join("/tmp", "webserver_config.py")
+
+ assert os.path.isfile(config_path)
Review Comment:
```suggestion
config_path = "/tmp/webserver_config.py"
assert WEBSERVER_CONFIG == config_path)
assert os.path.isfile(config_path)
```
Let's not use `get_airflow_home`, but otherwise feel free to take or leave these suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1133186274
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
Review Comment:
@uranusjr where should i use either of those modules? I am defining some constants here and then checking if those paths exist using `os.path.isfile`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1182060641
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
+
+ # new webserver config is created in this path
+ config_path = os.path.join("/tmp", "my_custom_webserver_config.py")
+
+ assert os.path.isfile(config_path)
+
+ if os.path.isfile(config_path):
+ os.remove(config_path)
Review Comment:
Thank you for that suggestion. Integrated this in the latest commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1137694479
##########
airflow/configuration.py:
##########
Review Comment:
Ah, TIL about AIRFLOW_HOME/config being added to sys.path automatically 🤦‍♂️ Makes a little more sense now 🎉
Still, our usage of webserver_config isn't tied to being in sys.path at all. I don't see a ton of value moving it, but if we want to, it should still be configurable (e.g. with an `AIRFLOW_WEBSERVER_CONFIG` or a proper conf entry). This solves the real problem, which is "I want it where I want it" - not everyone will find the config dir any easier or straight forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1455126258
@potiuk fixed the code. There are lot of occurrences in the docs. Should we handle it in a separate Github issue and PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1468232745
@uranusjr thanks for informing me that. Fixed the static checks. Can you help in merging the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1465092945
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1146271851
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,13 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER_CONFIG_FILE": "{AIRFLOW_HOME}/config/webserver_config.py"})
Review Comment:
Thanks! Was able to fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1188124562
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
> You might want to import it here after running initialize
Not sure how I can do that. Can i get some help here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1187954609
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #29926: Making webserver config customisable
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1196884815
##########
airflow/configuration.py:
##########
@@ -1548,10 +1548,7 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
- global WEBSERVER_CONFIG
- WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
-
+ WEBSERVER_CONFIG = local_conf.get("webserver", "config_file")
Review Comment:
(or is it aautomaticaly handled with the default?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1135076674
##########
RELEASE_NOTES.rst:
##########
@@ -8919,7 +8919,7 @@ New Webserver UI with Role-Based Access Control
The current webserver UI uses the Flask-Admin extension. The new webserver UI uses the `Flask-AppBuilder (FAB) <https://github.com/dpgaspar/Flask-AppBuilder>`_ extension. FAB has built-in authentication support and Role-Based Access Control (RBAC), which provides configurable roles and permissions for individual users.
-To turn on this feature, in your airflow.cfg file (under [webserver]), set the configuration variable ``rbac = True``\ , and then run ``airflow`` command, which will generate the ``webserver_config.py`` file in your $AIRFLOW_HOME.
+To turn on this feature, in your airflow.cfg file (under [webserver]), set the configuration variable ``rbac = True``\ , and then run ``airflow`` command, which will generate the ``webserver_config.py`` file in your $AIRFLOW_HOME/config.
Review Comment:
I don’t think this should be changed. This line describes things _when 1.10.0 was released_ and history should not be altered. Instead you should _add_ a `significant` entry in `newsfragments` explaining the relocation of the config file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1149828626
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
Why does this pass? Shouldn't it be your new name instead?
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
+
+ # new webserver config is created in this path
+ config_path = os.path.join("/tmp", "my_custom_webserver_config.py")
+
+ assert os.path.isfile(config_path)
+
+ if os.path.isfile(config_path):
+ os.remove(config_path)
Review Comment:
Maybe use tmp_path instead of having to handle cleanup explicitly? You'll have to move your mocking around.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1137660553
##########
airflow/configuration.py:
##########
Review Comment:
This feels a little half-baked.
- This doesn't actually make the path configurable, just moves it under a new config dir.
- If we want a config dir, why isn't `airflow.cfg` also moving into it?
I not sure I follow what a config dir buys us?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1137698124
##########
airflow/configuration.py:
##########
Review Comment:
So to summarize, I'd rather we make it actually configurable than to just move it to a dir that happens to be an easier place for the original issue author.
My 2c. @potiuk @uranusjr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1466424195
Coding issue aside, the main problem I have with this is it strongly couples Airflow’s webserver to Gunicorn. It is not prohibited, but I would strongly prefer if we add a concrete policy around this, so we won’t be tied to backward compatibility if a switch to another web server implementation becomes a discussion one day.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1133186384
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
+
global WEBSERVER_CONFIG
- WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
+ WEBSERVER_CONFIG = webserver_config_in_config
+
+ msg = (
+ "webserver_config.py is still present in {}, this location is deprecated. You should move your "
+ "webserver_config.py to {} "
+ )
+
+ msg = msg.format(old_webserver_config, webserver_config_in_config)
+
+ if os.path.isfile(old_webserver_config):
+ warnings.warn(msg, category=DeprecationWarning)
if not os.path.isfile(WEBSERVER_CONFIG):
import shutil
log.info("Creating new FAB webserver config file in: %s", WEBSERVER_CONFIG)
+ os.makedirs(os.path.dirname(AIRFLOW_HOME + "/config"), exist_ok=True)
Review Comment:
Thanks for the feedback. I dont think so. It is looking for a directory within `AIRFLOW_HOME` and not just for `AIRFLOW_HOME`. Maybe is would be equal to `os.makedirs(AIRFLOW_HOME + "/config", exist_ok=True)`
Did I understand your question right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1133503861
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
+
global WEBSERVER_CONFIG
- WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
+ WEBSERVER_CONFIG = webserver_config_in_config
+
+ msg = (
+ "webserver_config.py is still present in {}, this location is deprecated. You should move your "
+ "webserver_config.py to {} "
+ )
+
+ msg = msg.format(old_webserver_config, webserver_config_in_config)
+
+ if os.path.isfile(old_webserver_config):
+ warnings.warn(msg, category=DeprecationWarning)
if not os.path.isfile(WEBSERVER_CONFIG):
import shutil
log.info("Creating new FAB webserver config file in: %s", WEBSERVER_CONFIG)
+ os.makedirs(os.path.dirname(AIRFLOW_HOME + "/config"), exist_ok=True)
Review Comment:
`os.path.dirname(AIRFLOW_HOME + "/config")` returns _the directory `config` is in_, which is simply `AIRFLOW_HOME`. Maybe `dirname` is not what you think and should not be used here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1146256246
##########
airflow/configuration.py:
##########
@@ -1531,9 +1531,9 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ # The conf.get should provide the default_config in case the value is not set in webserver configs
Review Comment:
Sure, made the changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1149148020
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,21 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/webserver_config.py"})
Review Comment:
Good idea. Made the change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1196070284
##########
airflow/configuration.py:
##########
@@ -1548,9 +1548,8 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
global WEBSERVER_CONFIG
Review Comment:
Made the changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1519926144
I will try to look into this based on your suggestion @jedcunningham
Thanks for the feedback
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #29926: Making webserver config customisable
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29926:
URL: https://github.com/apache/airflow/pull/29926
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1193082607
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1455009300
@uranusjr the unit test is a WIP. The application code is up to date. I do not see formatting issues in that file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1483274912
@jedcunningham can you take a look when possible?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1125959885
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
Review Comment:
Please use os.path or pathlib instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1125960915
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
+
global WEBSERVER_CONFIG
- WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
+ WEBSERVER_CONFIG = webserver_config_in_config
+
+ msg = (
+ "webserver_config.py is still present in {}, this location is deprecated. You should move your "
+ "webserver_config.py to {} "
+ )
+
+ msg = msg.format(old_webserver_config, webserver_config_in_config)
+
+ if os.path.isfile(old_webserver_config):
+ warnings.warn(msg, category=DeprecationWarning)
if not os.path.isfile(WEBSERVER_CONFIG):
import shutil
log.info("Creating new FAB webserver config file in: %s", WEBSERVER_CONFIG)
+ os.makedirs(os.path.dirname(AIRFLOW_HOME + "/config"), exist_ok=True)
Review Comment:
Isn’t this just `os.makedirs(AIRFLOW_HOME, exist_ok=True)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1137705279
##########
airflow/configuration.py:
##########
Review Comment:
I'm not in favor of moving it. I'd rather see it actually be a proper config entry `e.g. [webserver] config_file` or something so this option is more easily discoverable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1455008348
This looks like a work in progress? A lot of leftover and incorrectly formatted code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #29926: Making webserver config customisable
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1125639369
##########
airflow/configuration.py:
##########
@@ -1535,9 +1535,24 @@ def initialize_config() -> AirflowConfigParser:
global WEBSERVER_CONFIG
WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
+ in_airflow_home = True
+
if not os.path.isfile(WEBSERVER_CONFIG):
+ if os.path.isfile(webserver_config_in_config):
+ in_airflow_home = False
+ WEBSERVER_CONFIG = webserver_config_in_config
+
import shutil
+ if in_airflow_home:
+ log.warning(
+ "webserver_config.py is still present in %s, this will be deprecated and moved to %s",
Review Comment:
```suggestion
"webserver_config.py is still present in %s, this locaation is deprecated. You should move your webserver_config.py to %s",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29926: Making webserver config customisable
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1478224380
LGTM - @jedcunningham ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1189550840
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
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 ?
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1192924113
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
Hmm, maybe try with `conf_vars` like this instead:
https://github.com/apache/airflow/blob/d6051fd10a0949264098af23ce74c76129cfbcf4/tests/operators/test_email.py#L59
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1466435849
@uranusjr any suggestions? I want to understand what you mean by a policy here so i can help in implementing it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1134218260
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
Review Comment:
Do no use `+` for path-joining.
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
Review Comment:
Do not use `+` for path-joining.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1468505216
A second maintainer is needed to approve the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1197415059
##########
airflow/configuration.py:
##########
@@ -1548,10 +1548,7 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
- global WEBSERVER_CONFIG
- WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
-
+ WEBSERVER_CONFIG = local_conf.get("webserver", "config_file")
Review Comment:
Thanks for clarifying this. Was not so aware about this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1135076674
##########
RELEASE_NOTES.rst:
##########
@@ -8919,7 +8919,7 @@ New Webserver UI with Role-Based Access Control
The current webserver UI uses the Flask-Admin extension. The new webserver UI uses the `Flask-AppBuilder (FAB) <https://github.com/dpgaspar/Flask-AppBuilder>`_ extension. FAB has built-in authentication support and Role-Based Access Control (RBAC), which provides configurable roles and permissions for individual users.
-To turn on this feature, in your airflow.cfg file (under [webserver]), set the configuration variable ``rbac = True``\ , and then run ``airflow`` command, which will generate the ``webserver_config.py`` file in your $AIRFLOW_HOME.
+To turn on this feature, in your airflow.cfg file (under [webserver]), set the configuration variable ``rbac = True``\ , and then run ``airflow`` command, which will generate the ``webserver_config.py`` file in your $AIRFLOW_HOME/config.
Review Comment:
I don’t think this should be changed. This line describes things _when 1.10.0 was released_ and history should not be altered. Also I believe this feature no longer exists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1135166008
##########
RELEASE_NOTES.rst:
##########
@@ -8919,7 +8919,7 @@ New Webserver UI with Role-Based Access Control
The current webserver UI uses the Flask-Admin extension. The new webserver UI uses the `Flask-AppBuilder (FAB) <https://github.com/dpgaspar/Flask-AppBuilder>`_ extension. FAB has built-in authentication support and Role-Based Access Control (RBAC), which provides configurable roles and permissions for individual users.
-To turn on this feature, in your airflow.cfg file (under [webserver]), set the configuration variable ``rbac = True``\ , and then run ``airflow`` command, which will generate the ``webserver_config.py`` file in your $AIRFLOW_HOME.
+To turn on this feature, in your airflow.cfg file (under [webserver]), set the configuration variable ``rbac = True``\ , and then run ``airflow`` command, which will generate the ``webserver_config.py`` file in your $AIRFLOW_HOME/config.
Review Comment:
Good idea @uranusjr.
Moved the content to a newsfragment file. Can you look at it again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1196007187
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
I [pushed a commit](https://github.com/apache/airflow/pull/29926/commits/41052e3a0ee3bed2350b891f6b594c24deed4c91) 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1196007187
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #29926: Making webserver config customisable
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1196883874
##########
airflow/configuration.py:
##########
@@ -1548,10 +1548,7 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
- global WEBSERVER_CONFIG
- WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
-
+ WEBSERVER_CONFIG = local_conf.get("webserver", "config_file")
Review Comment:
Needs formatting, but I think we need:
```suggestion
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #29926: Making webserver config customisable
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1196900616
##########
airflow/configuration.py:
##########
@@ -1548,10 +1548,7 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
- global WEBSERVER_CONFIG
- WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
-
+ WEBSERVER_CONFIG = local_conf.get("webserver", "config_file")
Review Comment:
Yeah I vaguely recall that - I wanted to clean up the code doing it once and failed miserably :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #29926: Making webserver config customisable
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1137742117
##########
airflow/configuration.py:
##########
Review Comment:
I am ok with whatever you guys agree to :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1137688177
##########
airflow/configuration.py:
##########
Review Comment:
Thanks for the review.
I am trying to make the webserver config file rather sit in the config directory than the root directory as suggested by @potiuk.
Didn't consider moving airflow.cfg there yet. Would it make sense to do that? I am ok with that idea since it is again a configuration file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1467706016
Need to fix line wrapping
https://github.com/apache/airflow/actions/runs/4413652731/jobs/7734666112#step:6:162
Test failures seem to be unrelated; not sure, I’ll investigate separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1182058034
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,22 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/my_custom_webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(AIRFLOW_HOME, "webserver_config.py")
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29926: Making webserver config customisable
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1455195474
> @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 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1125640818
##########
airflow/configuration.py:
##########
@@ -1535,9 +1535,24 @@ def initialize_config() -> AirflowConfigParser:
global WEBSERVER_CONFIG
WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
+ in_airflow_home = True
+
if not os.path.isfile(WEBSERVER_CONFIG):
+ if os.path.isfile(webserver_config_in_config):
+ in_airflow_home = False
+ WEBSERVER_CONFIG = webserver_config_in_config
+
import shutil
+ if in_airflow_home:
+ log.warning(
+ "webserver_config.py is still present in %s, this will be deprecated and moved to %s",
Review Comment:
Good idea. Let me make this change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1145483497
##########
airflow/configuration.py:
##########
@@ -1531,9 +1531,9 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ # The conf.get should provide the default_config in case the value is not set in webserver configs
Review Comment:
```suggestion
```
Don't think we need this comment, that's always the behavior of `get`.
##########
airflow/config_templates/config.yml:
##########
@@ -1264,6 +1264,14 @@ hive:
webserver:
description: ~
options:
+ config_file:
+ description: |
+ Path of webserver_config.py file used for configuring the webserver parameters with
+ respect to the AIRFLOW_HOME
Review Comment:
```suggestion
Path of webserver_config.py file used for configuring the webserver parameters
```
It can really be anywhere, so you can drop that part of it.
##########
airflow/config_templates/config.yml:
##########
@@ -1264,6 +1264,14 @@ hive:
webserver:
description: ~
options:
+ config_file:
+ description: |
+ Path of webserver_config.py file used for configuring the webserver parameters with
+ respect to the AIRFLOW_HOME
Review Comment:
We might even consider not having `webserver_config.py` in the description either, since it really could be named anything now!
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,13 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER_CONFIG_FILE": "{AIRFLOW_HOME}/config/webserver_config.py"})
Review Comment:
You're missing an `_`, try with `AIRFLOW__WEBSERVER__CONFIG_FILE`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1149161077
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,21 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER__CONFIG_FILE": "/tmp/webserver_config.py"})
+def test_webserver_configuration_config_file(admin_client):
+ conf = initialize_config()
+ conf.validate()
+
+ assert WEBSERVER_CONFIG == os.path.join(get_airflow_home(), "webserver_config.py")
+
+ config_path = os.path.join("/tmp", "webserver_config.py")
+
+ assert os.path.isfile(config_path)
Review Comment:
Sure. Used the AIRFLOW_HOME instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1134234804
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
Review Comment:
Ok sure. Used `os.path.join` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1455057758
@potiuk I realised that the last version of this PR was very confusing. Re worked it to address your concerns too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29926: Making webserver config customisable
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29926:
URL: https://github.com/apache/airflow/pull/29926#issuecomment-1455118835
Tests need fixing too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #29926: Making webserver config customisable
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1125959885
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
Review Comment:
Please use os.path or pathlib instead. Elsewhere as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1134171101
##########
airflow/configuration.py:
##########
@@ -1531,14 +1531,28 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
+ old_webserver_config = AIRFLOW_HOME + "/webserver_config.py"
+ # Prioritise airflow webserver config that is present in the config location
+ webserver_config_in_config = AIRFLOW_HOME + "/config/webserver_config.py"
+
global WEBSERVER_CONFIG
- WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
+ WEBSERVER_CONFIG = webserver_config_in_config
+
+ msg = (
+ "webserver_config.py is still present in {}, this location is deprecated. You should move your "
+ "webserver_config.py to {} "
+ )
+
+ msg = msg.format(old_webserver_config, webserver_config_in_config)
+
+ if os.path.isfile(old_webserver_config):
+ warnings.warn(msg, category=DeprecationWarning)
if not os.path.isfile(WEBSERVER_CONFIG):
import shutil
log.info("Creating new FAB webserver config file in: %s", WEBSERVER_CONFIG)
+ os.makedirs(os.path.dirname(AIRFLOW_HOME + "/config"), exist_ok=True)
Review Comment:
Right makes sense. Thanks for the feedback.
Making the required change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1137699588
##########
airflow/configuration.py:
##########
Review Comment:
I have a proposal here. How about we do this:
1. We have a constant called AIRFLOW_WEBSERVER_CONFIG around. If this is set to the path of the users webserver_config, we pick that.
2. In case that constant is unset, we can just pick the default location. Note: the default location will now be AIRFLOW_HOME/config (the new one)
I think this is a bit more reasonable. Would like to hear your thoughts too..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1141620277
##########
tests/www/views/test_views.py:
##########
@@ -62,6 +62,13 @@ def test_configuration_expose_config(admin_client):
check_content_in_response(["Airflow Configuration"], resp)
+@mock.patch.dict(os.environ, {"AIRFLOW__WEBSERVER_CONFIG_FILE": "{AIRFLOW_HOME}/config/webserver_config.py"})
Review Comment:
Trying something like this too
```
@mock.patch("airflow.configuration.conf")
def test_webserver_configuration_config_file(conf):
conf.get.return_value = "/tmp/webserver_config.py"
assert WEBSERVER_CONFIG == "/tmp/webserver_config.py"
```
I think I am missing something trivial here. Any idea?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1146255549
##########
airflow/config_templates/config.yml:
##########
@@ -1264,6 +1264,14 @@ hive:
webserver:
description: ~
options:
+ config_file:
+ description: |
+ Path of webserver_config.py file used for configuring the webserver parameters with
+ respect to the AIRFLOW_HOME
Review Comment:
Oh yes, great suggestion. Made the changes 👍🏽
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1196000965
##########
airflow/config_templates/config.yml:
##########
@@ -1334,6 +1334,13 @@ hive:
webserver:
description: ~
options:
+ config_file:
+ description: |
+ Path of webserver config file used for configuring the webserver parameters
+ version_added: 2.6.0
Review Comment:
This will be 2.7.0 now.
```suggestion
version_added: 2.7.0
```
##########
airflow/configuration.py:
##########
@@ -1548,9 +1548,8 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
global WEBSERVER_CONFIG
Review Comment:
Let's move this up to the list of globals at the top of the function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29926: Making webserver config customisable
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1196069355
##########
airflow/config_templates/config.yml:
##########
@@ -1334,6 +1334,13 @@ hive:
webserver:
description: ~
options:
+ config_file:
+ description: |
+ Path of webserver config file used for configuring the webserver parameters
+ version_added: 2.6.0
Review Comment:
Thank you. Made the changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29926: Making webserver config customisable
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29926:
URL: https://github.com/apache/airflow/pull/29926#discussion_r1196893688
##########
airflow/configuration.py:
##########
@@ -1548,10 +1548,7 @@ def initialize_config() -> AirflowConfigParser:
if local_conf.getboolean("core", "unit_test_mode"):
local_conf.load_test_config()
- # Make it no longer a proxy variable, just set it to an actual string
- global WEBSERVER_CONFIG
- WEBSERVER_CONFIG = AIRFLOW_HOME + "/webserver_config.py"
-
+ WEBSERVER_CONFIG = local_conf.get("webserver", "config_file")
Review Comment:
It's automatically handled. Most fallbacks can go honestly, much more useful in providers than core.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org