-
Notifications
You must be signed in to change notification settings - Fork 745
Fix pnpm Dockerfile generation by enabling pnpm via corepack #13267
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
Co-authored-by: davidfowl <[email protected]>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13267Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13267" |
|
@copilot As part of this test, attempt to add an end to end test in this vein #12898: Proposal We want to verify the end to end. These tests will have [RequireDocker] |
Co-authored-by: davidfowl <[email protected]>
Added an end-to-end test
The test is marked with |
| if (packageManager.ExecutableName == "pnpm") | ||
| { | ||
| builderStage.Run("corepack enable pnpm"); |
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 shouldn't be special casing on the Executable name. Instead we should add an extension mechanism to the JavaScriptPackageManagerAnnotation that allows any package manager to insert "preinstall" commands to the dockerfile.
On the JavaScriptPackageManagerAnnotation class, add a new callback named InitializeDockerBuildStage that takes a DockerfileStage. This should be invoked in both places we create dockerfiles, even if there is no JavaScriptInstallCommandAnnotation.
The WithPnpm method then customizes the annotations InitializeDockerBuildStage to add corepack enable pnpm.
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.
Refactored in commit d48f459:
- Added
InitializeDockerBuildStagecallback property toJavaScriptPackageManagerAnnotation - Invoked the callback in both Dockerfile creation paths (AddNodeApp and CreateDefaultJavaScriptAppBuilder) after WORKDIR is set
- Removed the special-case check for "pnpm" from
AddInstallCommand - Updated
WithPnpmto set theInitializeDockerBuildStagecallback:stage => stage.Run("corepack enable pnpm")
The callback is now invoked right after WORKDIR /app is set, before any file operations or package installation.
…er setup - Add InitializeDockerBuildStage callback to JavaScriptPackageManagerAnnotation - Invoke callback in both Dockerfile creation paths after WORKDIR - Move pnpm corepack logic from AddInstallCommand to WithPnpm method - Update test snapshots to reflect new command order Co-authored-by: eerhardt <[email protected]>
...pt.Tests/Snapshots/AddJavaScriptAppTests.VerifyPnpmDockerfile_hasLockFile=False.verified.txt
Show resolved
Hide resolved
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 fixes pnpm-based JavaScript app Dockerfile generation by adding support for package manager-specific initialization commands. The issue was that pnpm is not installed by default in Node.js Docker images and must be explicitly enabled via corepack enable pnpm.
Key Changes:
- Added an extensible callback mechanism to
JavaScriptPackageManagerAnnotationfor package managers to inject Docker build stage setup commands - Implemented the callback for pnpm to run
corepack enable pnpmduring Dockerfile generation - Added comprehensive end-to-end test that builds a Docker image to verify the fix works
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting.JavaScript/JavaScriptPackageManagerAnnotation.cs |
Added InitializeDockerBuildStage callback property (experimental API) to allow package managers to inject setup commands |
src/Aspire.Hosting.JavaScript/JavaScriptHostingExtensions.cs |
Invoked the initialization callback in both Dockerfile generation code paths (AddNodeApp and CreateDefaultJavaScriptAppBuilder); set the callback for pnpm to enable corepack |
tests/Aspire.Hosting.JavaScript.Tests/AddJavaScriptAppTests.cs |
Added VerifyPnpmDockerfileBuildSucceeds outerloop test that builds a Docker image to verify the generated Dockerfile works |
tests/Aspire.Hosting.JavaScript.Tests/Snapshots/AddJavaScriptAppTests.VerifyPnpmDockerfile_hasLockFile=True.verified.txt |
Updated snapshot to include the RUN corepack enable pnpm command |
tests/Aspire.Hosting.JavaScript.Tests/Snapshots/AddJavaScriptAppTests.VerifyPnpmDockerfile_hasLockFile=False.verified.txt |
Updated snapshot to include the RUN corepack enable pnpm command |
| /// <summary> | ||
| /// Gets or sets a callback to initialize the Docker build stage before installing packages. | ||
| /// This can be used to add package manager-specific setup commands to the Dockerfile. | ||
| /// </summary> |
Copilot
AI
Dec 3, 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 XML documentation for InitializeDockerBuildStage should follow the internal API documentation guidelines. Since this property is marked with [Experimental] and is intended for internal use within the Dockerfile generation mechanism, the documentation should be more concise without the verbose "This can be used..." phrase.
Consider simplifying to:
/// <summary>
/// Gets or sets a callback to initialize the Docker build stage before installing packages.
/// </summary>The detailed explanation about package manager-specific setup commands is better suited as inline comments where the callback is invoked (which is already done well in the implementation).
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 apply changes based on this feedback
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.
Simplified in commit 0967907. Removed the verbose "This can be used..." phrase to align with internal API documentation guidelines.
Co-authored-by: eerhardt <[email protected]>
|
@radical we added an outer loop test here. |
corepack enable pnpmRUN corepack enable pnpmbefore pnpm commands in Dockerfile generationOriginal prompt
This section details on the original issue you should resolve
<issue_title>WithPnpm breaks PublishAsDockerFile</issue_title>
<issue_description>### Is there an existing issue for this?
Describe the bug
.WithPnpm()plus.PublishAsDockerFile()causesaspire deployto fail.Other package managers, including
.WithNpm()and.WithYarn()succeed.There is an existing test that checks for a generated Dockerfile when using
.WithPnpm(), but I believe the test does not check if the Dockerfile runs.Expected Behavior
aspire deployShould deploy successfully, including successfully building the generated docker files.
Steps To Reproduce
Please see the README.md for https://github.com/yoDon/aspire-bug-report-pnpm
Steps:
Expected Behavior:
✓ PIPELINE SUCCEEDEDActual Behavior:
✗ PIPELINE FAILEDExceptions (if any)
.NET Version info
.NET SDK:
Version: 10.0.100
Commit: b0f34d51fc
Workload version: 10.0.100-manifests.4c0ca8ba
MSBuild version: 18.0.2+b0f34d51f
Runtime Environment:
OS Name: Mac OS X
OS Version: 15.3
OS Platform: Darwin
RID: osx-arm64
Base Path: /usr/local/share/dotnet/sdk/10.0.100/
.NET workloads installed:
There are no installed workloads to display.
Configured to use workload sets when installing new manifests.
No workload sets are installed. Run "dotnet workload restore" to install a workload set.
Host:
Version: 10.0.0
Architecture: arm64
Commit: b0f34d51fc
.NET SDKs installed:
6.0.400 [/usr/local/share/dotnet/sdk]
6.0.404 [/usr/local/share/dotnet/sdk]
6.0.405 [/usr/local/share/dotnet/sdk]
7.0.101 [/usr/local/share/dotnet/sdk]
7.0.102 [/usr/local/share/dotnet/sdk]
8.0.100 [/usr/local/share/dotnet/sdk]
8.0.201 [/usr/local/share/dotnet/sdk]
9.0.308 [/usr/local/share/dotnet/sdk]
10.0.100 [/usr/local/share/dotnet/sdk]
.NET runtimes installed:
Microsoft.AspNetCore.App 6.0.8 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 6.0.12 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 6.0.13 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.1 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.2 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.0 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.2 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 9.0.11 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 10.0.0 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.NETCore.App 6.0.8 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.12 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.13 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.1 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.2 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.0 [/usr/local/share/dotnet/sh...
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.