Chinmay1220 commented on code in PR #68344:
URL: https://github.com/apache/airflow/pull/68344#discussion_r4512381675
##########
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:
+ options["account_url"] = parse_blob_account_url(conn.host, conn.login)
Review Comment:
Addressed in 0282cb2: `adls.rst` now says Host is the bare storage account
name, and points private endpoints and custom domains at the `account_host`
field instead of a full URL. So the docs no longer lead to the dropped
`account_url` path.
##########
providers/microsoft/azure/tests/unit/microsoft/azure/fs/test_adls.py:
##########
@@ -141,6 +139,126 @@ def mocked_blob_file_system():
"account_key": "password",
},
),
+ (
+ Connection(
+ conn_id="testconn",
+ conn_type="adls",
+ host="testaccountname",
+ login="client_id",
+ password="client_secret",
+ extra={
+ "tenant_id": "tenant_id",
+ },
+ ),
+ {
+ "account_name": "testaccountname",
+ "client_id": "client_id",
+ "client_secret": "client_secret",
+ "tenant_id": "tenant_id",
+ },
+ ),
+ (
+ # A mixed-case bare host is still an account name; urlparse
lowercases it.
+ Connection(
+ conn_id="testconn",
+ conn_type="adls",
+ host="MyAccount",
+ login="client_id",
+ password="client_secret",
+ extra={
+ "tenant_id": "tenant_id",
+ },
+ ),
+ {
+ "account_name": "myaccount",
+ "client_id": "client_id",
+ "client_secret": "client_secret",
+ "tenant_id": "tenant_id",
+ },
+ ),
+ (
+ Connection(
+ conn_id="testconn",
+ conn_type="adls",
+ host="testaccountname",
+ password="account_key",
+ extra={},
+ ),
+ {
+ "account_name": "testaccountname",
+ "account_key": "account_key",
+ },
+ ),
+ (
+ Connection(
+ conn_id="testconn",
+ conn_type="adls",
+ host="testaccountname.dfs.core.windows.net",
+ login="client_id",
+ password="client_secret",
+ extra={
+ "tenant_id": "tenant_id",
+ },
+ ),
+ {
+ "account_url": "https://testaccountname.dfs.core.windows.net";,
+ "client_id": "client_id",
+ "client_secret": "client_secret",
+ "tenant_id": "tenant_id",
+ },
+ ),
+ (
+ Connection(
+ conn_id="testconn",
+ conn_type="adls",
+ host="https://custom.blob.core.mydomain.io";,
+ login="client_id",
+ password="client_secret",
+ extra={
+ "tenant_id": "tenant_id",
+ },
+ ),
+ {
+ "account_url": "https://custom.blob.core.mydomain.io";,
Review Comment:
Done in 5e829e2: this is now an `account_host` case (bare account name in
`host`, custom domain in `extra.account_host`) that asserts `account_name` +
`account_host`. The full DFS endpoint case above it had the same problem, so
0cdc7cf replaces it with a direct test of `_bare_account_name_from_host`. Happy
to change it if you'd prefer another approach.
--
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]
