potiuk commented on code in PR #68344:
URL: https://github.com/apache/airflow/pull/68344#discussion_r3678127009
##########
providers/microsoft/azure/newsfragments/68344.bugfix.rst:
##########
@@ -0,0 +1 @@
+Fix ADLS ``ObjectStoragePath`` connection parsing to pass ``account_name`` for
Active Directory ``adls`` connections while preserving the existing
``account_url`` behavior for WASB, full-URL, and custom-endpoint connections
Review Comment:
Please remove this file — `newsfragments/` doesn't apply to providers.
Provider release managers regenerate the changelog from `git log`, and only
`airflow-core/`, `chart/` and `dev/mypy/` have a towncrier config to consume
newsfragments, so nothing will ever pick this up. (There's one stray
`51944.bugfix.rst` in this same directory on `main` — that slipped through
rather than establishing a convention; it shouldn't be there either.)
If you want a user-visible note for this fix, add it directly to
`providers/microsoft/azure/docs/changelog.rst` instead.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/fs/adls.py:
##########
@@ -29,6 +30,25 @@
schemes = ["abfs", "abfss", "adl"]
+def _account_name_from_host(host: str | None) -> str | None:
Review Comment:
As far as I can see in this diff, `_account_name_from_host` has exactly one
caller — `_storage_account_name_from_host` — and that caller only returns the
value when `account_name == host`. That condition can never hold when `host`
contains a dot, so the `hostname.split(".", 1)[0]` here can never produce a
value that's actually used.
The two helpers collapse into one that answers the real question: "is this
host a bare account name?" Something like a single function returning `hostname
if hostname == host.lower() else None` would say the same thing with half the
surface.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/fs/adls.py:
##########
@@ -47,12 +67,17 @@ def get_fs(conn_id: str | None, storage_options: dict[str,
Any] | None = None) -
if connection_string:
return AzureBlobFileSystem(connection_string=connection_string)
- options: dict[str, Any] = {
- "account_url": parse_blob_account_url(conn.host, conn.login),
- }
-
# mirror handling of custom field "client_secret_auth_config" from extras.
Ignore if missing as AzureBlobFileSystem can handle.
tenant_id = get_field(conn_id=conn_id, conn_type=conn_type, extras=extras,
field_name="tenant_id")
+ options: dict[str, Any] = {}
+ account_name = _storage_account_name_from_host(conn.host)
+ # Preserve account_url for existing WASB, full URL, and custom endpoint
connections.
+ # ADLS Active Directory connections use login as client_id, so bare hosts
are account names.
+ if conn_type == "adls" and account_name:
+ options["account_name"] = account_name
+ elif conn_type != "adls" or conn.host or not tenant_id:
Review Comment:
`elif conn_type != "adls" or conn.host or not tenant_id:` packs three
independent reasons into one condition, and the case it deliberately leaves out
— `adls` + no host + a tenant_id, where *neither* `account_name` nor
`account_url` is set so adlfs falls back to `DefaultAzureCredential` — is
invisible unless you work the truth table by hand.
Worth either splitting into explicit branches with a short comment on the
fall-through case, or adding a test that pins it, since it's the one
combination with no key set at all.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/fs/adls.py:
##########
@@ -29,6 +30,25 @@
schemes = ["abfs", "abfss", "adl"]
+def _account_name_from_host(host: str | None) -> str | None:
+ if not host:
+ return None
+
+ parsed_url = urlparse(host if "://" in host else f"//{host}")
+ hostname = parsed_url.hostname
+ if not hostname:
+ return None
+
+ return hostname.split(".", 1)[0]
+
+
+def _storage_account_name_from_host(host: str | None) -> str | None:
+ account_name = _account_name_from_host(host)
+ if account_name and account_name == host:
Review Comment:
`urlparse(...).hostname` lowercases, so this equality is case-sensitive in a
surprising direction:
```python
>>> urlparse("//MyAccount").hostname
'myaccount'
```
With `host="MyAccount"` the comparison is `"myaccount" == "MyAccount"` ->
`False`, so the connection silently falls through to the `account_url` branch
instead of `account_name` — i.e. the bug this PR fixes still reproduces purely
because of host casing. Azure storage account names are lowercase by spec, so
it's an edge, but it's the kind that produces a confusing report later.
Comparing `account_name == host.lower()` (or normalising `host` once up
front) would make it case-insensitive.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
