Eason09053360 commented on PR #72145: URL: https://github.com/apache/airflow/pull/72145#issuecomment-5454803322
Thanks for the steer — I did the wider check before taking this to the list. **Devlist thread:** https://lists.apache.org/thread/v7wonnnlf2t444ojgkmdx0j03cw51op3 ### What the audit found I ran every `airflow` CLI command that takes an identifier, looks one thing up and prints it to stdout, in Breeze against a migrated metadata DB — redirecting stdout and stderr to separate files so the two streams could be told apart and sized: - **8 of the 10 cases already exit non-zero** on "not found"; 7 of those with a clean one-line message on stderr. - `config get-value` (this PR) and `airflow dags state <dag> <bad-run-id>` are the only two that exit 0. The latter prints the literal string `None` to stdout. `git log` suggests that was never a decision: #21793 added a `SystemExit` for the missing-DAG case in the same function but left that branch alone, and #46407 mechanically preserved it while refactoring for null logical dates. - `tasks states-for-dag-run` exits non-zero but surfaces a raw `DagRunNotFound` traceback rather than a clean message. Cosmetic and a different problem — left out of scope. <details> <summary>The script I ran (<code>breeze --answer yes run bash /opt/airflow/dev/audit.sh</code>)</summary> ```bash #!/usr/bin/env bash set -uo pipefail cd /opt/airflow airflow db migrate >/dev/null 2>&1 airflow dags reserialize >/dev/null 2>&1 # a real DAG is needed for the "dag exists, run does not" probe EXISTING_DAG=$(airflow dags list -o json | python -c \ 'import json,sys; d=json.load(sys.stdin); print(d[0]["dag_id"] if d else "")') run_case() { local label="$1"; shift local out err rc out=$(mktemp); err=$(mktemp) "$@" >"$out" 2>"$err"; rc=$? # separate files -> streams stay distinguishable printf '\nCASE : %s\nCMD : %s\nEXIT : %s\n' "$label" "$*" "$rc" printf 'OUT : %s bytes\n' "$(wc -c <"$out" | tr -d ' ')"; [ -s "$out" ] && sed 's/^/ > /' "$out" printf 'ERR : %s bytes\n' "$(wc -c <"$err" | tr -d ' ')"; [ -s "$err" ] && sed 's/^/ ! /' "$err" rm -f "$out" "$err" } run_case "config get-value" airflow config get-value missing-section missing-option run_case "variables get" airflow variables get no_such_variable run_case "pools get" airflow pools get no_such_pool run_case "connections get" airflow connections get no_such_conn run_case "providers get" airflow providers get no.such.provider run_case "dags details" airflow dags details no_such_dag run_case "dags state (dag missing)" airflow dags state no_such_dag some_run_id run_case "dags state (run missing)" airflow dags state "$EXISTING_DAG" no_such_run_id run_case "tasks states-for-dag-run" airflow tasks states-for-dag-run "$EXISTING_DAG" no_such_run_id run_case "assets details" airflow assets details --name no_such_asset ``` The row that matters most, verbatim: ``` CASE : dags state (run missing) CMD : airflow dags state aggregate_regional_sales no_such_run_id EXIT : 0 OUT : 5 bytes > None ERR : 0 bytes ``` </details> ### On airflow-ctl `airflowctl config get` exists but is generated over the Public API rather than hand-written, and that endpoint already returns `404 Option [section/option] not found.`. So the REST API and `airflowctl` already treat this as an error — the legacy CLI is the outlier, not the other way round. That is the framing the devlist thread proposes: this is not a new convention, it is finishing an existing one. It also notes that under AIP-94 (`contributing-docs/27_cli_implementation_guide.rst`) the existing `airflow` CLI remote commands get rewired to call the Public API, so `config get-value` will inherit the 404 and stop exiting 0 regardless — the open question is whether that happens deliberately now with a newsfragment, or silently later as a side effect. ### Newsfragment Happy to add one to this PR now. Holding off only until the list has had a chance to weigh in on whether the behaviour change is wanted at all, so the note describes what actually lands — say the word and I will push it straight away. --- Drafted-by: Claude Code (Opus 5); reviewed by @Eason09053360 before posting -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
