SEZ9 commented on PR #12443: URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-6105489494
Thanks for the continued iteration. Following up on my earlier points against the current head `871c6448a8d1`: - **F1 (decryptConfig undoing the fix) / F5 (fix private to RestUtil)** – I understand the quoting now lives in a shared `ConfigMapPathUtils` helper and is applied at the `Map` → `Config` conversion points in `ConfigBuilder`, `ConfigShadeUtils` and `ReadonlyConfig`, which would cover both the shade/decrypt hop and the `ReadonlyConfig.toConfig()` hop. Could you confirm that is the final shape, and whether file-loaded configs through `ConfigAdapter` are intentionally out of scope now (and why)? A short note in the PR description would be enough. - **F2 (docs)** – I see EN/ZH REST docs were updated; please confirm they describe the new key semantics (reserved HOCON characters now literal, `->` still a path separator). - **F3 / F4 (tests)** – good to see REST-level and shade-decrypt tests added. Could you point me to the cases covering (a) an escape round-trip for a key containing `"` or `\`, (b) a top-level key that hits the quoting branch, and (c) `->`, reserved tokens and padded keys? If any are not covered yet, please add them. - **F6** – same as above: please confirm the new tests actually exercise the quoting branch rather than only keys the parser already accepts. - **F7 (title)** – since the change now spans more than the engine server module, I'm withdrawing the `[Fix][Zeta]` ask. Once the above is confirmed I'm happy to approve. Thanks! <!-- streview-comment:1681 --> -- 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]
