SEZ9 commented on PR #11496:
URL: https://github.com/apache/seatunnel/pull/11496#issuecomment-5738780586
Thanks @NganWave for the detailed status update. Quick recap so nothing gets
lost before the next push:
**Addressed (I'll re-check once the next commit lands):**
- **F2** – replacing `withFallback(cleanSourceConfig)` with
`processVariablesMap` plus `originalResolvedConfigMap.keySet().removeIf(key ->
!originalRootKeys.contains(key))` is the right shape for keeping `-i` values
out of the final config. A small unit test asserting that a `-i` key which is
not a root key of the job config does not appear in the resolved output would
lock this in.
- **F6 / F8** – good to hear the fail-fast on unbalanced `{`/`[` and
unterminated quotes is back with unit tests. I'll verify the
quote-followed-by-non-delimiter case is covered when I re-review.
**F4 / F5** – I'm fine with no longer exporting `-i` variables as JVM system
properties; avoiding cross-job contamination is a legitimate reason, and as you
note `config.md` never documented that behaviour. The one ask is a short note
in the PR description saying that `-i` values are no longer visible via
`System.getProperties()`, so anyone who relied on it isn't surprised.
**Still open for the next push:**
- **F1** – `backfillUserVariables` should keep last-wins for duplicate `-i`
keys and must not NPE on a null parsed value. Please add tests for both the
duplicate-key and null-value cases.
- **F3** – `PLACEHOLDER_PATTERN` needs to accept `[` / `]` in defaults so
`${key:[a,b]}` keeps resolving as it did with the old `ConfigBuilder` regex; a
test with a list default would be great.
- **F7** – a changelog/upgrade note describing the new `ParameterSplitter`
tokenization for `-i` values containing braces or interior quotes.
- The two High-severity issues already raised in this thread as introduced
by `0318a671b4`: the `processVariable` changed-value check comparing a `String`
to the original `Object` (rewriting non-`String` scalar leaves whenever any
`-i` variable is supplied), and the dropped recursive
`processVariablesMap(...)` call in the `Map` branch of `processVariablesList`
(placeholders nested in a map-in-a-list no longer substituted). If "the new
issue in the latest commit" refers to these, please fold them into the same
push with the regression tests mentioned above; if it's something different,
let us know so it's tracked too.
Once that commit is up with the tests and docs, I'll do a full fresh pass
over the whole diff.
<!-- streview-comment:1162 -->
--
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]
