SEZ9 commented on PR #13021: URL: https://github.com/apache/seatunnel/pull/13021#issuecomment-6553162119
Thanks for the follow-up revision. I have not yet been able to verify the latest head `44172450151f6f263fce032c99cb7f6b76678903` against the diff directly, so the earlier points remain open until confirmed: 1. **PR13021-F4** – Is `HELP_EXIT_CODE` now centralized next to `USAGE_EXIT_CODE` in `SeaTunnelStarterConstants`, with the native mains opting in through a `parse(..., int helpExitCode)` overload and the shared parser default staying at 234? 2. **PR13021-F2** – Does bare `seatunnel-edge-agent db` (no subcommand) now exit 234 rather than 0 through the shared help command? 3. **PR13021-F3** – Do both subprocess test helpers drain stdout concurrently and destroy the child process on timeout? 4. **PR13021-F1** – Could you confirm `NativeHelpExitCodeTest` asserts exact values (`0` for `--help`, `234` for a missing required option) rather than `!= 0`, and what the `-pt invalid` cases assert? If they exit 1 via an uncaught `IllegalArgumentException`, that is still not the usage path; either route it to 234 or have the test document that 1 is intentional. 5. **PR13021-F6** – Please add cases to `EdgeAgentHelpExitCodeTest` for bare `db`, `db help`, and an unknown `db` subcommand with their expected exit codes, since these are the paths this PR changes. 6. **PR13021-F7** – `incompatible-changes.md` should call out the two no-argument forms explicitly: bare `seatunnel-connector.sh` (wrapper injects `-h`) and bare `seatunnel-edge-agent db`, with their resulting exit codes, so the "invalid arguments still exit nonzero" sentence stays accurate. 7. **PR13021-F5** (minor) – either share the `invokeMain`/`ProcessResult` harness between the two test classes or add a short comment on why a forked JVM is needed, and drop the 234-default assertion already covered in-process by `ClientCommandArgsTest`. Also, the Build check (`123364442476`) is showing ACTION_REQUIRED and the branch is behind `dev` (`b274eb1f2877f62a534933253c6243248521b7e2`); a rebase may help get CI running. Happy to approve once the items above are confirmed or addressed. <!-- streview-comment:2195 --> -- 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]
