DanielLeens commented on PR #11817: URL: https://github.com/apache/seatunnel/pull/11817#issuecomment-5304921369
Thanks both for the follow-up — this is exactly the kind of back-and-forth that makes a PR better. @abolfazlmadanii, holding off on extending the port-range validation until a maintainer confirms scope is the right call, and I agree the title should describe what the diff actually does (adds a new SMTP-port constraint plus registers the two missing options) rather than "migrate," since "migrate" implies a pre-existing check just moved location, and for `email_smtp_port` there wasn't one. That's a small, easy fix once scope is settled — no need to block on it separately. @goutamadwant, good catch on the test gap, and I can confirm it from the same angle: `EmailFactoryTest` only exercises `ConfigValidator.validate()`, which checks declared constraints (`Conditions`, ranges, etc.) but not "does the schema even know about this key" — that's `validateUnknownKeys()`'s job, and it's wired into the real CLI path (`ConfigValidationUtils`/`SeaTunnelConfValidateCommand`) but not into this test. So today the test suite would stay green even if `EMAIL_ATTACHMENT_NAME`/`EMAIL_FIELD_DELIMITER` were removed from `optionRule()` again in some future change — which is precisely the class of regression this PR is fixing. Adding a `validateUnknownKeys()` call (or a small helper that runs both) to `EmailFactoryTest` would turn that from "fixed by inspection" into "protected by a test," and I'd treat that as effectively closing my own default-value-test suggestion from the original review too, since a well-placed `validateUnknownKeys()` assertion covering all three options is more direct than testing each option's default individually. Current status, to keep this from going in circles: the open items are (1) the scope question on the SMTP-port range/comment work — waiting on a maintainer, (2) the title/description update once scope is settled, and (3) the `validateUnknownKeys()` test-coverage gap @goutamadwant raised. Nothing here changes my read that the core option-registration fix is correct and the root cause you found is real — happy to take another look as soon as you've had a chance to act on whichever of these lands first. -- 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]
