-
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
Changes from 8 commits
edf8260
e1aa82a
106cfb6
e0d5ffd
a650bd9
a481f66
da9a619
95f604b
c01acb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Diagnostics.CodeAnalysis; | ||
| using Aspire.Hosting.ApplicationModel; | ||
|
|
||
| namespace Aspire.Hosting.Pipelines; | ||
|
|
||
| /// <summary> | ||
| /// Service for managing pipeline output directories. | ||
| /// </summary> | ||
| [Experimental("ASPIREPIPELINES001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| public interface IPipelineOutputService | ||
| { | ||
| /// <summary> | ||
| /// Gets the output directory for deployment artifacts. | ||
| /// </summary> | ||
| /// <returns>The path to the output directory for deployment artifacts.</returns> | ||
| string GetOutputDirectory(); | ||
|
|
||
| /// <summary> | ||
| /// Gets the output directory for a specific resource's deployment artifacts. | ||
| /// </summary> | ||
| /// <param name="resource">The resource to get the output directory for.</param> | ||
| /// <returns>The path to the output directory for the resource's deployment artifacts.</returns> | ||
| string GetOutputDirectory(IResource resource); | ||
|
|
||
| /// <summary> | ||
| /// Gets a temporary directory for build artifacts. | ||
| /// </summary> | ||
| /// <returns>The path to a temporary directory for build artifacts.</returns> | ||
| string GetTempDirectory(); | ||
|
|
||
| /// <summary> | ||
| /// Gets a temporary directory for a specific resource's build artifacts. | ||
| /// </summary> | ||
| /// <param name="resource">The resource to get the temporary directory for.</param> | ||
| /// <returns>The path to a temporary directory for the resource's build artifacts.</returns> | ||
| string GetTempDirectory(IResource resource); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,68 @@ | ||||||||||||||||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||||||||||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| using System.Diagnostics.CodeAnalysis; | ||||||||||||||||||||||
| using Aspire.Hosting.ApplicationModel; | ||||||||||||||||||||||
| using Microsoft.Extensions.Configuration; | ||||||||||||||||||||||
| using Microsoft.Extensions.Options; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| namespace Aspire.Hosting.Pipelines; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||
| /// Default implementation of <see cref="IPipelineOutputService"/>. | ||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||
| [Experimental("ASPIREPIPELINES001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||||||||||||||||||||||
| internal sealed class PipelineOutputService : IPipelineOutputService | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| private readonly string? _outputPath; | ||||||||||||||||||||||
|
Comment on lines
16
to
20
|
||||||||||||||||||||||
| { | |
| 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> |
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> |
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?
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.
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.
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
ASPIREPIPELINES004for bothIPipelineOutputServiceandPipelineOutputService. Also added comprehensive XML documentation including default behavior forGetOutputDirectory(), field purposes, and the AppHost:PathSha256 isolation mechanism for temp directories.