Move ContainerBuildOptions to resource-specific annotation#13081
Move ContainerBuildOptions to resource-specific annotation#13081captainsafia merged 5 commits intomainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13081Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13081" |
c297d5d to
ffda18d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new extensibility point for configuring container build options in Aspire Hosting by migrating from a parameter-based approach to a resource-specific annotation-based callback mechanism. This enables more flexible, dynamic, and resource-specific configuration of container images.
Key Changes
- New Annotation Architecture: Introduced
ContainerBuildOptionsCallbackAnnotationandContainerBuildOptionsCallbackContextto provide a callback-based mechanism for configuring container build options on resources - API Simplification: Removed
ContainerBuildOptionsparameter fromIResourceContainerImageBuilder.BuildImageAsyncandBuildImagesAsyncmethods, making options resource-specific rather than call-specific - Comprehensive Test Coverage: Added 242 new test lines covering multiple annotation behavior scenarios, async callbacks, and default annotation verification
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting/ApplicationModel/ContainerBuildOptionsCallbackAnnotation.cs |
New file defining the callback annotation and context for configuring container build options with async support |
src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs |
Added ProcessContainerBuildOptionsCallbackAsync extension method to process all container build options annotations on a resource |
src/Aspire.Hosting/ApplicationModel/ProjectResource.cs |
Added default annotation that dynamically resolves image name and tag from MSBuild project properties (ContainerRepository, ContainerImageTag) |
src/Aspire.Hosting/Publishing/ResourceContainerImageBuilder.cs |
Refactored to resolve options from resource annotations instead of parameters, introduced ResolvedContainerBuildOptions internal class |
src/Aspire.Hosting/ProjectResourceBuilderExtensions.cs |
Added WithContainerBuildOptions extension methods (sync and async overloads) for project resources |
src/Aspire.Hosting/ContainerResourceBuilderExtensions.cs |
Updated WithDockerfile and WithDockerfileFactory to add default build options annotations, added WithContainerBuildOptions extension methods for container resources |
tests/Aspire.Hosting.Tests/Publishing/ResourceContainerImageBuilderTests.cs |
Updated all tests to use new annotation-based API, added comprehensive tests for annotation behavior including ordering, overrides, and async callbacks |
tests/Aspire.Hosting.Tests/MockImageBuilder.cs |
Updated mock to match new interface signature without options parameter |
tests/Aspire.Hosting.Docker.Tests/DockerComposeTests.cs |
Updated mock implementation to match new interface |
tests/Aspire.Hosting.Docker.Tests/DockerComposePublisherTests.cs |
Updated mock implementation to match new interface |
src/Aspire.Hosting/CompatibilitySuppressions.xml |
Added suppressions for API breaking changes (CP0002 and CP0006 diagnostics) |
| /// <summary> | ||
| /// Initializes a new instance of <see cref="ContainerBuildOptionsCallbackAnnotation"/> with a synchronous callback. | ||
| /// </summary> | ||
| /// <param name="callback">The synchronous callback action to configure container build options.</param> | ||
| public ContainerBuildOptionsCallbackAnnotation(Action<ContainerBuildOptionsCallbackContext> callback) |
There was a problem hiding this comment.
The XML documentation for this constructor should include a <remarks> tag explaining that the synchronous callback is converted to an async callback internally. According to the XML documentation guidelines, constructors with specific behavior should document those details.
Example:
/// <summary>
/// Initializes a new instance of <see cref="ContainerBuildOptionsCallbackAnnotation"/> with a synchronous callback.
/// </summary>
/// <param name="callback">The synchronous callback action to configure container build options.</param>
/// <remarks>
/// The synchronous callback is automatically wrapped in a <see cref="Task"/> for compatibility with the async callback infrastructure.
/// </remarks>| .WithAnnotation(defaultContainerBuildOptions) | ||
| .EnsureBuildPipelineStepAnnotation(); | ||
| } | ||
|
|
||
| return builder.WithAnnotation(annotation, ResourceAnnotationMutationBehavior.Replace) | ||
| .WithAnnotation(defaultContainerBuildOptions) |
There was a problem hiding this comment.
[nitpick] Inconsistent mutation behavior specification for ContainerBuildOptionsCallbackAnnotation. In WithDockerfileFactory (lines 713, 718), the mutation behavior is explicitly set to ResourceAnnotationMutationBehavior.Append, but here it's omitted (which defaults to Append). For consistency and clarity, the mutation behavior should be explicitly specified in both places.
Suggested fix:
.WithAnnotation(defaultContainerBuildOptions, ResourceAnnotationMutationBehavior.Append)| .WithAnnotation(defaultContainerBuildOptions) | |
| .EnsureBuildPipelineStepAnnotation(); | |
| } | |
| return builder.WithAnnotation(annotation, ResourceAnnotationMutationBehavior.Replace) | |
| .WithAnnotation(defaultContainerBuildOptions) | |
| .WithAnnotation(defaultContainerBuildOptions, ResourceAnnotationMutationBehavior.Append) | |
| .EnsureBuildPipelineStepAnnotation(); | |
| } | |
| return builder.WithAnnotation(annotation, ResourceAnnotationMutationBehavior.Replace) | |
| .WithAnnotation(defaultContainerBuildOptions, ResourceAnnotationMutationBehavior.Append) |
|
So, two things that are not great about this API:
|
6fbd3b9 to
e21b271
Compare
| /// <summary> | ||
| /// Gets the resource being built. | ||
| /// </summary> | ||
| public IResource Resource { get; } |
There was a problem hiding this comment.
Can I make a "build options" across all resources? For example if I want to build all images to a .tar file and not to the local registry?
There was a problem hiding this comment.
Yes, you'll have to apply the annotation to all the resources with the defaults you want. I can see this being something that would be done as part of the BuildPrereq step.
There was a problem hiding this comment.
Would it be better if we had a "global options" and each resource could override the global options?
There was a problem hiding this comment.
Would it be better if we had a "global options" and each resource could override the global options?
This sort of exists at the moment because we populate each resource with the annotation in its constructor (see here).
We could also consider codifying this behavior in some sort of global IOptions instance.
|
Where is the usage example? |
The annotation is currently optional but you can override it with something like: |
This pull request introduces a new extensibility point for configuring container build options in Aspire Hosting by adding the
ContainerBuildOptionsCallbackAnnotationand its associated context. This enables more flexible and dynamic configuration of container image names, tags, and platforms for both project and container resources. The changes also update resource builders and pipeline steps to use this new mechanism, ensuring consistency and easier customization.Extensibility for container build options:
ContainerBuildOptionsCallbackAnnotationandContainerBuildOptionsCallbackContextinContainerBuildOptionsCallbackAnnotation.cs, providing a callback-based mechanism for configuring container build options on resources.ProcessContainerBuildOptionsCallbackAsyncextension method inResourceExtensions.csto process all relevant annotations and accumulate container build options.Integration with resource builders:
WithDockerfileandWithDockerfileFactorymethods inContainerResourceBuilderExtensions.csto automatically add default container build options annotations based on Dockerfile metadata, and introduced newWithContainerBuildOptionsoverloads for custom configuration. [1] [2]ProjectResourceand its build pipeline to add a default container build options annotation that dynamically resolves image name and tag from MSBuild project properties, and updated build logic to use the new callback context. [1] [2] [3] [4]Compatibility and suppression updates:
CompatibilitySuppressions.xmlto maintain baseline compatibility. [1] [2]