potiuk commented on code in PR #33423:
URL: https://github.com/apache/airflow/pull/33423#discussion_r1297579757
##########
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:
> but I think we cannot entirely rule out the possibility that a user would
face this error, especially as we add more "command vendors", the number of
combination raises quadratically, and we cannot expect developers to test them
all.
We can try to enforce it with "officially" provided components, but with
custom components, all bets are off.
Yes. But one comment/ suggestion.
This is the same with "config" we handle now in exactly the same way. I
think there are two solutions for "all bets off" and potential conflicts:
1) introduce some kind of central registry of commands and "give them" to
those who want certain id (pessimistic, top-down-driven, centralized, does not
play well with free/open-source project). This for example what IP addresses,
DNS names, IMEI numbers for simcards are about as well as few others.
2) make those who introduce custom commands painfully aware that THEY have
to do everythig to make their commands unique. This is far softer, distributed
and more self-regulating. The problem is that those who develop things will
have problem if they use non-unique names. So anyone doing "aws executors" ;)
should have commands starting with "aws-" to avoid contention. That is more or
less what java and python packages follow. Nobody keeps the registry of those,
yet somehow everyone takes care to not use generic package names following
common conventions ("google.*",. "aws.*") and it **just works** (TM).
Obviously 2) is better for us.
This is something we might want to document as "soft convention" and mention
why ("When you create a custom config/CLI, you should make sure to use
unique-enough prefix indicating your unique product/service/etc. to avoid
conflicts with other commands/configs added by others.")
Maybe we should mention it somewhere where we document how to create custom
executor (and likely should be the same on custom provider for config). Once we
document it, it's really on those who will add new commands to worry about it
(and yes I know you are on both sides of it :D ).
--
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]
