Add airflowctl tasks state command - #71206
haseebmalik18 wants to merge 3 commits into
Conversation
|
@bugraoz93 @henry3260 Following up |
potiuk
left a comment
There was a problem hiding this comment.
Approving — this is cleanly done. The error handling matches failed_deps and states-for-dag-run exactly, plain print for the value with rich.print reserved for errors is the right split for machine-readable output, and no newsfragment is correct for airflow-ctl.
Two things I checked specifically rather than assumed:
- The generated docs artifacts were genuinely regenerated.
command_hashes.txtmoves thetaskshashea587dc8…→eb70701d…, andoutput_tasks.svghas its terminal class ids rewritten wholesale (terminal-2578352511-*→terminal-761188829-*) — the generator's signature, not a hand edit. - Test coverage is the most thorough I've seen in this queue today: run_id, logical-date, map_index, null state, mutually-exclusive selectors, malformed logical date, run-not-found, task-instance-not-found and non-404 re-raise, plus an integration case in
airflow-ctl-testsfor both invocation forms.
Two nits inline, neither blocking. One of them is really a heads-up about another PR rather than a criticism of this one.
At merge time, note that 10 of the 11 commits here are Merge branch 'main' into…. Since apache/airflow squashes using the commit messages, the squash body will carry all ten unless it is tidied up — worth a glance when this goes in.
This review was drafted by an AI-assisted tool and confirmed by an Airflow maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.
More on how Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
bd1c5db to
e9678ab
Compare
|
Addressed review comments |
e9678ab to
2645396
Compare
|
Thanks fot the patch! LGTM+1 |
For a mapped task queried without --map-index the API returns a 404 explaining the task is mapped, but tasks state and tasks failed-deps reported only a bare 'not found', leaving users guessing. Generated-by: Claude Opus 5
2645396 to
1196b37
Compare
potiuk
left a comment
There was a problem hiding this comment.
Thanks for this — and for the doc note on None, that drop-in compatibility with airflow tasks state is worth spelling out.
I rebased onto current main and pushed one small follow-up commit on top (1196b378ea) with the follow-up suggested above: when a mapped task is queried without --map-index, the API's 404 detail ("Task instance is mapped, add the map_index value to the URL") was swallowed and tasks state / tasks failed-deps printed only "…not found". The shared not-found message now adds "The task is mapped; pass --map-index to select one of its task instances" in that case, and both commands' not-found tests gained a mapped-task case.
Approving; merging once CI is green.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
This adds
airflowctl tasks state, which prints the state of a single task instance. It is the airflowctl equivalent of the deprecatedairflow tasks state.You select the run with either a run id or a logical date, and mapped tasks are supported through
--map-index:The output is just the state value (for example
success) to stay script friendly. It reuses the existing task instance API operation, and error handling matches the siblingfailed-depsandstates-for-dag-runcommands.closes: #66174