SEZ9 commented on PR #12293: URL: https://github.com/apache/seatunnel/pull/12293#issuecomment-6009146455
Thanks @sudeephazra for splitting the follow-ups into fragments — they all came through. On the points you addressed at cbe10d8098f001db5a50dd321a68a4df67419828: - **F1 (raw vs. trimmed values):** Routing `ADLSHadoopConf` through the validator's trimming helper and `required()`, plus a test that pushes whitespace-padded values through `buildWithReadOnlyConfig()` and asserts the URI / OAuth endpoint / client ID / secret come out clean, is the shape of fix I was looking for. I'll confirm against the diff. - **F2 (HNS requirement):** A sink prerequisite section covering HNS for transactional commit, the partial-output risk without it, and the same-container constraint for `tmp_path` / `path` addresses the gap as described. Will verify in the diff. - **F3 (undocumented validation rules / template):** Documenting account/container naming, auth mutual exclusion, and blocked Hadoop property keys in the sink and source docs (en + zh) and fixing the template placeholders covers what I raised. Will verify in the diff. - **F4 (OAuth endpoint construction):** Validating `tenant_id` (GUID or DNS name) and requiring an HTTPS authority with a valid host and no user-info/path/query/fragment closes the plain-HTTP part. As you note, there's no trusted-host allowlist, so the secret can still be POSTed to any HTTPS host the config names. I'm fine keeping custom authority hosts for sovereign clouds, but please make the `authority_host` docs state explicitly that any HTTPS host is accepted so users understand the trust boundary. I'll keep this open until I've checked the diff and that doc note. Still pending from the earlier review: - **F5** – option tables missing factory-exposed options and en/zh sink table drift in `docs/en/connectors/sink/ADLSFile.md`. - **F6** – the `ConfigShadeUtils` change adding `account_key` to the default log-mask list should be mentioned in the docs and the PR description, since it affects every connector. - **F7** – `docs/en/connectors/source/ADLSFile.md` defaults for `discovery_mode` / `start_mode` should match the code (`ONCE` / `EARLIEST`) and the Chinese doc. - **F8** – the `hadoop_adls_properties` prefix denylist in `ADLSConfigValidator` still admits class-loading / token-provider ABFS keys, which conflicts with the "non-routing, non-credential" contract on the option. Could you either point me at where F5–F8 are handled in the current revision or push follow-ups? Once those are in I'll do a final pass over the full diff. <!-- streview-comment:1555 --> -- 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]
