Set default OutputPath and add IntermediateOutputPath at PipelineContext level#12414
Set default OutputPath and add IntermediateOutputPath at PipelineContext level#12414
Conversation
- Update PublishingContextUtils.GetEnvironmentOutputPath to use fallback when OutputPath is null - Fallback is Path.Combine(Environment.CurrentDirectory, "aspire-output") - Add comprehensive tests for null and non-null output paths - Add tests for single and multiple environment scenarios Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12414Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12414" |
1 similar comment
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12414Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12414" |
captainsafia
left a comment
There was a problem hiding this comment.
@copilot We need to fix all the of the checks when initializing the output path:
Unify the handling for output path across all these areas.
There was a problem hiding this comment.
Delete this test file.
- Azure: Use fallback to Path.Combine(Environment.CurrentDirectory, "aspire-output") when OutputPath is null - Docker: Remove null check exception since PublishingContextUtils.GetEnvironmentOutputPath guarantees non-null - Kubernetes: Remove null check exception since PublishingContextUtils.GetEnvironmentOutputPath guarantees non-null - Delete PublishingContextUtilsTests.cs as requested Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
Unified output path handling across all three locations in commit 5ce261a:
|
|
@copilot Let's do this generally at the PipelineStepContext level and set a default top level when creating it. Let's also add an |
…utputPath - Set OutputPath default to Path.Combine(Environment.CurrentDirectory, "aspire-output") in PipelineContext constructor - Add IntermediateOutputPath property to PipelineContext, defaulting to Path.Combine(Path.GetTempPath(), "aspire", appHostSha) - IntermediateOutputPath uses IConfiguration["AppHost:PathSha256"] for consistent temporary folder per apphost - Change OutputPath from nullable to non-nullable since it always has a default value - Simplify PublishingContextUtils to use non-nullable OutputPath - Update Azure, Docker, and Kubernetes to use non-nullable OutputPath - Remove redundant null checks and exceptions Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Implemented in commit 7a481ce:
|
…and remove CLI default output path - Use Directory.CreateTempSubdirectory for IntermediateOutputPath instead of manual path construction - Remove PublishingContextUtils class entirely and inline logic where needed - Inline GetEnvironmentOutputPath logic in Docker and Kubernetes environment resources - Remove PublishingContextUtils.cs file and references from .csproj files - Remove special case logic in PublishCommand that sets default output path to "aspire-output" - Output path now defaults in PipelineContext only, not in CLI Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
|
@copilot Redo this change in the face of the merge conflcit |
Rebased all changes onto latest main in commit ffacbc0. All changes have been reapplied cleanly on top of the latest main branch. Tests pass successfully:
The branch is ready but I need assistance force-pushing the rebased branch since I don't have direct push access. |
| /// </summary> | ||
| public string IntermediateOutputPath { get; } = GetIntermediateOutputPath(serviceProvider); | ||
|
|
||
| private static string GetIntermediateOutputPath(IServiceProvider serviceProvider) |
There was a problem hiding this comment.
How does this directory get cleaned up?
There was a problem hiding this comment.
We dont clean anything up. I think it should be relative to your app folder tbh but I just want to flow a property through so we can begin to use it.
| private Task PublishAsync(PipelineStepContext context) | ||
| { | ||
| var outputPath = PublishingContextUtils.GetEnvironmentOutputPath(context, this); | ||
| // Inline logic from PublishingContextUtils.GetEnvironmentOutputPath |
There was a problem hiding this comment.
Why not continue using the shared Utils method?
There was a problem hiding this comment.
yea I didn't rrealize what it did originally.
|
Making a new PR |
Summary: Fix Default Output Path Issue During Aspire Deploy
The issue occurs when
aspire deployoraspire do <step>is called without an explicit--output-path. Previously, various parts of the code assumedOutputPathwould always be set, causing failures.Solution (Rebased on latest main)
Following feedback, the fix now sets defaults at the
PipelineContextlevel and simplifies the code:Default
OutputPathis set inPipelineContextconstructor toPath.Combine(Environment.CurrentDirectory, "aspire-output")when nullAdded
IntermediateOutputPathproperty toPipelineContextfor temporary build artifactsDirectory.CreateTempSubdirectoryto create temp foldersaspire-{appHostSha}usingIConfiguration["AppHost:PathSha256"]aspireRemoved
PublishingContextUtilsclass entirelyRemoved CLI default output path logic
PublishCommandno longer sets a default output path to "aspire-output"PipelineContext, ensuring consistencyChanged
OutputPathfrom nullable to non-nullable since it always has a default valueTesting Results
Files Changed
src/Aspire.Hosting/Pipelines/PipelineContext.cs: Added defaults for OutputPath and IntermediateOutputPath using Directory.CreateTempSubdirectorysrc/Aspire.Hosting/Pipelines/PipelineStepContext.cs: Exposed IntermediateOutputPath, changed OutputPath to non-nullablesrc/Shared/PublishingContextUtils.cs: Deleted - logic inlined where neededsrc/Aspire.Hosting.Azure/AzureEnvironmentResource.cs: Removed exception for null OutputPathsrc/Aspire.Hosting.Docker/DockerComposeEnvironmentResource.cs: Inlined output path logic, removed PublishingContextUtils usagesrc/Aspire.Hosting.Docker/Aspire.Hosting.Docker.csproj: Removed PublishingContextUtils.cs referencesrc/Aspire.Hosting.Kubernetes/KubernetesEnvironmentResource.cs: Inlined output path logic, removed PublishingContextUtils usagesrc/Aspire.Hosting.Kubernetes/Aspire.Hosting.Kubernetes.csproj: Removed PublishingContextUtils.cs referencesrc/Aspire.Hosting.Docker/DockerComposePublishingContext.cs: Removed null check exceptionsrc/Aspire.Hosting.Kubernetes/KubernetesPublishingContext.cs: Removed null check exceptionsrc/Aspire.Hosting/Cli/Commands/PublishCommand.cs: Removed default output path logic (now handled in PipelineContext)The fix ensures consistent and predictable output paths across all publish/deploy commands, with defaults set at a single location in
PipelineContext.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.