-
-
Notifications
You must be signed in to change notification settings - Fork 548
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
Update BlobStorage to use Azure.Storage.Blobs #1564
Conversation
…Azure.Storage.Blob
/// <param name="naming">How uploaded media files should be named</param> | ||
/// <param name="scope">The optional service scope. Default is singleton</param> | ||
/// <returns>The service collection</returns> | ||
public static IServiceCollection AddPiranhaBlobStorage( | ||
this IServiceCollection services, | ||
StorageCredentials credentials, |
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.
This constitutes the main breaking change when moving to the v.12 Azure SDK, as the StorageCredentials
class is no longer available.
_naming = naming; | ||
_storage.CreateIfNotExists(PublicAccessType.Blob); |
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.
Does this create a request towards Azure? If so I would recommend keeping the async variant as was before.
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.
@mikaellindemann this should not be inside the constructor, agree. On the other hand, the method IStorage.OpenAsync()
would not be a proper place either. Maybe an idea to add a method IStorage.InitializeAsync()
where initial plumbing and checks can be performed?
When using Azure.Storage.Blobs
assembly classes, there is no real concept of "sessions".
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.
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.
My point was that if it does asynchronous operations (IO being one of them) then it should be an asynchronous function. I didn't check what is behind CreateIfNotExists, but since ExistsAsync and CreateAsync was in use before, I guess that CreateIfNotExists is waiting on the asynchronous operation behind the scenes.
Not sure if I encouraged this change you have made, sorry if I did, but now you have introduced a breaking change in the storage extension point of Piranha. Also, you have limited any storage implementation not to be session based.
Wouldn't it make sense to keep the storage and session implementation as was, such that a BlobContainerClient was the underlying session for a BlobStorage? (Tbh. I don't know the details of IStorage, IStorageSession and Azure BlobStorage, so maybe this is a bad suggestion).
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.
@mikaellindemann I reversed most of those breaking changes. Guess I was a bit too enthousiastic. Please take a second look at the result.
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.
And with regards to that CreateIfNotExists, it's about ensuring the parent Storage Container is available. So this should optimally be part of the App Init chaining.
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.
And with Azure.Storage.Blobs assembly there is no real concept of sessions.
The BlobContainerClient
is maintained as a singleton via DI / BlobStorage
class. And BlobClient
class instances are lightweight and are instantiated from the BlobContainerClient
.
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.
@mikaellindemann I have now reverted any breaking changes to existing interfaces. And reverted all changes to the Piranha.Local.FileStorage
source.
What remains now inside this PR:
- the move to the Azure SDK v.12 using the recommended
Azure.Storage.Blobs
Nuget packages - Add the interface
IInitializable
to optionally indicate that a class implementsInit()
method, which is then called via App.cs
{ | ||
return (IStorageSession)new FileStorageSession(this, _basePath, _baseUrl, _naming); | ||
}); | ||
return Task.FromResult<IStorageSession>(this); |
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.
@mikaellindemann this should make things better scalable.
# Conflicts: # core/Piranha/IStorageSession.cs
Add check if IMediaService instance implements IInitializable
Hello! what is the state of this? |
The PR was opened after |
Other option could be to introduce new Piranha Nuget package with name |
What breaking changes does it contain? Do you perhaps mean the If the only breaking changes are related to this package AND we can continue using Piranha 9, why must it wait? If we increase the MAJOR version of this package to indicate it is a breaking change, and later on increase MINOR (or PATCH) for the 10.0 version bump, we can already release it? |
@tedvanderveen I like the idea of being able to use the new Azure SDK NOW... But I think creating an extra package for just 2 months would be a bit much. If you really wanted to, you could try and just put the code from this PR into your project and have it work that way?
|
I will merge this into |
return $"{ _containerUrl }/{ GetResourceName(media, id, true) }"; | ||
} | ||
return null; | ||
return string.IsNullOrWhiteSpace(id) ? default : $"{_blobContainerClient.Uri.AbsoluteUri}/{GetResourceName(media, id, 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.
This method is no longer checking that media != null
, and in GetResourceName()
there is an access to media.Id
without checking for null values either. But I can fix this after merge.
Use the v.12 version of the Azure Storage SDK to replace the now deprecated Microsoft.Azure.Storage.Blob nuget packages
See also #944