SEZ9 commented on PR #12443: URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-6206702823
Thanks @hutiefang76 for syncing `dev` in bdcfccdb1 and keeping both sets of `ConfigBuilderTest` cases rather than dropping either side. On the previously raised points, here is where I see things at this head: - **F1 / F5 (decryptConfig re-parse, shared root cause)**: the guard now sits at the three `Map` → `Config` hops (`ConfigBuilder.java:111`, `ConfigShadeUtils.java:229`, `ReadonlyConfig.java:79`) via `ConfigMapPathUtils.quoteInvalidPathKeys`, so the shade/decrypt re-parse no longer undoes the fix and the behaviour is shared instead of private to `RestUtil`. Both considered resolved. - **F2 (docs)**: the EN/ZH semantics (reserved HOCON characters literal, `->` still a path separator) are documented and unchanged by the merge. Resolved. - **F3 (`->` still splits)**: given the documented contract above, treating `->` as an intentional path separator is acceptable to me; no further change requested. - **F6 / F7**: fine as-is from my side. - **F4 (test coverage on the real submit path)**: this is the one open item. You note the 46 selected configuration/REST tests and root verify passed on the merged tree, but the embedded REST integration run has not been repeated on bdcfccdb1. Since the merge touched the test tree, please re-run that REST integration pass on the current head and post the result (or the command used) in the description, so the regex-key submit path is confirmed on exactly what will be merged. Once that run is reported, I have nothing else outstanding in this review scope. <!-- streview-comment:1784 --> -- 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]
