[GitHub] [airflow] vincbeck commented on a diff in pull request #33423: Add a mechanism to warn if executors override existing CLI commands

["vincbeck (via GitHub)" <gi...@apache.org>]

vincbeck commented on code in PR #33423:
URL: https://github.com/apache/airflow/pull/33423#discussion_r1297318297


##########
airflow/cli/cli_parser.py:
##########
@@ -59,13 +61,23 @@
         "a 3.3.0+ version of the Celery provider. If using a Kubernetes executor, install a "
         "7.4.0+ version of the CNCF provider"
     )
-    # Do no re-raise the exception since we want the CLI to still function for
+    # Do not re-raise the exception since we want the CLI to still function for
     # other commands.
 
 
 ALL_COMMANDS_DICT: dict[str, CLICommand] = {sp.name: sp for sp in airflow_commands}
 
 
+# Check if sub-commands are defined twice, which could be an issue.
+if len(ALL_COMMANDS_DICT) < len(airflow_commands):
+    dup = {k for k, v in collections.Counter([c.name for c in airflow_commands]).items() if v > 1}
+    raise CliConflictError(
+        f"The following CLI {len(dup)} command(s) are defined more than once: {sorted(dup)}\n"
+        f"This can be due to the executor '{ExecutorLoader.get_default_executor_name()}' "
+        f"redefining core airflow CLI commands."

Review Comment:
   Do we want to have this message? In the future it might no longer be true and other components such as auth managers could be able to also define their own CLI command



##########
tests/cli/conftest.py:
##########
@@ -34,17 +36,14 @@
 custom_executor_module.CustomCeleryKubernetesExecutor = type(  # type: ignore
     "CustomCeleryKubernetesExecutor", (celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
 )
-custom_executor_module.CustomCeleryExecutor = type(  # type:  ignore
-    "CustomLocalExecutor", (celery_executor.CeleryExecutor,), {}
-)
-custom_executor_module.CustomCeleryKubernetesExecutor = type(  # type: ignore
-    "CustomLocalKubernetesExecutor", (celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
+custom_executor_module.CustomLocalExecutor = type(  # type:  ignore
+    "CustomLocalExecutor", (local_executor.LocalExecutor,), {}
 )
-custom_executor_module.CustomCeleryExecutor = type(  # type:  ignore
-    "CustomKubernetesExecutor", (celery_executor.CeleryExecutor,), {}
+custom_executor_module.CustomLocalKubernetesExecutor = type(  # type: ignore
+    "CustomLocalKubernetesExecutor", (local_kubernetes_executor.LocalKubernetesExecutor,), {}
 )
-custom_executor_module.CustomCeleryKubernetesExecutor = type(  # type: ignore
-    "CustomCeleryKubernetesExecutor", (celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
+custom_executor_module.CustomKubernetesExecutor = type(  # type:  ignore
+    "CustomKubernetesExecutor", (kubernetes_executor.KubernetesExecutor,), {}

Review Comment:
   Nice!



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