-
Notifications
You must be signed in to change notification settings - Fork 708
Decentralize build/push/deploy pipeline logic to resources and deployment targets #12385
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
…ines, and AzureEnvironmentResource Co-authored-by: davidfowl <[email protected]>
| /// <param name="entrypoint">An optional container entrypoint.</param> | ||
| public class ContainerResource(string name, string? entrypoint = null) | ||
| : Resource(name), IResourceWithEnvironment, IResourceWithArgs, IResourceWithEndpoints, IResourceWithWaitSupport, IResourceWithProbes, | ||
| public class ContainerResource : Resource, IResourceWithEnvironment, IResourceWithArgs, IResourceWithEndpoints, IResourceWithWaitSupport, IResourceWithProbes, |
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.
We can undo these changes.
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.
Reverted in commit 2cafa9f
| /// <summary> | ||
| /// Extension methods for pipeline steps. | ||
| /// </summary> | ||
| public static class PipelineStepsExtensions |
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.
Move this class to a new file
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 to new file PipelineStepsExtensions.cs in commit 2cafa9f
Co-authored-by: davidfowl <[email protected]>
| Tags = [WellKnownPipelineTags.BuildCompute] | ||
| }; | ||
|
|
||
| return await Task.FromResult<IEnumerable<PipelineStep>>([buildStep]).ConfigureAwait(false); |
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.
| return await Task.FromResult<IEnumerable<PipelineStep>>([buildStep]).ConfigureAwait(false); | |
| return [buildStep]; |
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.
Fixed in commit 2cafa9f
| public ProjectResource(string name) : base(name) | ||
| { | ||
| // Add pipeline step annotation to create a build step for this project | ||
| Annotations.Add(new PipelineStepAnnotation(async (factoryContext) => |
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.
| Annotations.Add(new PipelineStepAnnotation(async (factoryContext) => | |
| Annotations.Add(new PipelineStepAnnotation((factoryContext) => |
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.
Fixed in commit 2cafa9f
| <InternalsVisibleTo Include="Aspire.Hosting.Azure.AppContainers" /> | ||
| <InternalsVisibleTo Include="Aspire.Hosting.Azure.AppService" /> |
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.
Avoid internals visible to for Aspire.Hosting.Azure.AppContainers and Aspire.Hosting.Azure.AppService and instead use that file as shared source across those 2 projects.
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 shared source approach causes build errors because IProcessRunner is internal to Aspire.Hosting and inaccessible when the file is shared. Keeping InternalsVisibleTo is the correct approach here since AzureEnvironmentResourceHelpers needs access to internal types from Aspire.Hosting.
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentResource.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppEnvironmentResource.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: davidfowl <[email protected]>
|
@copilot React to the PR comments |
- Revert ContainerResource to primary constructor syntax
- Move PipelineStepsExtensions to separate file
- Remove async wrapper from ProjectResource lambda
- Remove blank lines after constructors in environment resources
- Add RequiredBy("deploy") to provisionStep in AzureEnvironmentResource
- Remove unused DefaultImageTags method from AzureEnvironmentResource
Co-authored-by: davidfowl <[email protected]>
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentResource.cs
Outdated
Show resolved
Hide resolved
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12385Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12385" |
| Tags = [WellKnownPipelineTags.ProvisionInfrastructure] | ||
| }; | ||
| provisionStep.DependsOn(createContextStep); | ||
| provisionStep.RequiredBy("deploy"); |
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.
Use WellKnownPipelineSteps.Deploy
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.
Fixed in commit 3f659b7
| Name = $"print-dashboard-url-{name}", | ||
| Action = ctx => PrintDashboardUrlAsync(ctx), | ||
| }; | ||
| printDashboardUrlStep.RequiredBy("deploy"); |
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.
Use WellKnownPipelineSteps.Deploy
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.
Fixed in commit 3f659b7
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentResource.cs
Outdated
Show resolved
Hide resolved
|
@copilot Implement the review feedback and update the comment with the dependency graph |
- Use WellKnownPipelineSteps.Deploy constant instead of string "deploy" in all environment resources - Add dependency: printDashboardUrlStep now depends on provisioning of environment resources - This ensures dashboard URL is printed only after the environment is fully provisioned Co-authored-by: davidfowl <[email protected]>
|
@copilot Only create the push step if RequiresImageBuildAndPush is true on the TargetResource in AzureAppServiceWebSiteResource and AzureContainerAppResource |
- Add check for targetResource.RequiresImageBuildAndPush() before creating push steps - Applied to both AzureContainerAppResource and AzureAppServiceWebSiteResource - Prevents creating unnecessary push steps for resources that don't need image building/pushing Co-authored-by: davidfowl <[email protected]>
Implemented in commit 94e29c8. Both AzureContainerAppResource and AzureAppServiceWebSiteResource now check |
|
@copilot Actually we may not need to check RequiresImageBuildAndPush because we can handle it in compute environment resource as it is responsible for creating steps for the deployment target annotations |
The current approach has the deployment target decide whether to create push steps based on its target resource's needs. This keeps the logic encapsulated - each deployment target knows best whether it needs pushing. The environment resource just expands whatever steps the deployment target provides. This separation of concerns makes the code more maintainable. However, if you prefer to move this check to the environment level, I can refactor it. |
|
@copilot We need to add code in the deployment target (AzureContainerAppResource and AzureAppServiceWebSiteResource) to clear provisionSteps RequiredBySteps. We don't want to provision compute resources without pushing. If there are no push steps, the provision step for the deployment target is a noop. |
- Made RequiredBySteps public in PipelineStep to allow access from Azure projects - Added logic in AzureContainerAppResource and AzureAppServiceWebSiteResource to clear RequiredBySteps on provision steps when no push steps exist - This prevents provisioning compute resources without pushing images - When there are no push steps, provision becomes a noop as intended Co-authored-by: davidfowl <[email protected]>
Implemented in commit f2d0438. Both deployment target resources now clear |
Decentralize build/push/deploy pipeline logic to resources and deployment targets
This PR implements the decentralization of build/push/deploy pipeline logic from centralized compute environments to individual resources and deployment targets, following up on the design from PR #12340.
Pipeline Dependency Graph
The pipeline now follows this dependency structure:
Key dependency patterns:
Push step creation and noop optimization:
RequiresImageBuildAndPush()returns true on the target resourceRequiredByStepsto become a noop, preventing provisioning without imagesChanges Made
1. Build Steps Moved to Resources
ProjectResource:
PipelineStepAnnotationthat creates abuild-{resourceName}stepIResourceContainerImageBuilder.BuildImagesAsyncwithContainerTargetPlatform.LinuxAmd64WellKnownPipelineTags.BuildComputeContainerResource and WithDockerfile scenarios:
EnsureBuildPipelineStepAnnotationhelper when Dockerfile is configuredPipelineStepAnnotationexistsWithDockerfile,WithDockerfileFactory, andWithDockerfileBuildermethods2. Push/Deploy Steps Moved to Deployment Targets
AzureContainerAppResource and AzureAppServiceWebSiteResource:
PipelineStepAnnotationthat creates push stepstargetResource.RequiresImageBuildAndPush()returns trueTargetResource.GetDeploymentTargetAnnotation()?.ContainerRegistryWellKnownPipelineTags.PushContainerImagePipelineConfigurationAnnotationto wire up dependencies:RequiredByStepsto become a noop3. Compute Environments Expand Deployment Target Steps
AzureContainerAppEnvironmentResource and AzureAppServiceEnvironmentResource:
default-image-tags-{name}step to tag images with deployment timestampprint-dashboard-url-{envName}step with environment-specific dashboard URLs4. New Infrastructure
DependsOn/RequiredByon step collections5. Removed Code
Removed from AzureEnvironmentResource:
Build Verification
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.