Skip to content
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

support container client and blob client for azure blob storage #652

Merged
merged 7 commits into from Oct 10, 2021

Conversation

cbare
Copy link
Contributor

@cbare cbare commented Sep 9, 2021

Support ContainerClient and BlobClient in Azure storage

Support use of ContainerClient and BlobClient to access blobs in Azure storage in addition to existing support for BlobServiceClient. Overloads transport_params['client'] to accept any of the three types of clients supported by Azure storage, for example:

transport_params = {
    'client': ContainerClient.from_container_url(container_sas_url),
}

with smart_open.open('azure://path/to/blob', mode='wb', transport_parameters=transport_parameters) as fout:
   fout.write(data)

Motivation

This allows for a least-privilege approach. You can access azure storage with a SAS URL scoped down to a specific container or blob, rather having the full run of the storage account.

- Fixes #651 

Tests

in smart_open/tests/test_azure.py

  • test_read_container_client
  • test_read_blob_client
  • test_write_container_client

@cbare
Copy link
Contributor Author

cbare commented Sep 9, 2021

Ah flake8, my old friend. Apparently, flake8 takes offense at long lines caused by the type annotations. I added a type alias to resolve that. This introduces the first use of import from typing and modern type annotation syntax, so far as I could tell. Does that impact support for older versions of Python?

@cbare
Copy link
Contributor Author

cbare commented Sep 16, 2021

@mpenkov mind approving the CICD? Let me know what you think of the way I did the type annotations. I can change if a different way is better. Thanks!

@mpenkov
Copy link
Collaborator

mpenkov commented Sep 16, 2021

Sorry, I've been busy with gensim and other projects this week. I'll try to look at this more closely next week. Thank you for your efforts and your patience!

@cbare
Copy link
Contributor Author

cbare commented Sep 21, 2021

I don't understand why that test is failing with this NameError during register_transport('smart_open.azure'). Somehow importlib doesn't like my type alias, apparently.

smart_open/azure.py:46: in <module>
    azure.storage.blob.BlobServiceClient,
E   NameError: name 'azure' is not defined

Going back to using the Union in "# type:" comments, plus tagging those lines with "# noqa" seems to fix the above error and also appease flake8. The noqa bit might cause a type-checker some grief, if one where to be run, but Visual Code seems not to mind.

@mpenkov
Copy link
Collaborator

mpenkov commented Sep 28, 2021

fixes #651

@mpenkov
Copy link
Collaborator

mpenkov commented Sep 28, 2021

Somehow importlib doesn't like my type alias, apparently.

Could be because of relative import (azure.blah vs smart_open.azure.blah) but I can't be sure atm. We don't have explicit type annotations right now (but we may in the future) so your type annotations are fine as comments for now.

@mpenkov
Copy link
Collaborator

mpenkov commented Sep 28, 2021

Looks like tests are failing for an unrelated reason. I'll try to investigate over the weekend.

@mpenkov
Copy link
Collaborator

mpenkov commented Oct 10, 2021

OK, I finally got around to investigating this. The parameterizedtestcase package was misbehaving. I got rid of it, because it wasn't strictly necessary. I'll wait for the CI to complete and then continue reviewing this PR. Thanks for your patience!

@mpenkov
Copy link
Collaborator

mpenkov commented Oct 10, 2021

OK, looks good to me. Thank you!

@mpenkov mpenkov merged commit d9c864e into piskvorky:develop Oct 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants