-
Notifications
You must be signed in to change notification settings - Fork 720
Add support for pipeline step tagging and configuration phase dependency management #12293
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
Add support for pipeline step tagging and configuration phase dependency management #12293
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.
@copilot Fix these issues.
| #pragma warning disable ASPIREAZURE001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. | ||
| #pragma warning disable ASPIREPUBLISHERS001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. | ||
| #pragma warning disable ASPIREINTERACTION001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. | ||
| #pragma warning disable CS0618 // Type or member is obsolete |
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.
Why is this needed?
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 pragma was needed because we had marked WellKnownPipelineSteps as obsolete while keeping it for backward compatibility. I've now removed the obsolete class entirely and renamed WellKnownPipelineSteps to WellKnownPipelineTags as requested, so the pragma is no longer needed. Fixed in 3a44d2f.
| _steps.Add(step); | ||
| } | ||
|
|
||
| public void AddSecondPassCallback(Func<PipelinePassContext, Task> callback) |
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.
| public void AddSecondPassCallback(Func<PipelinePassContext, Task> callback) | |
| public void AddPipelineConfiguration(Func<PipelineConfigurationContext, Task> callback) |
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.
Done. Renamed to AddPipelineConfiguration in 3a44d2f.
| /// allowing modification of step dependencies and relationships. | ||
| /// </summary> | ||
| [Experimental("ASPIREPIPELINES001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| public class PipelinePassAnnotation : IResourceAnnotation |
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.
| public class PipelinePassAnnotation : IResourceAnnotation | |
| public class PipelineConfigurationAnnotation : IResourceAnnotation |
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.
Done. Renamed to PipelineConfigurationAnnotation in 3a44d2f.
| /// <summary> | ||
| /// Gets the resource that registered this pass callback, if any. | ||
| /// </summary> | ||
| public IResource? Resource { get; internal set; } |
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.
Remove the Resource property from this context and add a property for the DistributedApplicationModel.
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.
Done. Removed the Resource property and added ApplicationModel property to PipelineConfigurationContext. Tests updated to find resources via ApplicationModel.Resources. Fixed in 3a44d2f.
| /// Defines well-known pipeline step names used in the deployment process. | ||
| /// </summary> | ||
| [Experimental("ASPIREPIPELINES001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| [Obsolete("Use WellKnownPipelineTags instead.")] |
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.
Don't use obsolete here. Rename this type and remove the new one you added.
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.
Done. Removed the obsolete attribute and the duplicate WellKnownPipelineTags class. WellKnownPipelineSteps has been renamed to WellKnownPipelineTags as the single source of truth. Fixed in 3a44d2f.
|
@copilot - Fix the bulid errors in playground/pipelines/Pipelines.AppHost/AppHost.cs |
|
@copilot Update the |
Updated AzureEnvironmentResource to use unique step names instead of tag constants. Steps now have descriptive names:
The WellKnownPipelineTags constants are now properly used as tags for categorization rather than as step names. Fixed in cf743d0. |
|
@copilot There's a merge conflict DistributedApplicationPipelineTests.cs in main and the one in this branch. Resolve the merge conflicts with main. |
Resolved the merge conflict in DistributedApplicationPipelineTests.cs. Merged main branch and kept both sets of tests:
All 64 tests now passing. Fixed in ce6b7d9. |
ce6b7d9 to
ceba773
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12293Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12293" |
- Rename WellKnownPipelineSteps to WellKnownPipelineTags (keep old class for backward compatibility) - Add Tags property to PipelineStep - Create PipelinePassAnnotation and PipelinePassContext for second-pass callbacks - Implement step-to-resource mapping during step collection - Add AddSecondPassCallback API to IDistributedApplicationPipeline - Add extension methods for registering second-pass callbacks on resources - Add helper methods to find steps by tag, resource, or both Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
- Test Tags property on PipelineStep - Test second-pass callback execution and step dependency modification - Test finding steps by tag, resource, and tag+resource combination - Test WithPipelinePassCallback extension methods (sync and async) - Test resource capture in second-pass callbacks - Test complex dependency relationships using tags - Test WellKnownPipelineTags constants and backward compatibility - Ensure second-pass callbacks execute even when no steps exist Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
- Rename WellKnownPipelineSteps to WellKnownPipelineTags (removed obsolete class) - Rename PipelinePassAnnotation to PipelineConfigurationAnnotation - Rename PipelinePassContext to PipelineConfigurationContext - Rename AddSecondPassCallback to AddPipelineConfiguration - Rename WithPipelinePassCallback to WithPipelineConfiguration - Remove Resource property from context, add ApplicationModel property - Update all tests to use new API - Remove CS0618 pragma warnings (no longer needed) Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
- Fix build errors in playground AppHost (use WellKnownPipelineTags) - Rename ApplicationModel to Model for consistency with PipelineContext - Rename Find* methods to GetSteps with overloads (tag/resource/both) - Update StepToResourceMap to use required initialization pattern - Update all tests to use new API naming Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
- Changed provision step name from WellKnownPipelineTags.ProvisionInfrastructure to "provision-azure-bicep-resources" with tag - Changed build step name from WellKnownPipelineTags.BuildCompute to "build-container-images" with tag - Changed deploy step name from WellKnownPipelineTags.DeployCompute to "deploy-compute-resources" with tag - Step names are now unique identifiers while tags are used for categorization Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
- Merged latest changes from main branch - Resolved merge conflict in DistributedApplicationPipelineTests.cs - Kept both sets of tests: tagging/configuration tests and step filtering tests - All 64 tests passing (60 original + 4 new from main) Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
This reverts commit ce6b7d9.
ceba773 to
7a6fd89
Compare
This reverts commit 7a6fd89.
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 implements pipeline step tagging and configuration phase dependency management by renaming WellKnownPipelineSteps to WellKnownPipelineTags, adding a Tags property to pipeline steps, and introducing a configuration phase that allows modification of step dependencies after initial collection.
Key Changes:
- Renamed
WellKnownPipelineStepstoWellKnownPipelineTagsto reflect that these are categorization tags, not step names - Added
Tagsproperty toPipelineStepfor categorizing steps - Implemented configuration phase with
AddPipelineConfiguration()andWithPipelineConfiguration()APIs - Created
PipelineConfigurationContextwith methods to query steps by tag, resource, or both - Updated step names in
AzureEnvironmentResourceto be descriptive and unique, using tags for categorization
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| WellKnownPipelineTags.cs | Renamed class and updated documentation to clarify these are tags for categorization, not step names |
| PipelineStep.cs | Added Tags property for step categorization |
| PipelineConfigurationContext.cs | New context class providing APIs to query steps and access model/services |
| PipelineConfigurationAnnotation.cs | New annotation for resource-level configuration callbacks |
| PipelineStepExtensions.cs | Added WithPipelineConfiguration() extension methods for resources |
| IDistributedApplicationPipeline.cs | Added AddPipelineConfiguration() method to interface |
| DistributedApplicationPipeline.cs | Implemented configuration phase execution and step-to-resource mapping |
| AzureEnvironmentResource.cs | Updated to use unique step names with proper tagging |
| DistributedApplicationPipelineTests.cs | Added 13 comprehensive tests for new functionality |
| Pipelines.Library/DistributedApplicationPipelineExtensions.cs | Updated reference from WellKnownPipelineSteps to WellKnownPipelineTags |
| Pipelines.AppHost/AppHost.cs | Updated references from WellKnownPipelineSteps to WellKnownPipelineTags |
| // Execute configuration callbacks even if there are no steps | ||
| // This allows callbacks to run validation or other logic | ||
| await ExecuteConfigurationCallbacksAsync(context, allSteps, stepToResourceMap).ConfigureAwait(false); |
Copilot
AI
Oct 23, 2025
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.
Remove the explanatory comments about executing configuration callbacks. The method name ExecuteConfigurationCallbacksAsync is self-documenting, and the comment simply restates what the code does rather than explaining why or providing useful context.
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.
LGTM. I just had some minor comments.
| /// </summary> | ||
| public required DistributedApplicationModel Model { get; init; } | ||
|
|
||
| internal IReadOnlyDictionary<PipelineStep, IResource> StepToResourceMap { get; init; } = null!; |
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.
| internal IReadOnlyDictionary<PipelineStep, IResource> StepToResourceMap { get; init; } = null!; | |
| internal required IReadOnlyDictionary<PipelineStep, IResource> StepToResourceMap { get; init; } |
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.
This is unfortunate that it is internal, but the ctor is public. If we want to keep it that way, we should reall make this:
| internal IReadOnlyDictionary<PipelineStep, IResource> StepToResourceMap { get; init; } = null!; | |
| internal IReadOnlyDictionary<PipelineStep, IResource>? StepToResourceMap { get; init; } |
|
@copilot Open a new PR and address the feedback in #12293 (comment). |
Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
… safety (#12320) * Initial plan * Make StepToResourceMap nullable to address PR #12293 feedback Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
Pipeline Steps Tagging Support - MERGED WITH MAIN ✅
Successfully merged with main branch! All conflicts resolved.
Latest Changes
Commit cf743d0 - Azure Environment Resource:
Commit [current] - Merge with main:
Implementation Summary
✅ 1. WellKnownPipelineTags
✅ 2. Pipeline Configuration Phase
AddPipelineConfiguration()for registering callbacksWithPipelineConfiguration()for resource-level configuration✅ 3. PipelineConfigurationContext API
Services- IServiceProvider for dependency resolutionSteps- IReadOnlyList of all collected stepsModel- Access to DistributedApplicationModel (consistent naming)GetSteps()overloads for finding steps by tag, resource, or both✅ 4. Proper Usage of Tags
✅ 5. Test Coverage
Example Usage
Status: Ready for re-review ✅
Original prompt
Fixes #12289
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.