-
Notifications
You must be signed in to change notification settings - Fork 745
Add Push and PushPrereq pipeline steps for ContainerRegistryResource #13306
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
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13306Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13306" |
|
@copilot Bleh, we need to have the ContainerRegistryReferenceAnnotation on all resources so that we can correctly compute the remote image tag during the push phase. Add a PushPrereq step that will add the ContainerRegistryReferenceAnnotation to resources that don't already have it when a single container registry exists in the app model. When multiple registries exist, throw an exception that the user must call |
Done in ed1e587. The |
| } | ||
|
|
||
| // When multiple registries exist, require explicit WithContainerRegistry call | ||
| if (allRegistries.Length > 1) |
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.
Pushing to all doesn't seem like bad behavior though I understand its different from compute environments (at the moment).
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.
Yep, I modeled this based on the patterns we have for compute environments. IMO, I think pushing towards a 1:1 relationship between registries and images is probably better for push (and eventually pull). Pushing to multiple registries increases the overhead associated with a single push call. We'd have to authenticate/tag/push N times instead of 1. I also think it's the kind of thing where we'd want the user to be more intentional about where images gets pushed given the security implications.
|
Does this break when you have 2 compute environments now? |
Probably, lol. TBH, I haven't gotten that far. I'm just trying to get the end-to-end over in aspire-image-push working before layering on things. My intuition tells me that as a fall out of this PR we'll need to model the Azure Container Registry that is implicitly in the compute environments as an explicit thing and always model the push steps on a resource that implements IContainerRegistry instead of within the compute environments. |
|
@copilot We need to make sure that the PushPrereq runs unconditionally on compute resources that need a push. Currently, GetResourcesToPush introduces a gap in the implementation because we will never wire up the PipelineStepConfiguration for the PushPrereq if there isn't already a ContainerRegistryReferenceAnnotation set (which is done by the PushPrereq). |
Fixed in 335a826. Refactored |
src/Aspire.Hosting/ApplicationModel/ContainerRegistryResource.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/ContainerRegistryResource.cs
Outdated
Show resolved
Hide resolved
| /// <param name="builder">The distributed application builder.</param> | ||
| /// <param name="name">The name of the container registry resource.</param> | ||
| /// <returns>The created <see cref="AzureContainerRegistryResource"/>.</returns> | ||
| public static AzureContainerRegistryResource CreateDefaultContainerRegistry(IDistributedApplicationBuilder builder, string name) |
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.
CreateDefaultAzureContainerRegistry
Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
…meters Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
…e registry Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
e9c206e to
b56d325
Compare
9485950 to
6cdddc1
Compare
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 pull request adds container image push pipeline infrastructure to ContainerRegistryResource instances. The key changes include new well-known pipeline steps (Push and PushPrereq), refactoring of push logic from environment resources to individual container resources, and automatic registry selection logic.
Key Changes
- Added
WellKnownPipelineSteps.PushandPushPrereqconstants for pipeline coordination - Refactored push step creation from environment resources to individual container/project resources
- Implemented automatic registry annotation in
PushPrereqstep with clear error messages for multiple registries - Created
PipelineStepHelpers.PushImageToRegistryAsyncfor shared push logic - Updated Azure environments to create default ACR resources
- Extensive snapshot test updates reflecting new pipeline structure
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
WellKnownPipelineSteps.cs |
Added Push and PushPrereq constants |
PipelineStepHelpers.cs |
New helper for push operations |
DistributedApplicationPipeline.cs |
Added PushPrereq step with automatic registry annotation logic |
ContainerResourceBuilderExtensions.cs |
Renamed method to include push, added push step generation |
ProjectResource.cs |
Added push step generation alongside build steps |
AzureContainerRegistryResource.cs |
Added login step and pipeline configuration |
AzureContainerRegistryHelpers.cs |
Refactored helper methods |
| Azure environment resources | Removed push/login logic, delegated to registry resources |
| Test files | Updated assertions for new step counts and dependencies |
| Snapshot files | Updated Bicep generation for separate ACR resources |
| <InternalsVisibleTo Include="Aspire.Hosting.Azure.AppContainers" /> | ||
| <InternalsVisibleTo Include="Aspire.Hosting.Azure.AppService" /> | ||
| <InternalsVisibleTo Include="Aspire.Hosting.Azure.Tests" /> |
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.
File an issue for this one.
| foreach (var pushStep in pushSteps) | ||
| { | ||
| foreach (var buildStep in projectBuildSteps) | ||
| { | ||
| pushStep.DependsOn(buildStep); | ||
| } | ||
| pushStep.DependsOn(WellKnownPipelineSteps.PushPrereq); | ||
| } |
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.
| foreach (var pushStep in pushSteps) | |
| { | |
| foreach (var buildStep in projectBuildSteps) | |
| { | |
| pushStep.DependsOn(buildStep); | |
| } | |
| pushStep.DependsOn(WellKnownPipelineSteps.PushPrereq); | |
| } | |
| pushSteps.DependsOn(projectBuildSteps); | |
| pushSteps.DependsOn(WellKnownPipelineSteps.PushPrereq); |
|
|
||
| return builder.WithAnnotation(new PipelineConfigurationAnnotation(context => | ||
| { | ||
| var buildSteps = context.GetSteps(builder.Resource, WellKnownPipelineTags.BuildCompute); |
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.
See above comment for simplification of this.
| // Single registry - automatically add the annotation | ||
| if (allRegistries.Length == 1) | ||
| { | ||
| resource.Annotations.Add(new ContainerRegistryReferenceAnnotation(allRegistries[0])); |
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.
A little bit worrying that we do this so late. The annotation list is not threadsafe.
Description
Adds pipeline infrastructure for pushing container images to
ContainerRegistryResourceinstances. The registry resource now directly creates push steps for all resources that reference it.Key changes:
WellKnownPipelineSteps.PushandWellKnownPipelineSteps.PushPrereqconstantsContainerRegistryResourceregistersPipelineStepAnnotationto create per-resource push stepsContainerRegistryResourceregistersPipelineConfigurationAnnotationto wire Build → Push dependenciesPushunconditionally depends onPushPrereqto ensure annotations are set up before any push operationsPushPrereqstep automatically addsContainerRegistryReferenceAnnotationto resources that don't have one when a single registry exists (required for correct remote image tag computation during push)GetResourcesToPushonly includes resources withContainerRegistryReferenceAnnotationmatching the target registry.WithContainerRegistry()annotationExample usage:
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplateOriginal prompt
Container Registry Push Pipeline Step Plan
Overview
This plan proposes adding support for pushing container images to
ContainerRegistryResourceinstances through a well-known pipeline step. TheContainerRegistryResourcewill directly register pipeline steps for all resources that reference it.Motivation
Currently, the
ContainerRegistryResourceserves as a reference for container registries but doesn't participate in the deployment pipeline. To enable deployment scenarios where containers need to be pushed to external registries (Docker Hub, GHCR, private registries), we need to:The reference implementation from https://github.com/captainsafia/aspire-image-push demonstrates this need with a custom pipeline step that pushes images to a parameterized container registry.
Goals
WellKnownPipelineSteps.Pushas a well-known pipeline stepContainerRegistryResourceto directly create push steps for all resources that require image pushDeploymentTargetAnnotationfor consistencyAPI Design
1. Add Well-Known Pipeline Step
Add a new constant to
WellKnownPipelineSteps.cs:2. ContainerRegistryResource Pipeline Integration
Update
ContainerRegistryResourceto directly create push steps for all resources that reference it: