SEZ9 commented on PR #11496:
URL: https://github.com/apache/seatunnel/pull/11496#issuecomment-5552598972
Thanks for the update, @NganWave — taking the time to properly design the
JSON/array default-value support instead of pushing a partial fix is the right
call, and I'm glad the validation is being restored. When you do, please make
sure the `ParameterSplitter` path fails fast (or at least warns) on unbalanced
`{`/`[` or an unterminated quote rather than silently swallowing later splits.
In addition to the checklist already posted above, a few previously raised
points to cover in the rework:
1. Duplicate `-i` keys should not crash in `ConfigBuilder` — the old
behavior was last-wins, so please preserve that or document the intentional
change.
2. `-i` variables are no longer exported via `System.setProperty`, which
affects anything downstream resolving against `System.getProperties()`. If
dropping that side effect is intentional, please call it out explicitly as a
behavior change.
3. `withFallback(cleanSourceConfig)` merges `-i` values as top-level keys
into the final job config, risking secret exposure in rendered/logged config —
please scope the fallback or explain why the exposure is acceptable.
4. Existing `-i` values containing braces or interior quotes now tokenize
differently; a short upgrade/changelog note would help users migrating.
No rush — take the time you need, and ping me once it's ready for review
again with the new commit; I'll do a fresh full pass then.
<!-- streview-comment:795 -->
--
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]
