DanielLeens commented on PR #11496:
URL: https://github.com/apache/seatunnel/pull/11496#issuecomment-5726066646
Thanks @NganWave for the detailed status update, and @SEZ9 for consolidating
the remaining asks.
The head is still unchanged since my last review (`00d5018647`), so this is
an acknowledgment rather than a fresh pass -- I'll do the full re-review once
the promised commit lands and CI has run on it.
One thing worth making explicit before that push happens: my own 2026-09-16
review found two open High-severity issues, both introduced by `0318a671b4`
(not carryovers from earlier rounds):
1. `processVariable`'s changed-value check compares a `String` to the
original, un-stringified `Object` (`ConfigBuilder.java:387-412`), so every
non-`String` scalar leaf in the whole resolved config silently gets rewritten
to a `String` whenever any `-i` variable is supplied at all -- not just leaves
that actually contain a placeholder.
2. The recursive `processVariablesMap(...)` call was dropped from the `Map`
branch of `processVariablesList` (`ConfigBuilder.java:376-377`), so `${...}`
placeholders nested inside a map-in-a-list structure are silently no longer
substituted.
@NganWave, when you mention "the new issue in the latest commit" in your
last comment, I want to confirm that's referring to these two -- if so, please
fold them into the same push as F1/F3/F7, along with the regression tests we
discussed (a non-String scalar leaf test for Issue 1, a
placeholder-inside-map-inside-list test for Issue 2). If it's a different
issue, let me know so nothing gets lost in the meantime.
Once that lands, I'll do a complete fresh review of the full diff.
--
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]
