SEZ9 commented on PR #12101: URL: https://github.com/apache/seatunnel/pull/12101#issuecomment-6514150263
Thanks for the update on this PR. On CI: the automated check reports head `55831abaef1e1aac58988977c1da8e73a27d9417` with "Signal pull request update" = `SUCCESS` but "Build" = `ACTION_REQUIRED`, i.e. the Build has not actually executed for this branch yet. Could you enable workflows in your fork's Actions tab (if disabled) and start the existing Build for this branch, or point us to an already-existing matching run? No empty commit or rebase is needed for that. Separately, the previous review findings on `SeaTunnelConfValidateCommand.java` and the docs are still open from my side, so please confirm the status of each or push the fixes: 1. **Static-validation failure path (HIGH)** — the thrown `ConfigCheckException` still re-attaches the unsanitized exception as its cause, which undoes the credential sanitization. Please drop the raw cause (or wrap a sanitized one). 2. **JSON escaping (MEDIUM)** — sanitized messages go into the machine-readable JSON output; please make sure they are emitted through a proper JSON serializer so quotes/newlines/control chars cannot break the document. 3. **Null message (MEDIUM)** — `sanitize(message)` is called before the null guard; the guard only protects the JSON payload, not the thrown exception. Please move the null check ahead of `sanitize`. 4. **Stdout purity in JSON mode (MEDIUM)** — in JSON mode the failure path still propagates `ConfigCheckException`, so text-mode logging/error output can interleave with the JSON on stdout. Please ensure stdout contains only the JSON document in that mode. 5. **`FileUtils.getConfigPath()` moved into the try (MEDIUM)** — this changes the exception type/message for missing or unresolvable config files in default text mode. Please either keep the original behavior for text mode or call out the change explicitly. 6. **Success JSON emission inside the try (MEDIUM)** — a serialization failure would be misreported as a validation failure and could throw again inside the catch. Please emit the success JSON outside the validation try/catch. 7. **Docs (MEDIUM)** — the `schemaVersion` / `valid` / `phase` / `errors` contract should be documented in `docs/en/engines/zeta/user-command.md`. 8. **Readability (LOW)** — please factor the duplicated failure-JSON emission into one helper and add explicit parentheses to the mixed `&&`/`||` condition. Once the Build has run on the fork and the points above are addressed (or you explain why any of them should stay as-is), I'll do another pass. Note that final approval/dismissal of the earlier requested changes will be done by a write-capable maintainer. <!-- streview-comment:2150 --> -- 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]
