-
Notifications
You must be signed in to change notification settings - Fork 763
Register IPipelineOutputService in DI for pipeline output directory management #12563
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
Conversation
…ext level Co-authored-by: davidfowl <[email protected]>
|
I have two disagreements with this change:
|
|
@copilot Take safia's feedback and expose: Yeah, something like IPipelineOutputService that exposes methods for getting each directory type |
- Create IPipelineOutputService interface with GetOutputDirectory() and GetTempDirectory() methods - Implement PipelineOutputService with better naming than IntermediateOutputPath - Expose service via PipelineContext.OutputService and PipelineStepContext.OutputService - Keep backward compatible OutputPath property for existing consumers - Remove direct IntermediateOutputPath property per feedback Co-authored-by: davidfowl <[email protected]>
Implemented in 106cfb6. Created |
|
@copilot On IPipelineOutputService make it possible to get the output directory and temp directory for a specific resource |
- Add GetOutputDirectory(IResource) overload for resource-specific output directories - Add GetTempDirectory(IResource) overload for resource-specific temp directories - Implementation creates subdirectories using resource.Name under base directories - Enables per-resource isolation of output and temp artifacts Co-authored-by: davidfowl <[email protected]>
Implemented in e0d5ffd. Added |
|
@copilot Remove the OutputPath from PipelineContext and OutputService. Any step that needs it can resolve it from IPipelineOutputService explicitly. |
- Remove OutputPath convenience property per @davidfowl feedback - Consumers now explicitly call OutputService.GetOutputDirectory() - Update AzureEnvironmentResource to use OutputService.GetOutputDirectory() - Update PublishingContextUtils to use OutputService.GetOutputDirectory() - Update test helper to use OutputService.GetOutputDirectory() - Keep OutputService property for explicit access to output management Co-authored-by: davidfowl <[email protected]>
Implemented in a650bd9. Removed |
| /// Gets the service for managing pipeline output directories. | ||
| /// </summary> | ||
| public string? OutputPath { get; } = outputPath; | ||
| public IPipelineOutputService OutputService { get; } = new PipelineOutputService(outputPath, serviceProvider.GetRequiredService<IConfiguration>()); |
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 remove this from PipelineContext
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.
Implemented in a481f66. Removed OutputService from PipelineContext and registered IPipelineOutputService in the DI container instead. Steps now explicitly resolve the service via context.Services.GetRequiredService<IPipelineOutputService>() when needed. Also removed the outputPath parameter from PipelineContext constructor since it's now handled by the DI-registered service.
- Register IPipelineOutputService in DI container with PipelineOptions - Remove OutputService property from PipelineContext and PipelineStepContext - Remove outputPath parameter from PipelineContext constructor - Update all consumers to resolve IPipelineOutputService from Services - AzureEnvironmentResource - PublishingContextUtils - JsonDocumentManifestPublisher test helper - Remove IOptions<PipelineOptions> dependency from PipelineExecutor - Steps now explicitly resolve IPipelineOutputService when needed Co-authored-by: davidfowl <[email protected]>
|
@copilot PublishingContextUtils should use GetOutputDirectory(environment) when there's a compute environment. |
…specific overload - PipelineOutputService now injects IOptions<PipelineOptions> and IConfiguration directly in constructor - Simplified DI registration to use standard AddSingleton without factory lambda - PublishingContextUtils uses GetOutputDirectory(environment) for resource-specific paths - Removes manual path combination logic in favor of service method Co-authored-by: davidfowl <[email protected]>
Implemented in da9a619. |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12563Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12563" |
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 refactors the pipeline output path handling by introducing a new IPipelineOutputService to centralize output directory management. Previously, the output path was passed through the PipelineContext constructor and stored as a property. Now, a dedicated service manages output paths, temporary directories, and resource-specific paths, providing better encapsulation and defaulting behavior.
Key Changes
- Introduced
IPipelineOutputServiceinterface andPipelineOutputServiceimplementation for centralized output directory management - Removed
OutputPathproperty fromPipelineContextandPipelineStepContext - Updated all consumers to retrieve output paths via
IPipelineOutputService.GetOutputDirectory() - Modified CLI to only pass output path when explicitly provided (previously defaulted to "aspire-output" in CLI)
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting/Pipelines/IPipelineOutputService.cs |
New interface defining output directory and temp directory methods |
src/Aspire.Hosting/Pipelines/PipelineOutputService.cs |
Implementation providing default output directory logic and temp directory management |
src/Aspire.Hosting/Pipelines/PipelineContext.cs |
Removed outputPath parameter and OutputPath property |
src/Aspire.Hosting/Pipelines/PipelineStepContext.cs |
Removed OutputPath property that delegated to PipelineContext |
src/Aspire.Hosting/Publishing/PipelineExecutor.cs |
Removed dependency on IOptions<PipelineOptions> and outputPath construction |
src/Aspire.Hosting/DistributedApplicationBuilder.cs |
Registered IPipelineOutputService as singleton |
src/Shared/PublishingContextUtils.cs |
Updated to use IPipelineOutputService for environment-specific output paths |
src/Aspire.Hosting.Azure/AzureEnvironmentResource.cs |
Changed to use output service instead of context property, removed error validation |
src/Aspire.Hosting.Docker/DockerComposePublishingContext.cs |
Removed null check that threw exception for OutputPath |
src/Aspire.Hosting.Kubernetes/KubernetesPublishingContext.cs |
Removed null check that threw exception for OutputPath |
src/Aspire.Cli/Commands/PublishCommand.cs |
Only passes output path when explicitly provided instead of defaulting |
tests/Aspire.Hosting.Tests/Pipelines/DistributedApplicationPipelineTests.cs |
Updated test helper to use new PipelineContext constructor |
tests/Aspire.Hosting.Tests/Helpers/JsonDocumentManifestPublisher.cs |
Updated to retrieve manifest path via output service |
| _tempDirectory = new Lazy<string>(() => CreateTempDirectory(configuration)); | ||
| } | ||
|
|
||
| /// <inheritdoc/> |
Copilot
AI
Oct 31, 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.
The default fallback path 'aspire-output' should be documented in the XML documentation for the GetOutputDirectory() method. Currently, the documentation only states 'Gets the output directory for deployment artifacts' without mentioning that it defaults to {CurrentDirectory}/aspire-output when no output path is configured.
| /// <inheritdoc/> | |
| /// <summary> | |
| /// Gets the output directory for deployment artifacts. | |
| /// If no output path is configured, defaults to <c>{CurrentDirectory}/aspire-output</c>. | |
| /// </summary> |
| { | ||
| private readonly string? _outputPath; |
Copilot
AI
Oct 31, 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.
These private fields lack XML documentation comments. Per the XML documentation standards, internal implementation fields should have brief summary tags explaining their purpose, especially for a service class like this.
| { | |
| private readonly string? _outputPath; | |
| { | |
| /// <summary> | |
| /// Stores the resolved output directory path, or <c>null</c> if not specified. | |
| /// </summary> | |
| private readonly string? _outputPath; | |
| /// <summary> | |
| /// Lazily creates and stores the path to the temporary directory for pipeline output. | |
| /// </summary> |
| private static string CreateTempDirectory(IConfiguration configuration) | ||
| { | ||
| var appHostSha = configuration["AppHost:PathSha256"]; | ||
|
|
||
| if (!string.IsNullOrEmpty(appHostSha)) | ||
| { | ||
| return Directory.CreateTempSubdirectory($"aspire-{appHostSha}").FullName; | ||
| } | ||
|
|
||
| // Fallback if AppHost:PathSha256 is not available | ||
| return Directory.CreateTempSubdirectory("aspire").FullName; | ||
| } |
Copilot
AI
Oct 31, 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.
The purpose of using AppHost:PathSha256 for temp directory naming is not documented. Add an XML comment explaining why this SHA is used (likely for isolation between different app hosts) and what happens when it's not available.
Co-authored-by: Copilot <[email protected]>
| /// <summary> | ||
| /// Service for managing pipeline output directories. | ||
| /// </summary> | ||
| [Experimental("ASPIREPIPELINES001", UrlFormat = "https://aka.ms/aspire/diagnostics/{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.
| [Experimental("ASPIREPIPELINES001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | |
| [Experimental("ASPIREPIPELINES004", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] |
Let's create a new code for this to stop overloading on the code for the core APIs.
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 Update the diagnostic ID introduced in this Experimental attribute.
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.
Implemented in c01acb7. Updated the diagnostic ID to ASPIREPIPELINES004 for both IPipelineOutputService and PipelineOutputService. Also added comprehensive XML documentation including default behavior for GetOutputDirectory(), field purposes, and the AppHost:PathSha256 isolation mechanism for temp directories.
- Update Experimental attribute from ASPIREPIPELINES001 to ASPIREPIPELINES004 - Add XML documentation for GetOutputDirectory() default behavior - Add XML documentation for private fields in PipelineOutputService - Document purpose of AppHost:PathSha256 usage in CreateTempDirectory - Suppress ASPIREPIPELINES004 diagnostic in all consuming files Co-authored-by: captainsafia <[email protected]>
|
/backport to release/13.0 |
|
Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/18988279905 |
|
|
||
| if (!string.IsNullOrEmpty(appHostSha)) | ||
| { | ||
| return Directory.CreateTempSubdirectory($"aspire-{appHostSha}").FullName; |
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.
How does this directory get cleaned up?
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.
It doesnt' yet (same as all of the other temp directories today in aspire), We don't even us this yet unfortunately. Just flowing it through the system.
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.
Should we make it implement IDisposable and delete the directory when it gets disposed?
Summary
This PR implements the fix for default output path handling during aspire deploy/publish, based on PR #12414 but retaining PublishingContextUtils as requested.
Key Changes
IPipelineOutputService- Service pattern for managing pipeline output directoriesGetOutputDirectory()/GetOutputDirectory(IResource)- Returns output directory for deployment artifacts (defaults toaspire-output)GetTempDirectory()/GetTempDirectory(IResource)- Returns temporary directory for build artifactsPipelineOutputServiceinjectingIOptions<PipelineOptions>andIConfigurationcontext.Services.GetRequiredService<IPipelineOutputService>()PublishingContextUtils(unlike PR Set default OutputPath and add IntermediateOutputPath at PipelineContext level #12414) - uses resource-specific service methodsTesting Results
✅ CLI tests (DeployCommand): 6/6 passed
✅ Docker tests: 56/56 passed
✅ Kubernetes tests: 10/10 passed
Addresses feedback from @captainsafia and @davidfowl for service-based design with explicit DI resolution, resource-specific directory support, and comprehensive documentation.
Fixes #12412
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.