-
Notifications
You must be signed in to change notification settings - Fork 769
Set default OutputPath and add IntermediateOutputPath at PipelineContext level #12414
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
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot We need to fix all the of the checks when initializing the output path:
context.OutputPath ?? throw new InvalidOperationException("OutputPath is required for Azure publishing."), - https://github.com/dotnet/aspire/blob/main/src/Aspire.Hosting.Docker/DockerComposePublishingContext.cs#L40
public readonly string OutputPath = outputPath ?? throw new InvalidOperationException("OutputPath is required for Kubernetes publishing.");
Unify the handling for output path across all these areas.
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.
Delete this test 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.
Done in commit 5ce261a.
- 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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not continue using the shared Utils method?
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.
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.