Fail on JavaScript run script Dockerfile conflict#16969
Conversation
Detect explicit JavaScript run script configuration during publish when an existing Dockerfile would otherwise supply the container entrypoint. Fail with a clear message instead of silently ignoring runScriptName or WithRunScript, while preserving explicit container entrypoint overrides. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16969Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16969" |
|
@PatrickMatthiesen would appreciate if you can try the dogfood build once it's available to confirm this is fixing the issue you filed. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a publish-time pitfall for AddJavaScriptApp when an existing user-authored Dockerfile is discovered: if the app is configured with an explicit runScriptName / WithRunScript(...), Aspire now fails fast during manifest publishing rather than silently emitting a container that will run the Dockerfile ENTRYPOINT instead of the requested script.
Changes:
- Introduces publish-time validation that detects “existing Dockerfile + explicit run script” conflicts and throws a clear
InvalidOperationException. - Applies the validation consistently when publishing an individual JavaScript resource manifest and when publishing the full model manifest.
- Adds unit tests covering the new failure cases and the allowed cases (implicit default script, explicit entrypoint override via
PublishAsDockerFile).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Aspire.Hosting.JavaScript/JavaScriptHostingExtensions.cs |
Adds validation hooks during manifest publishing to block explicit run-script configuration when publishing uses an existing Dockerfile entrypoint. |
tests/Aspire.Hosting.JavaScript.Tests/AddJavaScriptAppTests.cs |
Adds tests verifying the new failure mode and the scenarios that remain permitted. |
|
Make sure deployment failures happen in steps not during model building |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| private const string BrowserCapability = "browser"; | ||
| private const string DefaultNodeVersion = "22"; | ||
| private const string DefaultJavaScriptRunScriptName = "dev"; | ||
| private const string PublishManifestStepName = "publish-manifest"; |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Oh, nice, thanks for giving it a look so quickly! It sadly didn't fix the issue, or maybe I used the wrong dogfood build? I made a small reproducible example repo, if you need more info or want a way to debug it :) |
|
Check |
|
That seems weird, as the version is reported as being this PR's Dogfood for me: Anything else I should try checking? Btw, did you see the README? I shipped the repo in a working state so you could see that removing either the Dockerfile entrypoint overwrite or the PublishAsDockerFile(...) entirely, breaks the deploy. If I do either of the two things, then the migrations resource fails, and the observer resource never starts: const applyMigrations = builder.addJavaScriptApp('apply-migrations', '..', {
runScriptName: 'migrate:postgres-schema'
}).withBun()
.publishAsDockerFile(async (container) => {
// await container.withEntrypoint('bun');
// await container.withArgs(['migrate:postgres-schema']);
});
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mitchdenny
left a comment
mitchdenny
left a comment
There was a problem hiding this comment.
Reviewed the implementation and reproduced two UX issues by running the PR build against a small AppHost (aspire publish --publisher manifest). Two inline comments below.
The overall fix works: explicit runScriptName with an existing user-authored Dockerfile correctly fails publish with a clear exception, and the escape hatch (PublishAsDockerFile(c => c.WithEntrypoint(...))) is honored.
| var hasExplicitRunScript = | ||
| !string.Equals(initialRunScriptName, DefaultJavaScriptRunScriptName, StringComparison.Ordinal) || | ||
| hasMultipleRunScriptAnnotations || | ||
| runScript.Args.Length > 0; |
There was a problem hiding this comment.
hasMultipleRunScriptAnnotations is true whenever WithRunScript(...) is called after AddJavaScriptApp, because CreateDefaultJavaScriptAppBuilder itself already adds one annotation at line 876. As a result, this code throws even when the user's explicit WithRunScript matches the default:
builder.AddJavaScriptApp("js", appDir)
.WithBun()
.WithRunScript("dev"); // same as the defaultRepro (verified against this PR build with dotnet run -- --publisher manifest): publish fails with JavaScript app resource 'js' is configured to run script 'dev', but publish is using the existing Dockerfile .... The user explicitly affirmed the default value, so the failure is surprising.
Suggested fix: compare the last JavaScriptRunScriptAnnotation's ScriptName (and Args) against the default instead of treating annotation count alone as a proxy for "explicit".
| throw new DistributedApplicationException( | ||
| $"JavaScript app resource '{resource.Name}' is configured to run script '{runScript.ScriptName}', but publish is using the existing Dockerfile '{dockerfileBuildAnnotation.DockerfilePath}'. " + | ||
| "An existing Dockerfile entrypoint cannot be changed automatically from runScriptName or WithRunScript. " + | ||
| "Remove or rename the Dockerfile so Aspire can generate one, or call PublishAsDockerFile(...) and set the container entrypoint explicitly."); |
There was a problem hiding this comment.
hasExplicitRunScript can be set purely by runScript.Args.Length > 0, even when runScript.ScriptName equals the default "dev". But the thrown message only mentions runScriptName and WithRunScript (the script name), not the args. So this:
builder.AddJavaScriptApp("js", appDir) // default script: "dev"
.WithBun()
.WithRunScript("dev", new[] { "--port", "8080" });fails with ... is configured to run script 'dev', but publish is using the existing Dockerfile ... — no mention of the args, which are the actual reason for the failure. Verified locally: the message produced for the args-only case is identical to the case where the user passed a non-default script name.
Suggested fix: when the trigger is runScript.Args.Length > 0, include the args in the message (e.g. ... is configured to run script 'dev' with args [--port, 8080], ...).
|
Tested this PR with the dogfood CLI
Error text from S1 is good — it explains why it failed and gives two unambiguous fixes (remove/rename the Dockerfile, or call Code review: nothing blocking. (One reviewer flagged the two |
|
This needs a pr-testing report. |
- Detect 'explicit run script' by inspecting the last
JavaScriptRunScriptAnnotation's ScriptName and Args rather than
counting annotations. This avoids a false positive when the user
re-states the default explicitly (e.g. .WithRunScript("dev")).
- When the run-script args are the trigger for the conflict, include
them in the thrown message so users can see why a default-named
script still produced a conflict.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ix-javascriptapp-entrypoint # Conflicts: # tests/Aspire.Hosting.JavaScript.Tests/AddJavaScriptAppTests.cs
mitchdenny
left a comment
mitchdenny
left a comment
There was a problem hiding this comment.
LGTM — patched the two issues I flagged earlier (overly aggressive heuristic + missing args context in error message) directly on this branch, all 137 JS hosting tests pass locally, and CI is green for everything related to this change.
The only red job (Azure.Messaging.EventHubs / windows-latest) is an unrelated infra flake: failed: 0 across all TFMs with a hang-dump detection at the job level. Nothing in this PR touches Azure or EventHubs.
|
Lets verify #16969 (comment) |
|
Before we continue on this PR, I asked Eric to verify and he commented on the issue directly that it's the documented behavior: #16957 (comment) |
Independent repro: PatrickMatthiesen/dockerfile-aspire-bug ✅I cloned PatrickMatthiesen/dockerfile-aspire-bug and ran his README repro against this PR's CLI build. The PR catches the silent-failure scenario cleanly. Setup
Test 1 — README Step 1 (working state, both apps override entrypoint via
|
|
❓ CLI E2E Tests unknown — 92 passed, 0 failed, 2 unknown (commit View all recordings
📹 Recordings uploaded automatically from CI run #26079523876 |
When AddJavaScriptApp is configured with an explicit runScriptName or WithRunScript and an existing user-authored Dockerfile is present, Aspire now fails at publish time with a clear error instead of silently ignoring the requested run script. Document the conflict, the error message, and the two resolution paths: remove/rename the Dockerfile, or use PublishAsDockerFile with an explicit container entrypoint. Documents changes from microsoft/aspire#16969. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pull request created: #1014
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#1014 targeting Added a new subsection "Run script and existing Dockerfile conflict" to
Note This draft PR needs human review before merging. |
Description
When
AddJavaScriptAppis published with an existing Dockerfile, an explicitrunScriptNameor laterWithRunScript(...)can be silently ignored because the Dockerfile entrypoint wins. This change fails fast with a clear error when Aspire cannot safely apply the requested run script to a user-authored Dockerfile.The validation is applied to both the original JavaScript executable resource and the replacement container resource used during full manifest publishing. It still allows the implicit default run script and explicit
PublishAsDockerFile(...)container entrypoint overrides.Fixes: #16957
Checklist
<remarks />and<code />elements on your triple slash comments?