edge3: Exit non-zero when worker admin CLI commands fail - #70112
Conversation
20d94d1 to
244b190
Compare
|
@jscheffl @dheerajturaga can you review this. |
|
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 🔴 Blocker — the new test fails. The test builds args with That 🟡 Suggestion — two identical bug sites remain. The same bare- Minor — test comment. The comment says it checks "exit code 1," but the assertion checks that Drafted-by: Claude Code (Opus 4.8); reviewed by @dheerajturaga before posting |
244b190 to
feb57e4
Compare
The add-worker-queues, remove-worker-queues, set-worker-concurrency,
update-maintenance-comments, and remove-worker subcommands raised bare
`SystemExit` after catching a TypeError from the model layer, which
yields exit status 0. Shell scripts wrapping these commands treated
failures as success and continued as if the queue, concurrency,
maintenance-comment, or worker-removal change had been applied.
Pass the exception message to SystemExit so the process exits with
status 1 and writes the failure to stderr, matching the file's own
convention for validation-error paths (see the explicit
`raise SystemExit("Error: ...")` calls in the same subcommands) and the
`raise SystemExit(str(e))` pattern already used elsewhere in the edge3
CLI. The model layer already logs the error before raising, so the
redundant logger.error call in the three subcommands that had one is
removed.
feb57e4 to
862f168
Compare
|
@dheerajturaga updated the pr based on your suggestion. |
dheerajturaga
left a comment
There was a problem hiding this comment.
Awesome! thanks for fixing this
The add-worker-queues, remove-worker-queues, set-worker-concurrency,
update-maintenance-comments, and remove-worker subcommands raised bare
`SystemExit` after catching a TypeError from the model layer, which
yields exit status 0. Shell scripts wrapping these commands treated
failures as success and continued as if the queue, concurrency,
maintenance-comment, or worker-removal change had been applied.
Pass the exception message to SystemExit so the process exits with
status 1 and writes the failure to stderr, matching the file's own
convention for validation-error paths (see the explicit
`raise SystemExit("Error: ...")` calls in the same subcommands) and the
`raise SystemExit(str(e))` pattern already used elsewhere in the edge3
CLI. The model layer already logs the error before raising, so the
redundant logger.error call in the three subcommands that had one is
removed.
add-worker-queues,remove-worker-queues, andset-worker-concurrencyraised bareSystemExitafter catching aTypeErrorfrom the model layer (raised when the target worker is inOFFLINE,OFFLINE_MAINTENANCE, orUNKNOWNstate).SystemExit()with no argument yields exit code0, so shell scripts wrapping these commands saw success and continued as if the queue or concurrency change had been applied, when in fact it had been silently rejected.Pass the exception message to
SystemExit(str(e))so the process exits with status1and writes the failure to stderr. Matches this file's own convention for validation-error paths (e.g.raise SystemExit("Error: No queues specified to add.")a few lines above each fix site) and theraise SystemExit(str(e))pattern already used inproviders/edge3/src/airflow/providers/edge3/cli/worker.py.The model layer at
providers/edge3/src/airflow/providers/edge3/models/edge_worker.pyalready callslogger.error(error_message)before raising, so the CLI-sidelogger.error(str(e))was strictly duplicative and is removed.Adds parametrized tests over all three subcommands asserting that
SystemExit.codecarries the error message (i.e. exit status is1, not0).Was generative AI tooling used to co-author this PR?