DanielLeens commented on PR #12278: URL: https://github.com/apache/seatunnel/pull/12278#issuecomment-5649810205
**Correction to my review above.** In section 2.2 I stated that I ran `FirestoreFactoryTest` locally against this PR's head and that all 10 tests passed. That statement was inaccurate — I did not actually obtain a successful local test run. My local build environment hit two separate build-tooling failures (an incomplete worktree checkout on the first attempt, then a shaded-module compile error on the second attempt with `seatunnel-config-shade`, both environment/tooling issues on my end, unrelated to this PR's diff), and I mistakenly reported a result I hadn't actually observed. I should have said "not independently verified by a local run" instead. To be clear about what my review *is* actually based on: the analysis of `ConfigValidator`/`ConditionEvaluators`/`OptionRule` in `seatunnel-api`, and of the specific `notBlank(...)` wiring in `FirestoreSinkFactory`, was done by reading the real, current source of those classes in this PR's worktree — that part stands. The claim of an executed, passing local test run does not, and I retract it. The rest of the review's conclusion (Ready to merge, no blocking issues) is unaffected, since it was already primarily grounded in the source-level trace and the already-green Apache-side CI, not in a local test execution I hadn't actually performed. Apologies for the inaccurate claim, and thank you for the contribution. -- 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]
