o-nikolas commented on code in PR #47322:
URL: https://github.com/apache/airflow/pull/47322#discussion_r1980173188
##########
airflow/settings.py:
##########
@@ -687,11 +687,7 @@ def initialize():
LAZY_LOAD_PROVIDERS: bool = conf.getboolean("core", "lazy_discover_providers",
fallback=True)
# Determines if the executor utilizes Kubernetes
-IS_K8S_OR_K8SCELERY_EXECUTOR = conf.get("core", "EXECUTOR") in {
- executor_constants.KUBERNETES_EXECUTOR,
- executor_constants.CELERY_KUBERNETES_EXECUTOR,
- executor_constants.LOCAL_KUBERNETES_EXECUTOR,
-}
+IS_K8S_OR_K8SCELERY_EXECUTOR = conf.get("core", "EXECUTOR") ==
executor_constants.KUBERNETES_EXECUTOR
Review Comment:
Thanks for the review @jedcunningham!
I decided to stop the scope of the change here, I'll explain why, but it's a
fair call out.
I can update the name of the variable at least. But whether or not it
requires an interface change I'd like to push back on. That was true even
before these changes, all the way back to AIP-51 (here is [the
issue](https://github.com/apache/airflow/issues/27933) covering this coupling)
and I don't think it should be contingent on this PR being merged.
It takes a lot of code to support it in the interface. We'd have to build a
mechanism for executors to vend UI pages, and that's a lot of logic in a space
where there's been lots of changes. K8s is the only executor that has a UI
page. The solution I proposed the last couple times this has come up is to
actually just remove this code altogether since I think that's the easiest path
(and we love removing code for Airflow 3.0 :smiley: ). And I'm not sure how
many folks really even make use of the UI endpoint to render the config file.
I'd love to hear your thoughts on that. If it sounds agreeable I'll open a
separate 3.0 PR for that!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
