Remove no-op overrides and deduplicate Dag run lookup in airflowctl - #70898
Closed
rjgoyln wants to merge 1 commit into
Closed
Remove no-op overrides and deduplicate Dag run lookup in airflowctl#70898rjgoyln wants to merge 1 commit into
rjgoyln wants to merge 1 commit into
Conversation
Three pieces of airflowctl did nothing but obscure the code around them: an except clause that re-raised its exception unchanged, an argparse override that only called super(), and a Dag run lookup that existed in two commands. The re-raise is the same pattern cleaned out of the operations layer in apache#70376; it survived there because it sits in client.py instead. Sharing the lookup also lets the run selector validation live in one place rather than being repeated 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three cleanups in
airflow-ctl, all behaviour-preserving.1.
client.py— a no-op re-raise.get_clientcaughtAirflowCtlNotFoundExceptiononly toraise eunchanged. This is the same pattern removed from the operations layer in #70376; this one survived because it sits inclient.pyrather thanoperations.py. The exception is still handled bysafe_call_commandincli_config.py, andapi_client.close()still runs in the survivingfinally.2.
cli_config.py— a redundant override.DefaultHelpParser._check_valueonly calledsuper(). airflow-core's namesake does real work (check_legacy_command), but airflow-ctl deliberately has no legacy-command mapping, so this was a degenerate copy of it.3.
dag_command/task_command— a duplicated Dag run lookup. Both commands carried a near-identical logical-date lookup, each preceded by the same run-selector guard. Guard and lookups now live inctl/utils/dag_run.py.Why two resolvers rather than one
task_commandmust not fetch the Dag run when arun_idis supplied. Doing so would add an HTTP request that did not previously happen, and would re-attribute a 404 fromTask instance … not foundtoDag run … not found. Soresolve_dag_run_idtakes a suppliedrun_idat face value, whileresolve_dag_run(used bydags state, which needs the response body either way) fetches it. They share the selector guard and the logical-date lookup, so no duplication survives the split.The two added assertions pin that invariant down — without them, a future "these two look the same, merge them" cleanup would silently add a request and swap the error message.
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