Support --map-index in airflowctl tasks state - #68776
IamJasonBian wants to merge 25 commits into
Conversation
Introduce `airflowctl tasks state` to retrieve the state of a task
instance by calling GET /api/v2/dags/{dag_id}/dagRuns/{run_id}/
taskInstances/{task_id}. Also add TaskInstancesOperations, help text,
and an integration test for the auto-generated taskinstances get command.
# Conflicts:
# airflow-ctl-tests/tests/airflowctl_tests/test_airflowctl_commands.py
Allow `airflowctl tasks state` to query mapped task instances, matching the legacy `airflow tasks state --map-index` behaviour. When the flag is non-negative the mapped task instance endpoint is called; otherwise the existing unmapped endpoint is used. The `taskinstances get` auto-generated command also gains optional mapped-instance support via the same `map_index` parameter on `TaskInstancesOperations.get`.
The integration test invoked `taskinstances get` with `--dag-id`/`--dag-run-id`/ `--task-id` flags, but that auto-generated command takes positional arguments, so argparse exited with code 2 and the test failed. The flag syntax belongs to the new `tasks state` command, which had no integration coverage at all. Point the entry at `tasks state` (flags) to cover the new command, and add a correctly-formed `taskinstances get` entry using positional args so both commands are exercised.
There was a problem hiding this comment.
Do you think it's also worth adding a test to the test_operations.py file for the get command? The tests you added only seem to cover tasks state.
There was a problem hiding this comment.
@justinpakzad added here, let me know if I'm checking the right things!
There was a problem hiding this comment.
Looks good. Just had another look and none of the existing tests in that file test the error path so I think we can remove test_get_not_found_raises.
Cover the underlying task instances get operation directly — mapped and unmapped endpoints plus 404 handling — not just the tasks state command.
No other tests in the file cover the error path, so drop test_get_not_found_raises and its now-unused ServerResponseError import.
|
@potluk - can we try a pass at the tests? ++ @jason810496 |
|
@IamJasonBian This PR has a few issues that need to be addressed before it can be reviewed — please see our Pull Request quality criteria. Issues found:
What to do next:
There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the Airflow Slack. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
The command took required --dag-id/--dag-run-id/--task-id flags, which no other airflowctl command does: the auto-generated commands and core airflow both take identifiers positionally. Users moving between `airflow tasks state` and `airflowctl tasks state` hit a different shape for the same command name. The state value was also printed via str() on a str-mixin enum, which renders as "TaskInstanceState.SUCCESS" rather than "success". Task instances are reached through the hand-written `tasks` group, so the operations class is excluded from command auto-generation to avoid shipping a second, overlapping `taskinstances` surface.
…' into airflowctl-tasks-state-map-index
|
@potiuk Ready for Review! AirflowConsole._normalize_data falls through to a bare str() on (str, Enum) fields, using a Testingchecked re-ran |
Resolve conflicts with the new tasks states-for-dag-run command (apache#69366): - keep both 'tasks state' (with --map-index) and 'tasks states-for-dag-run' in a single TASK_COMMANDS group - merge TaskInstancesOperations so it exposes both get() and list() - drop the TaskInstancesOperations exclusions so the auto-generated taskinstances group (get/list) stays available, add help text and an integration-test entry for 'taskinstances get' - regenerate command hashes and tasks/taskinstances help images
potiuk
left a comment
There was a problem hiding this comment.
Thanks for working on this, and sorry it sat for a while. Unfortunately #71206 has since added the same airflowctl tasks state command for #66174. It already supports --map-index and --logical-date, handles a missing task instance with a clean error, and is approved with green CI. So I'd suggest closing this one in favour of #71206.
For completeness, this branch also can't land as it stands:
task_stateis never registered inairflow-ctl/src/airflowctl/ctl/cli_config.py(there is notasks stateActionCommand).airflowctl tasks state ...is rejected by argparse, and the new tests intest_task_command.pyfail atparse_args.test_operations.pyadds a secondclass TestTaskInstancesOperations. One already exists earlier in that module, so the redefinition shadows it and its tests stop being collected.help_texts.yamlends up with twoget:keys undertaskinstances:, and the integration-test entry added is a duplicate of the existingtaskinstances getline rather than atasks statecase.
If you'd like to help further, a review or a follow-up on #71206 is very welcome. One open idea there is surfacing the API's "task is mapped, pass a map index" detail in the 404 message.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Description
Adds a
tasks statecommand toairflowctland supports--map-indexso thestate of a specific mapped task instance can be retrieved through the CLI:
Also fixes the airflowctl integration test that exercised this area: it had
invoked the auto-generated
taskinstances getcommand with--dag-id/--dag-run-id/--task-idflags, which that command does not accept (it takespositional args), causing argparse to exit 2. The entry now uses
tasks state(flags) to cover the new command, plus a correctly-formed positional
taskinstances getentry so both commands are exercised.Testing
Set up localhost:8080
Ran below after toggling in UI
Reran CI locally
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines