-
Notifications
You must be signed in to change notification settings - Fork 295
Update azure blob SDK version #1515
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
Conversation
if changes is None: | ||
raise Exception("Changes are required when writing") | ||
if not changes: | ||
return | ||
|
||
self.client.create_container(self.settings.container_name) | ||
self.client.set_container_acl( | ||
self.settings.container_name, public_access=PublicAccess.Container |
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.
I didn't see anything regarding public access
referenced in .NET or JS and tests all passed fine without it. I can add it back, if necessary.
if e_tag: | ||
await blob_reference.upload_blob( | ||
item_str, match_condition=MatchConditions.IfNotModified, etag=e_tag | ||
) | ||
except Exception as error: | ||
raise error | ||
else: | ||
await blob_reference.upload_blob(item_str, overwrite=True) |
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.
Match conditions and eTags changed quite a bit and I'm not 100% confident that I implemented this correctly. I tried some other approaches and this was the one that made all of our tests pass. Please review this part closely.
Edit: Fixed. I was trying to encode the blob names ( |
Partial fix for #1343
Please Read
#1343 is for porting over Azure Blobs and Queues from .NET and JS. In those SDK, Azure Blobs was plural-ified from
Bot.Builder.Azure.BlobStorage
to...Blob*s*Storage
, adding a new Blob Storage library. This was done because of a dependency namespace conflict that would break binary compatibility.As far as I can tell, we can't do this in the Python SDK because that would require the SDK to depend on two different versions of
azure.storage.blob
. The current version in our SDK is 2.1, but the most recent version is 12.7.1. This PR updates to 12.7.0 because there's an msrest dependency conflict at 122.7.1.So, this PR updates our current
blob_storage
to just use the new SDK. If approved, I'll then add an additionalqueue_storage
.There's some pretty big changes in the the internal code for this PR due to:
azure.storage.blob
10 major versions, andTesting
All tests pass when running Storage Emulator and setting this to True so the tests run.
The warning are all of this nature:
...but per @axelsrz, that's expected for our SDK and Python 3.8.
Backwards Compatibility Testing
I also tested by:
botbuilder.azure
from sample, then added newblob_storage.py
and ensured it used thatblob_storage
Conversation and UserState data still look the same:
I'm not sure why e_tag gets quotes placed around it. I don't do any encoding, so I think it's something done by
azure.storage.blob
. I sort of mitigate it hereNote
This library will eventually benefit from investigating some of the Blob Storage options, but I won't have the bandwidth to land this in R12.