-
Notifications
You must be signed in to change notification settings - Fork 81
Use DefaultAzureCredential by default (#497) #550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 4 commits
fafa0b7
88fd3f9
15e3a5e
ba13261
544094a
741dd25
c320d95
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,7 +49,13 @@ def get(self, key, default=None): | |
|
|
||
|
|
||
| class MockBlobServiceClient: | ||
| def __init__(self, test_dir, adls): | ||
| def __init__(self, test_dir=None, adls=None, account_url=None, credential=None): | ||
| if account_url is not None: | ||
| # account_url-based construction: store url and credential for verification | ||
| self._account_url = account_url | ||
| self._credential = credential | ||
| return | ||
|
Comment on lines
+52
to
+57
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per request, I added the account and credential settings to the mock classes:
But honestly, this doesn't feel right to me. If I understand correctly, the Extending the constructor feels unnatural. For example, we need a conditional block and an early return to avoid running into the Happy to incorporate any feedback from the maintainers on this topic. Also fine with leaving it as-is, if that's the preferred solution. |
||
|
|
||
| # copy test assets for reference in tests without affecting assets | ||
| shutil.copytree(TEST_ASSETS, test_dir, dirs_exist_ok=True) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| from azure.storage.filedatalake import DataLakeServiceClient | ||
| import pytest | ||
|
|
||
| import cloudpathlib.azure.azblobclient | ||
| from urllib.parse import urlparse, parse_qs | ||
| from cloudpathlib import AzureBlobClient, AzureBlobPath | ||
| from cloudpathlib.exceptions import ( | ||
|
|
@@ -19,7 +20,8 @@ | |
| ) | ||
| from cloudpathlib.local import LocalAzureBlobClient, LocalAzureBlobPath | ||
|
|
||
| from .mock_clients.mock_azureblob import MockStorageStreamDownloader | ||
| from .mock_clients.mock_azureblob import MockBlobServiceClient, MockStorageStreamDownloader | ||
| from .mock_clients.mock_adls_gen2 import MockedDataLakeServiceClient | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("path_class", [AzureBlobPath, LocalAzureBlobPath]) | ||
|
|
@@ -39,10 +41,115 @@ def test_azureblobpath_properties(path_class, monkeypatch): | |
| @pytest.mark.parametrize("client_class", [AzureBlobClient, LocalAzureBlobClient]) | ||
| def test_azureblobpath_nocreds(client_class, monkeypatch): | ||
| monkeypatch.delenv("AZURE_STORAGE_CONNECTION_STRING", raising=False) | ||
| monkeypatch.delenv("AZURE_STORAGE_ACCOUNT_URL", raising=False) | ||
| monkeypatch.setattr( | ||
| "cloudpathlib.azure.azblobclient.DefaultAzureCredential", None | ||
| ) | ||
| with pytest.raises(MissingCredentialsError): | ||
| client_class() | ||
|
|
||
|
|
||
| def _mock_azure_clients(monkeypatch): | ||
| """Monkeypatch BlobServiceClient and DataLakeServiceClient with mocks.""" | ||
| monkeypatch.setattr( | ||
| cloudpathlib.azure.azblobclient, "BlobServiceClient", MockBlobServiceClient | ||
| ) | ||
| monkeypatch.setattr( | ||
| cloudpathlib.azure.azblobclient, "DataLakeServiceClient", MockedDataLakeServiceClient | ||
| ) | ||
|
|
||
|
|
||
| def test_default_credential_used_with_account_url(monkeypatch): | ||
| """DefaultAzureCredential is used when account_url is provided without credential.""" | ||
| monkeypatch.delenv("AZURE_STORAGE_CONNECTION_STRING", raising=False) | ||
| monkeypatch.delenv("AZURE_STORAGE_ACCOUNT_URL", raising=False) | ||
| _mock_azure_clients(monkeypatch) | ||
|
|
||
| client = AzureBlobClient(account_url="https://myaccount.blob.core.windows.net") | ||
|
|
||
| assert isinstance(client.service_client, MockBlobServiceClient) | ||
| assert client.service_client._account_url == "https://myaccount.blob.core.windows.net" | ||
| assert isinstance(client.service_client._credential, DefaultAzureCredential) | ||
|
|
||
| assert isinstance(client.data_lake_client, MockedDataLakeServiceClient) | ||
| assert client.data_lake_client._account_url == "https://myaccount.dfs.core.windows.net" | ||
| assert isinstance(client.data_lake_client._credential, DefaultAzureCredential) | ||
|
|
||
|
|
||
| def test_no_default_credential_when_explicit_credential(monkeypatch): | ||
| """DefaultAzureCredential is NOT used when an explicit credential is provided.""" | ||
| monkeypatch.delenv("AZURE_STORAGE_CONNECTION_STRING", raising=False) | ||
| monkeypatch.delenv("AZURE_STORAGE_ACCOUNT_URL", raising=False) | ||
| _mock_azure_clients(monkeypatch) | ||
|
|
||
| explicit_cred = "my-explicit-credential" | ||
| client = AzureBlobClient( | ||
| account_url="https://myaccount.blob.core.windows.net", | ||
| credential=explicit_cred, | ||
| ) | ||
|
|
||
| assert client.service_client._credential == explicit_cred | ||
| assert not isinstance(client.service_client._credential, DefaultAzureCredential) | ||
|
|
||
|
|
||
| def test_fallback_when_azure_identity_not_installed(monkeypatch): | ||
| """When azure-identity is not installed, credential=None is passed through.""" | ||
| monkeypatch.delenv("AZURE_STORAGE_CONNECTION_STRING", raising=False) | ||
| monkeypatch.delenv("AZURE_STORAGE_ACCOUNT_URL", raising=False) | ||
| monkeypatch.setattr( | ||
| cloudpathlib.azure.azblobclient, "DefaultAzureCredential", None | ||
| ) | ||
| _mock_azure_clients(monkeypatch) | ||
|
|
||
| client = AzureBlobClient(account_url="https://myaccount.blob.core.windows.net") | ||
|
|
||
| assert client.service_client._credential is None | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can remove per comments above
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've addressed this comment. Feel free to resolve. |
||
|
|
||
|
|
||
| def test_account_url_env_var_blob(monkeypatch): | ||
| """AZURE_STORAGE_ACCOUNT_URL env var with .blob. URL creates both clients.""" | ||
| monkeypatch.delenv("AZURE_STORAGE_CONNECTION_STRING", raising=False) | ||
| monkeypatch.setenv( | ||
| "AZURE_STORAGE_ACCOUNT_URL", "https://myaccount.blob.core.windows.net" | ||
| ) | ||
| _mock_azure_clients(monkeypatch) | ||
|
|
||
| client = AzureBlobClient() | ||
|
|
||
| assert isinstance(client.service_client, MockBlobServiceClient) | ||
| assert client.service_client._account_url == "https://myaccount.blob.core.windows.net" | ||
| assert isinstance(client.service_client._credential, DefaultAzureCredential) | ||
|
|
||
| assert isinstance(client.data_lake_client, MockedDataLakeServiceClient) | ||
| assert client.data_lake_client._account_url == "https://myaccount.dfs.core.windows.net" | ||
| assert isinstance(client.data_lake_client._credential, DefaultAzureCredential) | ||
|
|
||
|
|
||
| def test_account_url_env_var_dfs(monkeypatch): | ||
| """AZURE_STORAGE_ACCOUNT_URL env var with .dfs. URL creates both clients.""" | ||
| monkeypatch.delenv("AZURE_STORAGE_CONNECTION_STRING", raising=False) | ||
| monkeypatch.setenv( | ||
| "AZURE_STORAGE_ACCOUNT_URL", "https://myaccount.dfs.core.windows.net" | ||
| ) | ||
| _mock_azure_clients(monkeypatch) | ||
|
|
||
| client = AzureBlobClient() | ||
|
|
||
| assert client.service_client._account_url == "https://myaccount.blob.core.windows.net" | ||
| assert client.data_lake_client._account_url == "https://myaccount.dfs.core.windows.net" | ||
|
|
||
|
|
||
| def test_missing_creds_error_no_env_vars(monkeypatch): | ||
| """MissingCredentialsError is still raised when nothing is configured.""" | ||
| monkeypatch.delenv("AZURE_STORAGE_CONNECTION_STRING", raising=False) | ||
| monkeypatch.delenv("AZURE_STORAGE_ACCOUNT_URL", raising=False) | ||
| monkeypatch.setattr( | ||
| cloudpathlib.azure.azblobclient, "DefaultAzureCredential", None | ||
| ) | ||
| with pytest.raises(MissingCredentialsError): | ||
| AzureBlobClient() | ||
|
|
||
|
|
||
| def test_as_url(azure_rigs): | ||
| p: AzureBlobPath = azure_rigs.create_cloud_path("dir_0/file0_0.txt") | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow existing convention and add to import block of azure dependencies above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
azure.identityimport was intentionally left out of the big import block above, asazure-identityis an optional azure dependency, meaning you can use the azure blob client without it (you will just not be able to use identity-based authentication).I've left a comment to make this clearer. Let me know if you prefer a different approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put it in with the other dependencies, we don't want multiple dependency fallback paths. Either a user has all the backend dependencies, or they don't, which is how you implemented it in the pyproject.toml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me! I've addressed this comment. Feel free to resolve.