Repository navigation
Escape server and user text in airflowctl command messages - #76265
Conversation
github-actions
Bot
added
need workflow approval
and removed
need workflow approval
labels
Andrushika
left a comment
Andrushika
left a comment
There was a problem hiding this comment.
Left few comments, thanks.
github-actions
Bot
added
need workflow approval
and removed
need workflow approval
labels
shahar1
left a comment
shahar1
left a comment
There was a problem hiding this comment.
The escaping is correct and the message text is unchanged, but the dags clear preview still renders Dag run ids as markup, and a few of the new escapes have no test. None of this is blocking.
I ran the airflow-ctl CLI tests at this head (496 passed). I then removed each escape in turn to see which ones the tests notice.
Smaller observations
- minor
airflow-ctl/src/airflowctl/ctl/commands/dag_command.py:250(not in the diff, so not inline):_print_dag_runs_to_clearstill prints the server'sdag_run_idas markup. A run id containing[/x]raisesMarkupErrorbefore the confirmation prompt. A run idrun[abc]is listed asrun, so the prompt shows the wrong id before a destructive action. Reaching this needs a Deployment whoseallowed_run_id_patternis wider than the default^[A-Za-z0-9_.~:+-]+$, which rejects[.{escape(dag_run.dag_run_id)}plus a test would fix it, and the PR already lists Dag run ids in its scope. - minor
airflow-ctl/src/airflowctl/ctl/commands/dag_command.py:123: some of the new escapes have no test (see inline). - minor
airflow-ctl/tests/airflow_ctl/ctl/commands/test_auth_command.py:653: the new tests use unspec'd mocks (see inline). - nit PR description: the
Tests:line still names three tests, and the branch now has more, covering tasks, connections, auth and config. Worth refreshing if the description feeds the squash message.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The findings
below are observations, not blockers; an Apache Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Airflow handles maintainer review:
Contributing guide.
Several airflowctl commands interpolated server error text, Dag run ids and logical dates into Rich markup unescaped. A value containing a tag-like fragment such as "[/x]" made rich.print raise MarkupError, so the user saw a traceback instead of the actual error. Escape the interpolated text in the dags, tasks, connections, auth and config commands and in the Dag run lookup helpers.
variables import printed the bulk result unescaped, and variable keys can contain "[" and "/", so a key such as "a[/x]" crashed the success message with MarkupError after the import had already been applied. Escape it, and add tests for the task, connections, auth and config messages escaped earlier in this PR so removing the escapes fails them.
The server-provided dag_run_id was printed as Rich markup before the confirmation prompt, so ids such as "[/x]" raised MarkupError and "run[abc]" was listed as "run". Escape it. Add tests so removing any of the existing escapes (next-execution error, tasks states-for-dag-run, logical-date not found, Dag pause error, run id not found) fails a test, and give the mocked clients added by the PR a Client spec. Claude-Session: https://claude.ai/code/session_01BVx6g9eNzpyiubnaPgUz9o
6bee40a to
00e2a90
Compare
|
@Rishabh00b I took over to close last minor gaps before starting the release process of airflowctl v0.1.6. |
Thanks for the review and for finishing the remaining escapes! Glad it could help with the v0.1.6 release. |
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 abc3a20 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. |
…mand messages (#76265) (#76621) Several airflowctl commands interpolated server error text, Dag run ids and logical dates into Rich markup unescaped. A value containing a tag-like fragment such as "[/x]" made rich.print raise MarkupError, so the user saw a traceback instead of the actual error. (cherry picked from commit abc3a20) Conflict resolution: this branch has no ctl/utils/dag_run.py, so the escapes from that module go to the lookup helpers it keeps inline in dag_command.py and task_command.py, and its tests are rewritten against `dags state` and `tasks states-for-dag-run`. `dags clear` and `tasks failed-deps`, `state` and `logs` do not exist here, so their escapes and tests are dropped. "Error retrieving DAG" keeps this branch's wording. Generated-by: Claude Code (Opus 5.5) Claude-Session: https://claude.ai/code/session_01BVx6gA44HQY4GEf2fEAQwE Co-authored-by: Rishabh <[email protected]>
…mand messages (apache#76265) (apache#76621) Several airflowctl commands interpolated server error text, Dag run ids and logical dates into Rich markup unescaped. A value containing a tag-like fragment such as "[/x]" made rich.print raise MarkupError, so the user saw a traceback instead of the actual error. (cherry picked from commit abc3a20) Conflict resolution: this branch has no ctl/utils/dag_run.py, so the escapes from that module go to the lookup helpers it keeps inline in dag_command.py and task_command.py, and its tests are rewritten against `dags state` and `tasks states-for-dag-run`. `dags clear` and `tasks failed-deps`, `state` and `logs` do not exist here, so their escapes and tests are dropped. "Error retrieving DAG" keeps this branch's wording. Generated-by: Claude Code (Opus 5.5) Claude-Session: https://claude.ai/code/session_01BVx6gA44HQY4GEf2fEAQwE Co-authored-by: Rishabh <[email protected]>
Several airflowctl commands put server error text, Dag run ids and logical dates into Rich markup unescaped. A value containing a tag-like fragment such as
[/x]makesrich.printraiseMarkupError, so the user gets a traceback instead of the actual error.The text is now escaped in the dags, tasks, connections, auth and config commands and in the Dag run lookup helpers. It is independent of the
safe_call_commandfix, which covers the central handler.Tests: removing any of the escapes fails a test, except the rejected value in the logical-date not-found message, which has already passed
datetime.fromisoformatand cannot contain markup.Was generative AI tooling used to co-author this PR?