DanielLeens commented on PR #12293: URL: https://github.com/apache/seatunnel/pull/12293#issuecomment-6574856858
Thanks @SEZ9. This list matches the F1-F8 set from earlier rounds, and the head has not moved: it is still `3d3c1b7cd75894f04b803edd90b6241db2221112`, the head my latest full review covered. So I can map each item to the current code instead of asking @sudeephazra to repeat anything. This is code reading only; nothing was executed locally. - **F1 (normalized values):** `ADLSHadoopConf.buildWithReadOnlyConfig` (`ADLSHadoopConf.java:59-94`) takes `account`, `container`, `suffix`, the credentials, `authority_host` and `tenant_id` from `ADLSConfigValidator.required(...)`, and the advanced map is written as `options.put(key.trim(), value.trim())`. The routing/auth values are not re-read raw. Coverage is in `ADLSHadoopConfTest` and `ADLSConfigValidatorTest`, as the author listed. - **F2 (HNS):** `docs/en/connectors/sink/ADLSFile.md:41-42` states that HNS is needed for transactional commit and exactly-once, because the commit renames staged files into `path`. - **F3 (validation rules):** the naming, auth mutual-exclusion (`ADLSFile.md:106-108`) and blocked-property rules are documented; the template placeholders were reworked at `config/v2.batch.adls.config.template:39`. - **F4 (OAuth endpoint):** the HTTPS and tenant checks run in `ADLSRuntimeCompatibility` before the endpoint is built. On the "client_secret goes to the configured host" concern: `ADLSFile.md:111-114` says `authority_host` is a trust boundary, that validation checks syntax only, and that only trusted operators should set it. That is the sentence you asked for, and it lives in the user-facing reference. - **F5 (option tables):** en/zh sink tables were aligned with the factory options (English sink table around `:61-66`). - **F6 (global masking):** `ADLSFile.md:138-139` documents that `account_key` log masking is global and is not encryption; the GitHub PR description was updated as well. - **F7 (source defaults):** English source doc now says `ONCE` / `EARLIEST` (`:74`, `:76`). - **F8 (denylist):** the denylist was expanded in `ADLSConfigValidator`, and the docs (`ADLSFile.md:135-136`) now say the map is trusted-operator input and not an exhaustive filter. Please do verify these against the diff as you planned. If any item still looks open to you at this head, quote the line so @sudeephazra gets one concrete ask. The one open source blocker is not in F1-F8, and it is still Issue 1 from my review (https://github.com/apache/seatunnel/pull/12293#pullrequestreview-5728340249): the duplicate-key negative case `AzuriteWasbFileIT.java:181` expects the validator's duplicate-key message, but the starter reparses the config (`ConfigBuilder.java:261`) and the padded keys have already collapsed by the time the connector validator runs. That needs a test-boundary change; it is not a connector production change. CI at this head: the apache-side rollup currently reports no checks, and the exact-head fork Build run (https://github.com/sudeephazra/seatunnel-adls-connector-v2/actions/runs/41288883572) was cancelled, so there is no passing CI evidence yet. Compare shows 1 commit behind `dev`, which I am not asking for a sync over by itself. -- 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]
