kaxil commented on PR #73374: URL: https://github.com/apache/airflow/pull/73374#issuecomment-5778890621
Round 2 at 016a9bc. The three code comments from the first round are addressed: the account now comes from `host` first, the Azure error message uses `.value`, and the `urllib.parse` import is at module level. Keeping `abfs://` and `abfss://` for a separate PR is fine. Four new items, all measured against the real datafusion binding and object_store source: - The worker's `AZURE_*` environment is read by `from_env()` before the connection's credential is overlaid, and object_store checks an environment access key or workload identity before a SAS token or client secret. With `AZURE_STORAGE_ACCOUNT_KEY` set, or the AKS workload-identity variables set, the connection's SAS or service principal is never used. The docs currently say ambient credentials come last. - `_resolve_wasb_account` keeps only the first host label and the binding has no endpoint parameter, so sovereign-cloud, private-endpoint and Azurite hosts are silently redirected to `*.blob.core.windows.net`. - A partial service principal (`tenant_id` with `login` or `password` missing) falls through to ambient auth or forwards the client secret as a shared key instead of raising. - With neither `host` nor `login` set, the account becomes the string `"None"`, where the previous code let the binding fall back to `AZURE_STORAGE_ACCOUNT_NAME`. That is the shape of the default `wasb_default` connection the example DAG uses. Inline comments have the details and suggested fixes. Verdict: hold until these are addressed. -- 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]
