junjunclub commented on PR #11186: URL: https://github.com/apache/seatunnel/pull/11186#issuecomment-4911529337
Hi @SEZ9, thank you for the thorough review — this was very helpful. I want to clarify that most of the issues you flagged were not introduced by this PR's actual changes. The branch had accidentally diverged from an outdated dev base and pulled in ~129 commits / 654 changed files from upstream and unrelated work. That polluted diff included .asf.yaml governance edits, .github/workflows/backend.yml CI churn, .skills/ files, connector doc rewrites, Helm install script changes, etc. — none of which are part of my intended changes. I've cleaned up the branch by rebasing onto the latest dev and force-pushing. The PR now contains 2 commits and 16 files, scoped to the connect dry-run feature only: 85a073f45 feat: layer1 with jdbc connector 9dfebf63b [Feature][Core] Add connect dry-run validation What is actually in the current diff: Core: DryRunConnectValidator, SeaTunnelConfValidateCommand, ClientCommandArgs API SPI: SupportSourceDryRunValidation, SupportSinkDryRunValidation Connectors: JDBC source/sink factory hooks, FakeSource, DuckDB dry-run test Tests: SeaTunnelConfValidateCommandTest, ClientCommandArgsTest, dry-run test factories Docs: docs/en/engines/zeta/user-command.md, docs/zh/engines/zeta/user-command.md Mapping to your review items: Flink/Spark e2e modules dropped from CI — Not in this PR (.github/workflows/backend.yml is unchanged) Core feature not reviewable in diff — Resolved (DryRunConnectValidator and SPI hooks are now clearly visible) Helm curl | bash supply-chain regression — Not in this PR 4 & 9. .asf.yaml collaborator / allow_update_branch — Not in this PR Mass CI action version bumps — Not in this PR .skills/ files without ASF headers — Not in this PR Missing docs for --dry-run connect — Partially addressed in docs/en/engines/zeta/user-command.md (and zh counterpart), covering --dry-run connect semantics, connector support matrix, and VALIDATED / SKIPPED behavior Credential masking in dry-run error output — Valid concern for the actual code; happy to address in a follow-up commit if you'd like centralized sanitization in DryRunConnectValidator Could you please re-review the updated diff? The force-pushed branch should now show only the dry-run connect feature. I'm happy to address any remaining feedback on the actual 16-file scope — especially around credential sanitization (#5) and any gaps in the docs. Sorry for the noise in the earlier diff; it was a git base issue on my side, not intentional scope creep. -- 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]
