dheerajturaga commented on PR #70112: URL: https://github.com/apache/airflow/pull/70112#issuecomment-5073041683
Thanks for tackling this — the exit-code bug is real and the motivation is spot on. I reviewed the change and have one blocker plus a couple of suggestions. **The production fix is correct.** Switching the three `except TypeError` handlers from bare `raise SystemExit` (exit code 0) to `raise SystemExit(str(e))` (exit 1 + message on stderr) is the right call. The model layer already logs the error before raising, so dropping the CLI-side `logger.error(str(e))` is a clean de-dup, and `raise SystemExit(str(e))` matches the existing convention in `cli/worker.py`. (Omitting `from e` is fine here — B904 isn't in the repo's ruff config, and the `worker.py` sites omit it too.) **🔴 Blocker — the new test fails.** The test builds args with `types.SimpleNamespace`, but these commands are wrapped by `@cli_utils.action_cli`, whose `_check_cli_args` (`airflow-core/src/airflow/utils/cli.py:56`) rejects anything that isn't an `argparse.Namespace`: ``` ValueError: 1st positional argument should be argparse.Namespace instance, but is <class 'types.SimpleNamespace'> ``` That `ValueError` escapes before any `SystemExit`, so `pytest.raises(SystemExit)` never matches and all three cases error out — which is what the red `Non-DB tests: providers … edge3` and `Provider distributions tests` jobs are reporting. Swapping `SimpleNamespace` → `argparse.Namespace` (same fields) makes all three pass, and the test then correctly fails without the production change. **🟡 Suggestion — two identical bug sites remain.** The same bare-`raise SystemExit`-after-`except TypeError` pattern is still present at `edge_command.py:329-330` (`remote_worker_update_maintenance_comment`) and `:344-345` (`remove_remote_worker`); their model functions (`change_maintenance_comment`, `remove_worker`) log-and-raise `TypeError` just like the three you fixed. Folding these two into this PR (and into the parametrized test) would make the fix complete rather than partial. **Minor — test comment.** The comment says it checks "exit code 1," but the assertion checks that `.code` equals the message string; the exit-status-1 behavior happens later when the interpreter prints a non-int `code`. Might reword to avoid conflating the two. --- Drafted-by: Claude Code (Opus 4.8); reviewed by @dheerajturaga 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]
