-
Notifications
You must be signed in to change notification settings - Fork 609
AzureStorage auto create blob containers #9008
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
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.
Pull Request Overview
This PR introduces functionality to automatically create blob containers in Azure Storage and includes relevant updates to test coverage, resource configuration, and provisioning logic. Key changes include:
- Adding tests to verify Bicep generation and functional behavior of blob container creation on an Azure Storage emulator.
- Extending resource and extensions classes (AzureStorageResource, AzureBlobStorageResource, and AzureBlobStorageContainerResource) with new APIs for blob container handling and provisioning.
- Updating the end-to-end playground to reference and wait for the new blob container resource.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/Aspire.Hosting.Azure.Tests/AzureStorageExtensionsTests.cs | Added a test verifying Bicep manifest generation for blob container-related resources. |
tests/Aspire.Hosting.Azure.Tests/AzureStorageEmulatorFunctionalTests.cs | Introduced a new functional test to verify blob container creation and blob upload behavior on an emulator. |
src/Aspire.Hosting.Azure.Storage/AzureStorageResource.cs | Added a Blobs property for aggregating blob resources and adjusted constant declarations. |
src/Aspire.Hosting.Azure.Storage/AzureStorageExtensions.cs | Added logic to integrate blob containers into provisioning and new methods for adding and configuring blob container resources. |
src/Aspire.Hosting.Azure.Storage/AzureStorageEmulatorConnectionString.cs | Adjusted connection string formatting by removing a trailing semicolon. |
src/Aspire.Hosting.Azure.Storage/AzureBlobStorageResource.cs | Updated connection string composition and added a method for converting to a provisioning entity; note minor comment typo. |
src/Aspire.Hosting.Azure.Storage/AzureBlobStorageContainerResource.cs | Added a new resource class for Azure Blob Storage container with associated provisioning conversion. |
playground/AzureStorageEndToEnd/... | Updated end-to-end projects to reference and wait for blob container resources accordingly. |
Files not reviewed (1)
- tests/Aspire.Hosting.Azure.Tests/AzureStorageExtensionsTests.ResourceNamesBicepValid.verified.bicep: Language not supported
Comments suppressed due to low confidence (2)
src/Aspire.Hosting.Azure.Storage/AzureStorageEmulatorConnectionString.cs:15
- Removing the trailing semicolon from the connection string may affect how connection properties are parsed. Please verify that the new format is compatible with downstream consumers expecting a semicolon delimiter.
builder.Append("${key}=http://{endpoint.Property(EndpointProperty.IPV4Host)}:{endpoint.Property(EndpointProperty.Port)}/devstoreaccount1")
src/Aspire.Hosting.Azure.Storage/AzureBlobStorageResource.cs:68
- There is a typo in the comment: 'on;y' should be 'only'.
// We don't inject the queue resource here since we on;y want it to
Are you doing the client integration side of this as well? |
src/Aspire.Hosting.Azure.Storage/AzureBlobStorageContainerResource.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.Storage/AzureBlobStorageContainerResource.cs
Outdated
Show resolved
Hide resolved
For the most part this changeset copies the patterns implemented for AzureServiceBus. I tested it locally in the emulator and deploying to Azure (configuring the Azure subscription details in secrets.json) EmulatorAzurevar builder = DistributedApplication.CreateBuilder(args);
var storage = builder.AddAzureStorage("storage");
var blobs = storage.AddBlobs("blobs");
var blobContainer = blobs.AddBlobContainer("mycontainer");
var queues = storage.AddQueues("queues");
builder.AddProject<Projects.AzureStorageEndToEnd_ApiService>("api")
.WithExternalHttpEndpoints()
.WithReference(blobs).WaitFor(blobs)
.WithReference(blobContainer).WaitFor(blobContainer)
.WithReference(queues).WaitFor(queues); |
This comment was marked as outdated.
This comment was marked as outdated.
playground/AzureStorageEndToEnd/AzureStorageEndToEnd.AppHost/Program.cs
Outdated
Show resolved
Hide resolved
@@ -37,7 +37,7 @@ | |||
<!-- Issue: https://github.com/dotnet/aspire/issues/8488 --> | |||
<!-- xUnit1051: Calls to methods which accept CancellationToken should use TestContext.Current.CancellationToken to allow test cancellation to be more responsive. --> | |||
<!-- TODO: Re-enable and remove this. --> | |||
<NoWarn>$(NoWarn);xUnit1051</NoWarn> | |||
<NoWarn>$(NoWarn);xUnit1051;CS0162;CS1591;CS9113;IDE0059;IDE0051;IDE2000;IDE0005</NoWarn> |
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.
Building new API and experimenting is horribly difficult because of these (and many more) "warnings as errors".
I'll revert this when I get to the finish line.
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.
Opps, I forgot to rollback this. I'll fix it in a follow up.
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.
Did you revert 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.
|
||
partial class AspireBlobStorageExtensions | ||
{ | ||
private sealed class BlobStorageComponent : AzureComponent<AzureStorageBlobsSettings, BlobServiceClient, BlobClientOptions> |
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.
Moved this verbatim from AspireBlobStorageExtensions.cs
/// Azure SDK recommends treating clients as singletons <see href="https://devblogs.microsoft.com/azure-sdk/lifetime-management-and-thread-safety-guarantees-of-azure-sdk-net-clients/"/>, | ||
/// so this should be the exact same instance used by other parts of the application. | ||
/// </param> | ||
private sealed class AzureBlobStorageContainerHealthCheck(BlobContainerClient blobContainerClient) : IHealthCheck |
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's AzureBlobStorageHealthCheck
for BlobServiceClient
(comes from https://www.nuget.org/packages/AspNetCore.HealthChecks.Azure.Storage.Blobs). But there are no healthchecks for the containers.
Given that we're creating a blob container, I think checking that the container exists is sufficient.
src/Components/Aspire.Azure.Storage.Blobs/AzureBlobStorageContainerSettings.cs
Outdated
Show resolved
Hide resolved
tests/Aspire.Hosting.Azure.Tests/Aspire.Hosting.Azure.Tests.csproj
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
src/Components/Aspire.Azure.Storage.Blobs/AspireBlobStorageBuilder.cs
Outdated
Show resolved
Hide resolved
src/Components/Aspire.Azure.Storage.Blobs/AzureBlobStorageContainerSettings.cs
Outdated
Show resolved
Hide resolved
@eerhardt @sebastienros I added tests but I'm really unsure if or what more tests are needed. Implementation-wise, I think, this is ready. |
|
||
partial class AspireBlobStorageExtensions | ||
{ | ||
private sealed partial class BlobStorageContainerComponent : AzureComponent<AzureBlobStorageContainerSettings, BlobContainerClient, BlobClientOptions> |
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.
(nit) if we are going to break these out into their own files, do they need to be nested classes? Why not just make them internal and not nested?
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 are several reasons why I've done this. This style is present in our guidelines (https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md#example-file); and it's a common practice in the other repos I've been involved with. IMO, it improves the ability to navigate the codebase.
I can put all of these in the same file, if you insist.
But if these types are not nested, then the visibility needs to be increased which, essentially, will break the encapsulation (making the private implementation no longer private). It would also make the implementation inconsistent with the rest.
@eerhardt please let me know if there are any tweaks you want to make it and I'll action those as a follow up. |
Resolves #5167
Description
Checklist
<remarks />
and<code />
elements on your triple slash comments?doc-idea
templatebreaking-change
templatediagnostic
template