Share airflowctl's Dag run lookup without a new request for run_id - #70904
Conversation
SameerMesiah97
left a comment
There was a problem hiding this comment.
Looks good. This is a relatively low-risk refactor,
The dags and tasks commands each carried their own copy of the same logical-date Dag run lookup, differing only in what they returned, and each repeated the run-selector guard in front of it. Sharing them lets the selector handling live in one place rather than in front of every command that accepts one. A supplied run_id is deliberately still taken at face value rather than fetched, so commands acting on a nested resource keep reporting a miss against that resource instead of against the Dag run. The added assertions pin that down, since collapsing the two resolvers would otherwise silently add a request and re-attribute the 404.
5b0e588 to
3aaccc1
Compare
|
Heads-up on an incoming conflict — not a review.
if (args.run_id is None) == (args.logical_date is None):
rich.print("[red]Provide either run_id or --logical-date, but not both[/red]")
sys.exit(1)
run_id = args.run_id or _find_run_id_by_logical_date(api_client, args.dag_id, args.logical_date)I've approved that PR, so it will most likely land first. When it does, this one will conflict in Nothing to do right now. Flagging it so the conflict isn't a surprise when it turns up, and so the extra call site reads as expected rather than as something that crept in while this was waiting. This note is only about the overlap — I haven't left a review on this PR itself.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
potiuk
left a comment
There was a problem hiding this comment.
Clean refactor — the two lookup helpers move verbatim, the duplicated logical-date lookup in task_command.py collapses into the shared one, and keeping resolve_dag_run / resolve_dag_run_id separate (with the docstring saying why a supplied run_id isn't fetched) is the right call. The new assert_not_called() checks turn that "no extra request" behaviour into an enforced contract. Approving.
This review was drafted by an AI-assisted tool and
confirmed by an Apache 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 Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Backport failed to create: airflow-ctl/v0-1-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker 6e48e0f airflow-ctl/v0-1-testThis should apply the commit to the airflow-ctl/v0-1-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
Summary
airflowctl tasks failed-depsandairflowctl tasks states-for-dag-runintentionally do not look up a Dag run when the user supplies arun_id; the ID is passed directly to the task-instance endpoint. That behavior was introduced in #69915 to avoid an unnecessary HTTP request, but the remaining logical-date lookup and selector validation were duplicated across commands.This PR moves the shared logic into
airflowctl/ctl/utils/dag_run.py, removing the duplication while preserving the existing request pattern and user-visible behavior.Why two resolvers?
The split is intentional.
resolve_dag_run_idreturns a suppliedrun_iddirectly, avoiding the extra Dag run lookup.resolve_dag_runfetches the Dag run because commands such asdags stateneed the full response to display fields such asstateandconf.Both helpers share the selector validation and logical-date lookup, but keep the different
run_idbehavior intact.A single parameterized resolver was considered, but since the two helpers naturally return different types (
strvsDAGRunResponse), combining them would require either a union return type or@overloaddefinitions. That adds more complexity than the small amount of dispatch logic it would eliminate.Tests
The "don't look up a supplied
run_id" behavior was previously not enforced by the test suite.As a demonstration, adding a verification-only
dag_runs.getcall (for example, to validate a suppliedrun_id) produces the following result:mainThe new assertions make this behavior an explicit contract and prevent future refactors from unintentionally adding the extra request.
Notes
airflow-ctlrelease managers regenerate the changelog fromgit log.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines