Adds Azure Blob Storage media backend#303
Adds Azure Blob Storage media backend#303terricain wants to merge 3 commits intopiccolo-orm:masterfrom
Conversation
terricain
force-pushed
the
add_azure_blob_storage
branch
from
e51a538 to
34921d2
Compare
|
@terricain Thanks for this! |
terricain
force-pushed
the
add_azure_blob_storage
branch
from
34921d2 to
3565b00
Compare
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #303 +/- ##
==========================================
- Coverage 92.69% 90.36% -2.33%
==========================================
Files 34 43 +9
Lines 2053 2596 +543
==========================================
+ Hits 1903 2346 +443
- Misses 150 250 +100 ☔ View full report in Codecov by Sentry. |
dantownsend
left a comment
dantownsend
left a comment
There was a problem hiding this comment.
Thanks a lot for this - sorry it took me a while to get around to reviewing it.
I think it looks great, just left some minor comments 👍
| storage_account_name: str, | ||
| container_name: str, | ||
| folder_name: t.Optional[str] = None, | ||
| connection_kwargs: t.Optional[t.Dict[str, t.Any]] = None, |
There was a problem hiding this comment.
We don't really use connection_kwargs for much at the moment - it is just being used as a way of passing a connection string.
Should we pass the connection_kwargs to DefaultAzureCredential? I know the user can set most of the options via environment variables, but it might be convenient to pass them in directly.
In which case, connection string, could be a separate argument, or:
connection_kwargs: t.Optional[t.Dict[str, t.Any]] | str = NoneIf it's a string, we treat it as a connection string.
What do you think?
There was a problem hiding this comment.
I thought about it, I think instead of having a union of an optional dictionary or string, leave it as an optional dictionary and if 'connection_string' is not present, pass the kwargs to DefaultCredential
piccolo_api/media/azure.py
Outdated
| if "connection_string" in self.connection_kwargs: | ||
| self.connection_string = self.connection_kwargs[ | ||
| "connection_string" | ||
| ] | ||
| elif "AZURE_STORAGE_CONNECTION_STRING" in os.environ: | ||
| self.connection_string = os.environ[ | ||
| "AZURE_STORAGE_CONNECTION_STRING" | ||
| ] |
There was a problem hiding this comment.
Not really a big deal, but we can use the walrus operator to simplify this a little bit.
if connection_string := (
self.connection_kwargs.get("connection_string")
or os.environ.get("AZURE_STORAGE_CONNECTION_STRING")
):
self.connection_string = connection_stringThere was a problem hiding this comment.
Managed to simplify this somewhat taking into account the previous message.
| self.container_name | ||
| ) | ||
|
|
||
| return container_client, blob_service_client |
There was a problem hiding this comment.
I don't know a huge amount about Azure, and the differences between the two clients. We could split this into two separate methods ... one for each client type ... not sure. We can leave it as is for now.
There was a problem hiding this comment.
We could just return the blob_service_client (which we have to instantiate differently based on how you're authing) and then call something like self.get_client().get_container_client(self.container_name) if needed but we need the container client 90% of the time, so I figured what we have now is slightly simpler. Up to you.
|
Sorry for the wait, been a bit busy, have addressed the comments, let me know what you think. |
|
@dantownsend this is ready to be merged I think |
3 participants
Adds a MediaStorage plugin for Azure Blob Storage