DanielLeens commented on PR #12443: URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-6181438602
Thanks @SEZ9 and @hutiefang76 for closing the loop on the last open questions. I re-checked head `871c6448a8d1` (unchanged) against the new asks and also against the current `dev`. **Answers to the latest open asks (all verifiable on the current head):** - F1 / F5: the guard sits at `ConfigBuilder.java:110`, `ConfigShadeUtils.java:229` and `ReadonlyConfig.java:79`, all through `ConfigMapPathUtils.quoteInvalidPathKey`, so it covers the REST Map to Config hop and the shade/decrypt hop. The description now lists ConfigAdapter, `MetadataProviderManager` and `AssertSink` as out of scope. - F2 / F3: the EN/ZH wording is in `rest-api-v2.md` (lines 921-922 in EN) and states that quoted keys stay literal only when the path parser rejects them. - F4 / F6: `ConfigBuilderTest.java:34,61` covers `->` nesting and padded keys; the escape round trip and the top-level quoted key are `RestUtilTest` and `RestApiSubmitJobConfigShadeDecryptTest`; the REST-level cases are `RestApiIT#testSubmitJobWithHoconFormatAndQuotedLiteralKey` and `#testSubmitJobWithJsonFormat`. All of them exercise the quoting branch. - F7: the title is `[Fix][Core]`, which matches the changed modules (core, api and engine server), and @SEZ9's earlier comment already withdrew the `[Fix][Zeta]` ask. **New merge-state fact (not a code issue in your change):** GitHub now reports this PR as `dirty` (merge conflicts), 9 ahead / 117 behind `dev`. The conflict was introduced by #12491 (JSON params via CLI, merged today), which rewrote `ConfigBuilder.backfillUserVariables` and added cases to `ConfigBuilderTest`. A local merge-tree shows two conflict hunks, both in `ConfigBuilderTest.java` (the other files merge textually, but please re-run the new tests after the merge since #12491 changed `ConfigBuilder` around the same area). @hutiefang76, could you please sync with the latest `dev`, resolve the conflicts and push? Since the head has to change, CI will also restart on the merged code, which should clear the queued/failed runner jobs as well. I have no source-level blocker from my side. As before, I do not have write-level review controls, so the final approval has to come from a committer. -- 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]
