-
Notifications
You must be signed in to change notification settings - Fork 769
Expose Azure Container Registry from compute environments via ContainerRegistry property #13755
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
base: main
Are you sure you want to change the base?
Conversation
…mentation Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
…prove error handling Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
src/Aspire.Hosting.Azure.ContainerRegistry/IAzureComputeEnvironmentWithContainerRegistry.cs
Outdated
Show resolved
Hide resolved
eerhardt
left a comment
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.
@copilot please implement the feedback
src/Aspire.Hosting.Azure.ContainerRegistry/IAzureComputeEnvironmentWithContainerRegistry.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.ContainerRegistry/IAzureComputeEnvironmentWithContainerRegistry.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13755Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13755" |
| } | ||
| } | ||
|
|
||
| ReferenceExpression IAzureContainerRegistry.ManagedIdentityId => ReferenceExpression.Create($"{ContainerRegistryManagedIdentityId}"); |
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.
@davidfowl @captainsafia - what you think about obsoleting the IAzureContainerRegistry interface? I think we made a mistake in designing it. It is more like "IAzureContainerRegistryInformation" or "IAzureContainerRegistryReference" - it doesn't really represent a registry, but instead a reference to a registry.
The ManagedIdentityId property on it is odd - it is the identity that the compute environment uses to pull from the registry, not the identity of the registry itself.
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 exposes the Azure Container Registry used by Azure compute environments (Azure Container Apps and App Service) through a new public API, enabling consumers to programmatically access and configure the registry for scenarios like setting up additional roles and permissions.
Key Changes:
- Introduces
IAzureContainerRegistryResourceinterface that combines Azure resource and container registry capabilities - Extends
IAzureComputeEnvironmentResourceinterface with a non-nullableContainerRegistryproperty - Implements the property in both
AzureContainerAppEnvironmentResourceandAzureAppServiceEnvironmentResourcewith validation logic - Adds comprehensive test coverage for all scenarios including default registry, explicit registry, and error cases
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting.Azure/IAzureContainerRegistryResource.cs |
New interface combining IAzureResource and IContainerRegistry for Azure Container Registry resources |
src/Aspire.Hosting.Azure/IAzureComputeEnvironmentResource.cs |
Extended interface with ContainerRegistry property returning IAzureContainerRegistryResource |
src/Aspire.Hosting.Azure.ContainerRegistry/AzureContainerRegistryResource.cs |
Updated to implement new IAzureContainerRegistryResource interface instead of IContainerRegistry directly |
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppEnvironmentResource.cs |
Implements ContainerRegistry property with validation, throws exceptions for unconfigured or non-Azure registries |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentResource.cs |
Implements ContainerRegistry property with validation, throws exceptions for unconfigured or non-Azure registries |
tests/Aspire.Hosting.Azure.Tests/AzureContainerAppEnvironmentExtensionsTests.cs |
Comprehensive tests covering default registry, explicit registry preference, and error scenarios |
tests/Aspire.Hosting.Azure.Tests/AzureAppServiceEnvironmentExtensionsTests.cs |
Comprehensive tests covering default registry, explicit registry preference, and error scenarios |
| /// <summary> | ||
| /// Gets the Azure Container Registry resource used by this compute environment. | ||
| /// </summary> | ||
| IAzureContainerRegistryResource ContainerRegistry { get => throw new NotImplementedException(); } |
Copilot
AI
Jan 8, 2026
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.
The interface documentation should follow Aspire's XML documentation guidelines more closely. According to guideline 1000002, public API documentation should include remarks with additional context about when and how to use the API. Consider adding a remarks section explaining scenarios where consumers would use this property, such as configuring additional roles, permissions, or ACR settings.
| /// <summary> | ||
| /// Gets the Azure Container Registry resource used by this Azure Container App Environment resource. | ||
| /// </summary> | ||
| public AzureContainerRegistryResource ContainerRegistry | ||
| { | ||
| get | ||
| { | ||
| var registry = GetContainerRegistry(); | ||
|
|
||
| if (registry is null) | ||
| { | ||
| throw new InvalidOperationException($"No container registry is configured for the Azure Container App Environment '{Name}'."); | ||
| } | ||
|
|
||
| if (registry is AzureContainerRegistryResource azureRegistry) | ||
| { | ||
| return azureRegistry; | ||
| } | ||
|
|
||
| throw new InvalidOperationException( | ||
| $"The container registry configured for the Azure Container App Environment '{Name}' is not an Azure Container Registry. " + | ||
| $"Only Azure Container Registry resources are supported. Use '.WithAzureContainerRegistry()' to configure an Azure Container Registry."); | ||
| } | ||
| } |
Copilot
AI
Jan 8, 2026
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.
According to Aspire's XML documentation guidelines (guideline 1000002), public API properties should include remarks and example sections for clarity. The property documentation should explain when exceptions are thrown and provide usage examples. Consider adding:
- A remarks section explaining the validation behavior and when to use this property
- An example section showing practical usage
- Exception tags documenting the InvalidOperationException scenarios
| /// <summary> | ||
| /// Gets the Azure Container Registry resource used by this Azure App Service Environment resource. | ||
| /// </summary> | ||
| public AzureContainerRegistryResource ContainerRegistry | ||
| { | ||
| get | ||
| { | ||
| var registry = GetContainerRegistry(); | ||
|
|
||
| if (registry is null) | ||
| { | ||
| throw new InvalidOperationException($"No container registry is configured for the Azure App Service Environment '{Name}'."); | ||
| } | ||
|
|
||
| if (registry is AzureContainerRegistryResource azureRegistry) | ||
| { | ||
| return azureRegistry; | ||
| } | ||
|
|
||
| throw new InvalidOperationException( | ||
| $"The container registry configured for the Azure App Service Environment '{Name}' is not an Azure Container Registry. " + | ||
| $"Only Azure Container Registry resources are supported. Use '.WithAzureContainerRegistry()' to configure an Azure Container Registry."); | ||
| } | ||
| } |
Copilot
AI
Jan 8, 2026
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.
According to Aspire's XML documentation guidelines (guideline 1000002), public API properties should include remarks and example sections for clarity. The property documentation should explain when exceptions are thrown and provide usage examples. Consider adding:
- A remarks section explaining the validation behavior and when to use this property
- An example section showing practical usage
- Exception tags documenting the InvalidOperationException scenarios
| /// <summary> | ||
| /// Represents an Azure Container Registry resource. | ||
| /// </summary> | ||
| public interface IAzureContainerRegistryResource : IContainerRegistry, IAzureResource | ||
| { | ||
| } |
Copilot
AI
Jan 8, 2026
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.
According to Aspire's XML documentation guidelines (guideline 1000002), public API interfaces should include a remarks section providing additional context about when and how to use the interface. Consider adding:
- A remarks section explaining that this interface combines Azure resource capabilities with container registry functionality
- A seealso reference to related types like IAzureComputeEnvironmentResource or AzureContainerRegistryResource
| /// <summary> | ||
| /// Gets the Azure Container Registry resource used by this Azure Container App Environment resource. | ||
| /// </summary> | ||
| public AzureContainerRegistryResource ContainerRegistry |
Copilot
AI
Jan 8, 2026
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.
The new public property ContainerRegistry on AzureContainerAppEnvironmentResource is missing from the auto-generated API file. Since this is a public API addition, it needs to be tracked in the API surface file. The API files should be regenerated after adding this property.
Similarly, the new interface IAzureContainerRegistryResource and the updated IAzureComputeEnvironmentResource interface with its new ContainerRegistry property need to be included in the appropriate API files.
| /// <summary> | ||
| /// Gets the Azure Container Registry resource used by this Azure App Service Environment resource. | ||
| /// </summary> | ||
| public AzureContainerRegistryResource ContainerRegistry |
Copilot
AI
Jan 8, 2026
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.
The new public property ContainerRegistry on AzureAppServiceEnvironmentResource is missing from the auto-generated API file. Since this is a public API addition, it needs to be tracked in the API surface file. The API files should be regenerated after adding this property.
| public AzureContainerRegistryResource ContainerRegistry | |
| internal AzureContainerRegistryResource ContainerRegistry |
This pull request introduces a new interface,
IAzureContainerRegistryResource, and updates the Azure compute environment resources to consistently expose and validate their associated Azure Container Registry resources. It also adds comprehensive tests to ensure correct behavior and validation when configuring container registries for Azure App Service and Container App environments.Key changes include:
API and Interface Improvements
IAzureContainerRegistryResourceinterface to represent Azure-specific container registry resources, and updatedAzureContainerRegistryResourceto implement this new interface. [1] [2]IAzureComputeEnvironmentResourceinterface to include a strongly typedContainerRegistryproperty of typeIAzureContainerRegistryResource, ensuring all compute environments expose their Azure container registry in a consistent way.Resource Property and Validation Enhancements
ContainerRegistryproperty to bothAzureContainerAppEnvironmentResourceandAzureAppServiceEnvironmentResource, which throws a clear exception if no registry is configured or if a non-Azure registry is set, enforcing correct usage. [1] [2]Checklist
<remarks />and<code />elements on your triple slash comments?Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.