Adds Azure Blob Storage media backend #303
Conversation
terricain
force-pushed
the
add_azure_blob_storage
branch
from
e51a538
to
34921d2
Compare
piccolo_api/media/azure.py
Outdated
|
|
||
| _, blob_service_client = self.get_client() | ||
|
|
||
| now = datetime.datetime.now(datetime.UTC) |
There was a problem hiding this comment.
It looks like the tests are failing because of this on older Python versions - we'll have to use datetime.timezone.utc.
There was a problem hiding this comment.
Ah yep, makes sense 😄. Should be fixed now
|
@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. |
| sys.exit( | ||
| "Specified connection string did not contain AccountKey" |
There was a problem hiding this comment.
Maybe we just raise a ValueError here
|
|
||
| if not self.sign_urls: | ||
| return ( | ||
| f"https://{self.storage_account_name}.blob.core.windows.net/" |
There was a problem hiding this comment.
Is it worth having blob.core.windows.net as a class variable, so someone can override this somehow if needed? Just in case there are other services which are API compatible with Azure storage.
| 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?
| 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_string| f"https://{self.storage_account_name}.blob.core.windows.net" | ||
| ) |
There was a problem hiding this comment.
We reference this URL in quite a few places - what do you think about setting it in the constructor, so we just do self.account_url = f'https://{self.storage_account_name}.blob.core.windows.net' and then reference self.account_url in the rest of the code?
| 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.
Adds a MediaStorage plugin for Azure Blob Storage