Surface build output on failure for Docker, Podman, and dotnet publish#16093
Surface build output on failure for Docker, Podman, and dotnet publish#16093
Conversation
Introduce ProcessFailedException that captures stdout/stderr from failed build processes. The exception's Message property includes the last 50 lines of output so it surfaces through the pipeline step reporter without requiring --log-level debug. All three container build paths now throw ProcessFailedException consistently: - Docker buildx (deduped to use base class ExecuteContainerCommand) - Podman build - dotnet publish /t:PublishContainer Uses ConcurrentQueue<string> for thread-safe output buffering since OnOutputData/OnErrorData fire on different threads. 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 -- 16093Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16093" |
There was a problem hiding this comment.
Pull request overview
This PR improves diagnostics for container image builds by capturing and surfacing build stdout/stderr when Docker, Podman, or dotnet publish /t:PublishContainer fails, so users can see actionable error output without re-running with --log-level debug.
Changes:
- Added
ProcessFailedExceptionto carry exit code plus captured build output and include formatted tail output inMessage. - Updated Docker and Podman build paths (including buildkit instance creation) to capture output and throw
ProcessFailedExceptionon non-zero exit codes. - Updated
dotnet publishcontainer build path to capture output and throwProcessFailedException, plus added an integration test asserting build output is present.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/Publishing/ResourceContainerImageManagerTests.cs | Adds Docker build failure integration test validating captured output is present on failure. |
| src/Aspire.Hosting/Publishing/ResourceContainerImageManager.cs | Captures dotnet publish output and throws ProcessFailedException on failure. |
| src/Aspire.Hosting/Publishing/ProcessFailedException.cs | Introduces new exception type that formats and surfaces tail build output in Message. |
| src/Aspire.Hosting/Publishing/PodmanContainerRuntime.cs | Captures Podman build output and throws ProcessFailedException on failure. |
| src/Aspire.Hosting/Publishing/DockerContainerRuntime.cs | Refactors Docker buildx path to shared execution helper, captures output, throws ProcessFailedException (including for buildkit creation failures). |
| src/Aspire.Hosting/Publishing/ContainerRuntimeBase.cs | Extends command execution helper to optionally buffer stdout/stderr lines for callers to include in exceptions. |
Copilot's findings
Comments suppressed due to low confidence (1)
tests/Aspire.Hosting.Tests/Publishing/ResourceContainerImageManagerTests.cs:1457
- This Docker build-failure test uses a remote base image (
mcr.microsoft.com/cbl-mariner/...). If the pull fails (network, throttling, outage), the build may fail before theCOPY nonexistent-file-12345.txtstep and the assertion on that filename can become flaky. Consider usingFROM scratch(or another already-cached test image used elsewhere) so the failure deterministically comes from the missing file.
await File.WriteAllTextAsync(dockerfilePath, """
FROM mcr.microsoft.com/cbl-mariner/base/core:2.0
COPY nonexistent-file-12345.txt /app/
""");
- Files reviewed: 6/6 changed files
- Comments generated: 6
tests/Aspire.Hosting.Tests/Publishing/ResourceContainerImageManagerTests.cs
Outdated
Show resolved
Hide resolved
…gging - Add using to TestTempDirectory in test to prevent temp folder leaks - Add catch/log pattern in BuildProjectContainerImageAsync for consistent error logging across build paths Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The existing BuildImageAsync_ProjectBuildFailureIncludesResourceName test expected InvalidOperationException but now gets ProcessFailedException after our changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JamesNK
left a comment
There was a problem hiding this comment.
1 issue found (correctness). The new exception type won't pass through the pipeline step executor cleanly — it will get wrapped with a redundant step-name prefix instead of surfacing the formatted build output directly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JamesNK
left a comment
There was a problem hiding this comment.
Clean implementation. The bounded BuildOutputCapture with CircularBuffer properly handles thread-safety and memory. ProcessFailedException correctly extends DistributedApplicationException for pipeline pass-through. The ThrowOnNonZeroReturnCode = false fix in ExecuteDotnetPublishAsync is a correctness improvement over the prior code. All three build paths (Docker, Podman, dotnet publish) are updated consistently.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| /// <summary> | ||
| /// Retains a bounded tail of build output while tracking the total number of lines observed. | ||
| /// </summary> | ||
| internal sealed class BuildOutputCapture(int maxRetainedLineCount = 256) |
There was a problem hiding this comment.
Is the last 256 lines enough? Sometimes it gets pretty large.
There was a problem hiding this comment.
It's 0 right now. 😄
| /// <summary> | ||
| /// Returns the last <paramref name="maxLines"/> lines of build output formatted for display. | ||
| /// </summary> | ||
| public string GetFormattedOutput(int maxLines = 50) |
There was a problem hiding this comment.
50 is even smaller than 256. Are we sure this is enough to give all the information to someone?
There was a problem hiding this comment.
In my testing yes. It's also better than 0. You can always set the log level to see it all.
IEvangelist
left a comment
There was a problem hiding this comment.
2 issues found: 1 correctness bug and 1 security issue.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎬 CLI E2E Test Recordings — 71 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24422121976 |
|
📄 Documentation check complete — no docs PR required All changes in this PR are internal implementation details ( The improvement (surfacing build output on failure for Docker, Podman, and
|
Description
Surface build output on failure for Docker, Podman, and dotnet publish container builds.
Problem
When a container image build fails, users see only:
The actual build output (compiler errors, missing files, Dockerfile issues) is logged at Debug level only. Users have to re-run with
--log-level debugto see what went wrong.Solution
Introduce
ProcessFailedExceptionthat captures stdout/stderr from failed build processes. The exception'sMessageproperty includes the last 50 lines of output so it surfaces through the pipeline step reporter automatically.Changes
ProcessFailedException— new internal exception type withExitCode,BuildOutput(raw lines), andGetFormattedOutput().Messageis overridden to include formatted output.ExecuteContainerCommandWithExitCodeAsync, throwsProcessFailedExceptionwith captured outputProcessFailedExceptionwith captured outputProcessFailedExceptionwith captured outputProcessFailedExceptionConcurrentQueue<string>for thread-safe output buffering (callbacks fire on different threads)Before
After
Checklist