SEZ9 commented on PR #12944: URL: https://github.com/apache/seatunnel/pull/12944#issuecomment-6513324268
Thanks for the update, @Nikk8091. Dropping the two assertions that only rechecked the `datasource` fixture in `b3904d55` sounds like the right direction for the tautological-assertion point; I'll confirm it against the diff once I can build the branch. Appreciate the candor on the compile state. Since compilation still stops at the unresolved `ConfigMapPathUtils` import before tests run, the test changes remain unverified for now, so the standalone-compilation finding stays open. Your plan — update the base once the prerequisite lands, confirm standalone compilation, and rerun `MetadataProviderManagerTest` and `MetalakeIT.testDataSource` in CI — is what I'd want to see before closing it out. It would also help to note in the PR description which prerequisite revision you end up building against. Noted that the unwrapped()/parseMap short-circuit will be a separate follow-up; a link here when it exists is fine. Remaining items from my earlier pass that, as far as I can tell from your description, `b3904d55` doesn't address: - The key-quoting and `->` path-expansion contract for provider-supplied keys still needs to be documented on `mergeConfig` and in the Metadata SPI doc, since the new tests lock that behavior in. - The Metadata SPI concepts doc still says job-level parameters take precedence, while the test asserts datasource-over-job. One of the two needs to change — let me know which direction you intend. Happy to re-review once the rebase is up. <!-- streview-comment:2130 --> -- 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]
