fix(build): always give go-dockerclient an output stream - #193
Conversation
The vendored fsouza/go-dockerclient rejects BuildImage calls with a nil OutputStream before sending anything to the daemon, but dockercrtclient only set one when showBuildLogs was requested. A plain 'mint build' without --show-blogs therefore failed immediately with 'missing output stream'. Always buffer the build output into ref.buildLog, the same pattern slimbuilder.go already uses. The regression test drives the real BuildImage against the local Docker daemon (skipped when no daemon is reachable) with exactly the options shape buildFatImage constructs. Fixes mintoolkit#87 Signed-off-by: Eljees <3.14hell@gmail.com>
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by free · Input: 28.8K · Output: 5.2K · Cached: 143.1K |
|
thanks for the PR @Eljees ! |
There was a problem hiding this comment.
🟡 Changes recommended
The new integration test has unsafe cleanup/tagging behavior and the build-log buffer can return stale logs when a caller provides a custom OutputStream.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes mint build / mint slim (fat-image build path) failing immediately with missing output stream when --show-blogs is not used, by ensuring go-dockerclient.BuildImage() always receives a non-nil OutputStream. It also adds a regression test to reproduce and prevent issue #87.
Changes:
- Always provide a non-nil
BuildImageOptions.OutputStreamby buffering output intoref.buildLogwhen the caller doesn’t supply a stream. - Add a live Docker daemon regression test reproducing #87 and asserting the build log is captured.
File summaries
| File | Description |
|---|---|
| pkg/crt/docker/dockercrtclient/dockercrtclient.go | Ensures Docker build output always has a destination stream to avoid ErrMissingOutputStream. |
| pkg/crt/docker/dockercrtclient/repro_test.go | Adds a regression test exercising the nil-OutputStream + showBuildLogs=false scenario against a real Docker daemon. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if options.OutputStream != nil { | ||
| buildOptions.OutputStream = options.OutputStream | ||
| } else if ref.showBuildLogs { | ||
| } else { | ||
| // go-dockerclient's BuildImage() unconditionally rejects a nil | ||
| // OutputStream (see vendor/github.com/fsouza/go-dockerclient/image.go, | ||
| // ErrMissingOutputStream), so a destination must always be provided, | ||
| // not just when showBuildLogs is requested (mintoolkit/mint#87). | ||
| ref.buildLog.Reset() | ||
| buildOptions.OutputStream = &ref.buildLog | ||
| } |
| dir := t.TempDir() | ||
| dockerfile := "FROM scratch\nCOPY hello.txt /hello.txt\n" | ||
| if err := os.WriteFile(filepath.Join(dir, "Dockerfile"), []byte(dockerfile), 0o644); err != nil { | ||
| t.Fatalf("write Dockerfile: %v", err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(dir, "hello.txt"), []byte("hello\n"), 0o644); err != nil { | ||
| t.Fatalf("write hello.txt: %v", err) | ||
| } | ||
|
|
||
| b := NewBuilder(client, false) // showBuildLogs=false, matches the user's repro command (no --show-blogs) | ||
|
|
||
| opts := imagebuilder.DockerfileBuildOptions{ | ||
| Dockerfile: "Dockerfile", | ||
| BuildContext: dir, | ||
| ImagePath: "el-mint87-repro:latest", | ||
| // OutputStream intentionally left nil - this is the exact shape | ||
| // buildFatImage() in pkg/app/master/command/build/image.go constructs. | ||
| } | ||
|
|
||
| buildErr := b.BuildImage(opts) | ||
| defer client.RemoveImage("el-mint87-repro:latest") | ||
|
|
| func TestBuildImage_MissingOutputStream_Issue87(t *testing.T) { | ||
| client, err := docker.NewClientFromEnv() | ||
| if err != nil { | ||
| t.Fatalf("docker.NewClientFromEnv: %v", err) | ||
| } | ||
| if err := client.Ping(); err != nil { | ||
| t.Skipf("no docker daemon reachable, skipping live repro: %v", err) | ||
| } | ||
|
|
Reset ref.buildLog unconditionally so BuildOutputLog() always reflects the current build, including when the caller supplies its own OutputStream -- previously the buffer was only reset on the showBuildLogs path, so a custom-stream build left the previous build's output readable. The regression test now builds a uniquely named image and removes it only when it actually created one, so it cannot touch a pre-existing image or collide with a parallel run. Signed-off-by: Eljees <3.14hell@gmail.com>
|
Thanks @kcq, and thanks to the reviewer bot — both points were worth acting on. Pushed 7d1b2f7: Stale build log. Correct observation, and it turned out to predate this PR: on Test hygiene. Fixed: the image tag is now unique per run ( Re-verified after the change against a live daemon: the test passes with the fix and still fails with the exact |
|
thanks again for the PR @Eljees |
Fixes #87.
mint build(andmint slimbuilding a fat image first) without--show-blogsfailed immediately withmissing output stream.dockercrtclient.BuildImageonly setbuildOptions.OutputStreamwhenshowBuildLogswas requested, but the vendoredfsouza/go-dockerclientrejects a nilOutputStreamunconditionally (image.go,ErrMissingOutputStream) — before a single request reaches the daemon.The fix always buffers the build output into
ref.buildLog, which is exactly the patternslimbuilder.goalready uses for the slim-image build path;--show-blogskeeps controlling only whether that buffer is printed afterwards.The regression test drives the real
BuildImageagainst the local Docker daemon (auto-skipped when no daemon is reachable) with the same options shapebuildFatImageconstructs — a nilOutputStreamandshowBuildLogs=false. Without the fix it fails with the exact error from the issue; with it, the image builds and the build log is captured. Verified red→green against a live daemon;go vetandgofmtare clean.