potiuk commented on code in PR #72858:
URL: https://github.com/apache/airflow/pull/72858#discussion_r3977380185
##########
airflow-core/src/airflow/cli/cli_parser.py:
##########
@@ -76,6 +104,11 @@
log.warning("Failed to load CLI commands from providers: %s", e)
# do not re-raise for the same reason as above
+ # Commands registered through a provider "cli" section take precedence
over the compat loading below
+ provider_command_names = {command.name for command in airflow_commands} - {
Review Comment:
This set is built once, before both compat loops, and never updated as they
extend `airflow_commands`. So `_exclude_registered_commands` dedupes against
provider `cli`-section commands, but not against commands the fallback itself
just registered.
That reopens the exact failure the helper was added to prevent, one level
down. With a provider executor whose provider has no `cli` section — the
population this fallback exists for — plus a custom subclass:
```ini
[core]
executor =
airflow.providers.celery.executors.celery_executor.CeleryExecutor,my_company.executors.MyCeleryExecutor
```
`executors_defined_cli` is empty so neither path is skipped,
`CeleryExecutor` registers `celery`, and `MyCeleryExecutor` inherits
`get_cli_commands()` and registers `celery` again — checked against a
`provider_command_names` that is empty here. The duplicate then trips the check
at line 178 and raises `CliConflictError` at import time, which takes down
every `airflow` command, not just the affected one.
Before this PR the loop skipped anything not in `executors_not_defined_cli`,
so the subclass was never imported and the collision was unreachable.
The tidiest fix is to make the set live rather than a snapshot — add
`registered_names.add(command.name)` just before `result.append(command)` in
`_exclude_registered_commands` (line 86). That closes it for the auth-manager
loop too, which reuses the same set after the executor loop has already
appended to `airflow_commands`.
##########
airflow-core/tests/unit/cli/test_cli_parser.py:
##########
@@ -179,7 +179,7 @@ def test_dynamic_conflict_detection(self,
mock_cli_command_functions: MagicMock)
@pytest.mark.parametrize(
"module_pattern",
- ["airflow.auth.managers", "airflow.executors.executor_loader"],
+ ["airflow.api_fastapi.auth.managers",
"airflow.executors.base_executor"],
Review Comment:
Two different changes bundled in one parametrize list, and I'd like to
separate them.
`airflow.auth.managers` → `airflow.api_fastapi.auth.managers` is a real fix:
the old module path no longer exists, so that parameter was asserting nothing
and passing vacuously. Good catch.
`airflow.executors.executor_loader` → `airflow.executors.base_executor` is
different — it drops the guard #59805 added, because this PR moves
`ExecutorLoader` to a module-level import. The docstring two lines below still
says *"cli_parser does not import auth_managers or executor_loader at import
time"*, so the test now claims coverage it no longer has.
To be fair on the actual impact: I checked, and there is **no** import-cost
regression. `airflow.cli.cli_config` already pulls
`airflow.executors.executor_loader` in transitively via `from airflow.jobs.job
import JobState`, so the module is in `sys.modules` before `cli_parser` runs
either way. The guard was already weaker than it looks.
Still, keeping that one import lazy where it was costs nothing and keeps the
guard meaningful for the `AIRFLOW_PACKAGE_NAME` docs-build path, which skips
the whole provider block. If removing it is deliberate, that's fine — please
just update the docstring and note the reasoning in the PR description so the
next person doesn't have to reconstruct it from the diff.
##########
airflow-core/src/airflow/cli/cli_parser.py:
##########
@@ -99,40 +132,48 @@
component="executors",
not_defined_cli_dict=str(executors_not_defined_cli)
)
)
- from airflow.executors.executor_loader import ExecutorLoader
-
- for executor_name in
ExecutorLoader.get_executor_names(validate_teams=False):
- # Skip if the executor already has CLI commands defined via
the 'cli' section in provider.yaml
- if executor_name.module_path not in executors_not_defined_cli:
- log.debug(
- "Skipping loading for '%s' as it is defined in 'cli'
section.",
- executor_name.module_path,
- )
- continue
-
- try:
- executor, _ =
ExecutorLoader.import_executor_cls(executor_name)
- airflow_commands.extend(executor.get_cli_commands())
- except Exception:
- log.exception("Failed to load CLI commands from executor:
%s", executor_name)
- log.error(
- "Ensure all dependencies are met and try again. If
using a Celery based executor install "
- "a 3.3.0+ version of the Celery provider. If using a
Kubernetes executor, install a "
- "7.4.0+ version of the CNCF provider"
+ executors_defined_cli = {
Review Comment:
Nit: this is the exact complement of `executors_not_defined_cli` built
immediately above, over the same `providers_manager.executor_without_check`
iterable — so it walks it a second time and states the same partition twice.
```python
executors_defined_cli = {
executor_name for executor_name, _ in
providers_manager.executor_without_check
} - executors_not_defined_cli.keys()
```
Same applies to `auth_managers_defined_cli` at line 188. Beyond saving a
pass, it keeps the two halves from drifting if the membership predicate ever
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
