From 9beebb19a6f31244639f016b91662932d5455a7b Mon Sep 17 00:00:00 2001 From: Nafeez Nazer Date: Fri, 11 Sep 2026 11:21:16 -0700 Subject: [PATCH] CC-8654 Make container image build output concise --- .changeset/quiet-container-builds.md | 7 + packages/containers-shared/src/build.ts | 251 ++++++++++++++---- packages/containers-shared/src/images.ts | 1 + packages/containers-shared/src/login.ts | 61 ++++- .../containers-shared/src/process-output.ts | 38 +++ packages/containers-shared/src/utils.ts | 30 ++- .../tests/build-and-push.test.ts | 59 ++++ .../containers-shared/tests/build.test.ts | 101 +++++-- .../containers-shared/tests/login.test.ts | 119 ++++++++- .../containers-shared/tests/utils.test.ts | 48 +++- .../durable-object-container-applications.ts | 1 + ...able-object-container-applications.test.ts | 1 + .../src/__tests__/containers/deploy.test.ts | 53 ++-- .../build-container-images.test.ts | 4 +- .../src/__tests__/preview/containers.test.ts | 3 +- .../build-container-images.ts | 4 +- packages/wrangler/src/preview/containers.ts | 4 +- 17 files changed, 674 insertions(+), 111 deletions(-) create mode 100644 .changeset/quiet-container-builds.md create mode 100644 packages/containers-shared/src/process-output.ts diff --git a/.changeset/quiet-container-builds.md b/.changeset/quiet-container-builds.md new file mode 100644 index 00000000000..30f86d68bc2 --- /dev/null +++ b/.changeset/quiet-container-builds.md @@ -0,0 +1,7 @@ +--- +"wrangler": patch +--- + +Make container image build and push output concise + +Wrangler now shows compact image progress with elapsed time while hiding successful Docker build, login, tag, and push output. Failures retain bounded diagnostics, and `WRANGLER_LOG=debug` restores live Docker output. diff --git a/packages/containers-shared/src/build.ts b/packages/containers-shared/src/build.ts index 610f5d5abcc..77044e693fe 100644 --- a/packages/containers-shared/src/build.ts +++ b/packages/containers-shared/src/build.ts @@ -2,6 +2,8 @@ import { spawn } from "node:child_process"; import crypto from "node:crypto"; import { existsSync, readFileSync } from "node:fs"; import { join } from "node:path"; +import { spinner } from "@cloudflare/cli-shared-helpers/interactive"; +import { formatTime, isNonInteractiveOrCI } from "@cloudflare/workers-utils"; import { getDockerPath } from "@cloudflare/workers-utils/docker-path"; import { UserError } from "@cloudflare/workers-utils/errors"; import { isDirectory } from "@cloudflare/workers-utils/fs-helpers"; @@ -11,6 +13,10 @@ import { dockerImageInspect } from "./inspect"; import { getCloudflareContainerRegistry } from "./knobs"; import { ensureContainerLimits, getContainerAccount } from "./limits"; import { dockerLoginImageRegistry } from "./login"; +import { + createBoundedOutputCollector, + withDockerDebugHint, +} from "./process-output"; import { verifyDockerInstalled } from "./utils"; import { runDockerCmd, runDockerCmdWithOutput } from "./utils"; import type { @@ -34,6 +40,82 @@ export type BuiltContainerImage = BuiltImage & { container: DockerfileContainerConfig; }; +type DockerOutputMode = "capture" | "inherit"; + +function getDockerOutputMode(): DockerOutputMode { + return logger.loggerLevel === "debug" ? "inherit" : "capture"; +} + +async function runImageStep( + startMessage: string, + doneMessage: string, + operation: () => Promise +): Promise { + if ( + logger.loggerLevel !== undefined && + logger.loggerLevel !== "log" && + logger.loggerLevel !== "debug" + ) { + return operation(); + } + + const startedAt = Date.now(); + if (isNonInteractiveOrCI() || getDockerOutputMode() === "inherit") { + logger.log(`${startMessage}...`); + const result = await operation(); + logger.log(`${doneMessage} ${formatTime(Date.now() - startedAt)}`); + return result; + } + + const status = spinner(); + status.start(startMessage); + try { + const result = await operation(); + status.stop(`${doneMessage} ${formatTime(Date.now() - startedAt)}`); + return result; + } catch (error) { + status.stop(); + throw error; + } +} + +function runDockerCommand(pathToDocker: string, args: string[]) { + return runDockerCmd( + pathToDocker, + args, + getDockerOutputMode() === "inherit" ? undefined : ["ignore", "pipe", "pipe"] + ); +} + +function pushDockerImage(pathToDocker: string, imageTag: string) { + return runDockerCommand( + pathToDocker, + getDockerOutputMode() === "inherit" + ? ["push", imageTag] + : ["push", "--quiet", imageTag] + ); +} + +function loginToImageRegistry(pathToDocker: string, domain: string) { + return dockerLoginImageRegistry(pathToDocker, domain, getDockerOutputMode()); +} + +function withPlainProgress(buildCmd: string[]): string[] { + if ( + buildCmd.some( + (argument) => + argument === "--progress" || argument.startsWith("--progress=") + ) + ) { + return buildCmd; + } + + const command = buildCmd[0]; + return command === undefined + ? buildCmd + : [command, "--progress", "plain", ...buildCmd.slice(1)]; +} + export function isDockerfileContainerConfig( container: ContainerNormalizedConfig ): container is DockerfileContainerConfig { @@ -100,22 +182,26 @@ type StartedContainerBuild = Awaited>; * @param verifyDockerIsRunning - When `true` (the default), verifies Docker is installed * and the daemon is running before building. Set to `false` when the caller has already * performed this check. + * @param outputMode - Capture build output, or inherit the terminal for local development. * @returns An object with an `abort` function and a `ready` promise. */ export async function startContainerBuild({ build, pathToDocker, verifyDockerIsRunning, + outputMode, }: { build: BuildArgs; pathToDocker: string; verifyDockerIsRunning?: boolean; + outputMode?: DockerOutputMode; }): Promise { const { buildCmd, dockerfile } = await constructBuildCommand(build); return await dockerBuild(pathToDocker, { buildCmd, dockerfile, verifyDockerIsRunning, + outputMode, }); } @@ -221,6 +307,7 @@ async function tagAndPushImage({ pathToDocker, sourceTag, targetTag, + displayName, externalAccountId, complianceConfig, cleanupSourceTag, @@ -228,6 +315,7 @@ async function tagAndPushImage({ pathToDocker: string; sourceTag: string; targetTag: string; + displayName: string; externalAccountId: string; complianceConfig?: ComplianceConfig; cleanupSourceTag?: boolean; @@ -237,12 +325,18 @@ async function tagAndPushImage({ targetTag, complianceConfig ); - await runDockerCmd(pathToDocker, ["tag", sourceTag, namespacedImageTag]); + await runDockerCommand(pathToDocker, ["tag", sourceTag, namespacedImageTag]); if (cleanupSourceTag) { logger.debug(`Untagging built image: ${sourceTag}.`); - await runDockerCmd(pathToDocker, ["image", "rm", sourceTag]); + await runDockerCommand(pathToDocker, ["image", "rm", sourceTag]); } - await runDockerCmd(pathToDocker, ["push", namespacedImageTag]); + await runImageStep( + `Pushing image ${displayName}`, + `Pushed image ${displayName}`, + async () => { + await pushDockerImage(pathToDocker, namespacedImageTag); + } + ); return namespacedImageTag; } @@ -257,6 +351,7 @@ export async function pushImageIfChanged({ accountId, complianceConfig, cleanupSourceTag, + displayName = targetTag, }: { pathToDocker: string; sourceTag: string; @@ -265,6 +360,7 @@ export async function pushImageIfChanged({ accountId?: string; complianceConfig?: ComplianceConfig; cleanupSourceTag?: boolean; + displayName?: string; }): Promise { /** * Get `RepoDigests`: @@ -291,7 +387,7 @@ export async function pushImageIfChanged({ containerConfig, }); - await dockerLoginImageRegistry( + await loginToImageRegistry( pathToDocker, // Won't be an external registry since this is building from a Dockerfile // rather than specifying an image URI. @@ -331,12 +427,12 @@ export async function pushImageIfChanged({ const parsedRemoteManifest = JSON.parse(remoteManifest); if (parsedRemoteManifest.Descriptor.digest === hash) { - logger.log("Image already exists remotely, skipping push"); + logger.log(`Image ${displayName} is unchanged; reusing existing upload.`); logger.debug( `Untagging built image: ${sourceTag} since there was no change.` ); - await runDockerCmd(pathToDocker, ["image", "rm", sourceTag]); + await runDockerCommand(pathToDocker, ["image", "rm", sourceTag]); return { remoteDigest }; } @@ -348,8 +444,8 @@ export async function pushImageIfChanged({ } } // Re-tag the image to include the account ID. - logger.log( - `Image does not exist remotely, pushing: ${resolveImageName( + logger.debug( + `Pushing image as ${resolveImageName( account.external_account_id, targetTag, complianceConfig @@ -359,6 +455,7 @@ export async function pushImageIfChanged({ pathToDocker, sourceTag, targetTag, + displayName, externalAccountId: account.external_account_id, complianceConfig, cleanupSourceTag, @@ -429,17 +526,24 @@ export async function buildCommand( const pathToDocker = args.pathToDocker ?? getDockerPath(); try { - const build = await startContainerBuild({ - pathToDocker, - build: { - tag: args.tag, - pathToDockerfile, - buildContext: args.PATH, - platform: args.platform, - // No option to add env vars at build time...? - }, - }); - await build.ready; + await runImageStep( + `Building image ${args.tag}`, + `Built image ${args.tag}`, + async () => { + const build = await startContainerBuild({ + pathToDocker, + outputMode: getDockerOutputMode(), + build: { + tag: args.tag, + pathToDockerfile, + buildContext: args.PATH, + platform: args.platform, + // No option to add env vars at build time...? + }, + }); + await build.ready; + } + ); if (args.push) { await pushImageIfChanged({ @@ -447,6 +551,7 @@ export async function buildCommand( sourceTag: args.tag, targetTag: args.tag, complianceConfig, + displayName: args.tag, }); } } catch (error) { @@ -469,7 +574,7 @@ export async function pushCommand( ) { try { const dockerPath = args.pathToDocker ?? getDockerPath(); - await dockerLoginImageRegistry( + await loginToImageRegistry( dockerPath, getCloudflareContainerRegistry(complianceConfig) ); @@ -479,10 +584,11 @@ export async function pushCommand( pathToDocker: dockerPath, sourceTag: args.TAG, targetTag: args.TAG, + displayName: args.TAG, externalAccountId: accountId, complianceConfig, }); - logger.log(`Pushed image: ${newTag}`); + logger.debug(`Pushed image as ${newTag}`); } catch (error) { if (error instanceof Error) { throw new UserError(error.message, { @@ -523,6 +629,7 @@ async function checkImagePlatform( * @param containerConfig - Optional container configuration for limit validation. * @param verifyDockerIsRunning - Whether to verify Docker before building. * @param complianceConfig - Compliance configuration used to select the managed registry. + * @param options - User-facing display options. * @returns An {@link ImageRef} describing the built or pushed image. */ export async function buildAndMaybePush( @@ -531,15 +638,24 @@ export async function buildAndMaybePush( push: boolean, containerConfig?: DockerfileContainerConfig, verifyDockerIsRunning?: boolean, - complianceConfig?: ComplianceConfig + complianceConfig?: ComplianceConfig, + options: { displayName?: string } = {} ): Promise { + const displayName = options.displayName ?? args.tag; try { - const build = await startContainerBuild({ - pathToDocker, - verifyDockerIsRunning, - build: args, - }); - await build.ready; + await runImageStep( + `Building image ${displayName}`, + `Built image ${displayName}`, + async () => { + const build = await startContainerBuild({ + pathToDocker, + verifyDockerIsRunning, + outputMode: getDockerOutputMode(), + build: args, + }); + await build.ready; + } + ); if (!push) { return { newTag: args.tag }; @@ -552,6 +668,7 @@ export async function buildAndMaybePush( containerConfig, complianceConfig, cleanupSourceTag: true, + displayName, }); } catch (error) { if (error instanceof Error) { @@ -574,20 +691,25 @@ async function buildContainerImage( const localTag = `${getContainerImageRepositoryName( containerConfig )}:wrangler-${crypto.randomUUID()}`; - logger.log("Building image", localTag); - try { - const build = await startContainerBuild({ - pathToDocker, - verifyDockerIsRunning, - build: { - tag: localTag, - pathToDockerfile: containerConfig.dockerfile, - buildContext: containerConfig.image_build_context, - args: containerConfig.image_vars, - }, - }); - await build.ready; + await runImageStep( + `Building image ${containerConfig.name}`, + `Built image ${containerConfig.name}`, + async () => { + const build = await startContainerBuild({ + pathToDocker, + verifyDockerIsRunning, + outputMode: getDockerOutputMode(), + build: { + tag: localTag, + pathToDockerfile: containerConfig.dockerfile, + buildContext: containerConfig.image_build_context, + args: containerConfig.image_vars, + }, + }); + await build.ready; + } + ); return { container: containerConfig, localTag }; } catch (error) { @@ -660,6 +782,7 @@ export async function pushBuiltContainerImage( accountId, complianceConfig, cleanupSourceTag: true, + displayName: builtImage.container.name, }); builtImage.localTagCleaned = true; return imageRef; @@ -692,7 +815,11 @@ export async function cleanupBuiltImages( } try { logger.debug(`Untagging built image: ${builtImage.localTag}.`); - await runDockerCmd(pathToDocker, ["image", "rm", builtImage.localTag]); + await runDockerCommand(pathToDocker, [ + "image", + "rm", + builtImage.localTag, + ]); builtImage.localTagCleaned = true; } catch (error) { if (error instanceof Error) { @@ -736,6 +863,7 @@ function getContainerImageRepositoryName( * @param options.dockerfile - The Dockerfile content to pipe into stdin. * @param options.verifyDockerIsRunning - When `true` (the default), verifies Docker is installed * and the daemon is running before spawning the build. Set to `false` to skip the check. + * @param options.outputMode - Capture build output, or inherit the terminal. Defaults to capture. * * @returns An object with an `abort` function and a `ready` promise. */ @@ -745,6 +873,7 @@ export async function dockerBuild( buildCmd: string[]; dockerfile: string; verifyDockerIsRunning?: boolean; + outputMode?: DockerOutputMode; } ): Promise<{ abort: () => void; ready: Promise }> { if (options.verifyDockerIsRunning !== false) { @@ -762,8 +891,17 @@ export async function dockerBuild( reject = rej; }); - const child = spawn(dockerPath, options.buildCmd, { - stdio: ["pipe", "inherit", "inherit"], + const outputMode = options.outputMode ?? "capture"; + const buildCmd = + outputMode === "capture" + ? withPlainProgress(options.buildCmd) + : options.buildCmd; + const capturedOutput = createBoundedOutputCollector(); + const child = spawn(dockerPath, buildCmd, { + stdio: + outputMode === "capture" + ? ["pipe", "pipe", "pipe"] + : ["pipe", "inherit", "inherit"], // We need to set detached to true so that the child process // will control all of its child processes and we can kill // all of them in case we need to abort the build process. @@ -778,23 +916,38 @@ export async function dockerBuild( child.stdin.write(options.dockerfile); child.stdin.end(); } + child.stdout?.on("data", capturedOutput.append); + child.stderr?.on("data", capturedOutput.append); - child.on("exit", (code) => { + child.on("close", (code) => { if (code === 0) { resolve(); } else if (!errorHandled) { errorHandled = true; + const details = capturedOutput.read(); + const message = details + ? `Docker build failed with exit code ${code}:\n${details}` + : `Docker build exited with code: ${code}`; reject( - new UserError(`Docker build exited with code: ${code}`, { - telemetryMessage: false, - }) + new UserError( + outputMode === "capture" ? withDockerDebugHint(message) : message, + { + telemetryMessage: false, + } + ) ); } }); child.on("error", (err) => { if (!errorHandled) { errorHandled = true; - reject(err); + const message = `Docker build failed: ${err.message}`; + reject( + new UserError( + outputMode === "capture" ? withDockerDebugHint(message) : message, + { telemetryMessage: false } + ) + ); } }); return { diff --git a/packages/containers-shared/src/images.ts b/packages/containers-shared/src/images.ts index 22118f3abf7..4e667d1f9cb 100644 --- a/packages/containers-shared/src/images.ts +++ b/packages/containers-shared/src/images.ts @@ -155,6 +155,7 @@ export async function prepareContainerImagesForDev(args: { const build = await startContainerBuild({ pathToDocker: dockerPath, verifyDockerIsRunning: false, + outputMode: "inherit", build: { tag: options.image_tag, pathToDockerfile: options.dockerfile, diff --git a/packages/containers-shared/src/login.ts b/packages/containers-shared/src/login.ts index b0be6c4c9bd..f18f71153a7 100644 --- a/packages/containers-shared/src/login.ts +++ b/packages/containers-shared/src/login.ts @@ -2,6 +2,10 @@ import { spawn } from "node:child_process"; import { UserError } from "@cloudflare/workers-utils/errors"; import { ImageRegistriesService, ImageRegistryPermissions } from "./client"; import { OpenAPI } from "./client/core/OpenAPI"; +import { + createBoundedOutputCollector, + withDockerDebugHint, +} from "./process-output"; export function configureOpenAPIForContainerPull( accountId: string, @@ -22,10 +26,13 @@ export function configureOpenAPIForContainerPull( * Gets push and pull credentials for a configured image registry * and runs `docker login`, so subsequent image pushes or pulls are * authenticated + * + * @param outputMode - Capture output for concise callers, or inherit it for development and debug workflows. */ export async function dockerLoginImageRegistry( pathToDocker: string, - domain: string + domain: string, + outputMode: "capture" | "inherit" = "inherit" ) { // how long the credentials should be valid for const expirationMinutes = 15; @@ -42,24 +49,56 @@ export async function dockerLoginImageRegistry( const child = spawn( pathToDocker, ["login", "--password-stdin", "--username", credentials.username, domain], - { stdio: ["pipe", "inherit", "inherit"] } - ).on("error", (err) => { - throw err; - }); + { + stdio: + outputMode === "capture" + ? ["pipe", "pipe", "pipe"] + : ["pipe", "inherit", "inherit"], + } + ); + const capturedOutput = createBoundedOutputCollector(); + child.stdout?.on("data", capturedOutput.append); + child.stderr?.on("data", capturedOutput.append); - child.stdin.write(credentials.password); - child.stdin.end(); - await new Promise((resolve, reject) => { + const login = new Promise((resolve, reject) => { + let settled = false; + child.on("error", (error) => { + if (settled) { + return; + } + settled = true; + const message = `Docker login failed: ${error.message}`; + reject( + new UserError( + outputMode === "capture" ? withDockerDebugHint(message) : message, + { + telemetryMessage: false, + } + ) + ); + }); child.on("close", (code) => { + if (settled) { + return; + } + settled = true; if (code === 0) { resolve(); } else { + const details = capturedOutput.read(); + const message = details + ? `Docker login failed with exit code ${code}:\n${details}` + : `Docker login failed with exit code: ${code}`; reject( - new UserError(`Login failed with code: ${code}`, { - telemetryMessage: false, - }) + new UserError( + outputMode === "capture" ? withDockerDebugHint(message) : message, + { telemetryMessage: false } + ) ); } }); }); + + child.stdin?.end(credentials.password); + await login; } diff --git a/packages/containers-shared/src/process-output.ts b/packages/containers-shared/src/process-output.ts new file mode 100644 index 00000000000..0db5a00c6b2 --- /dev/null +++ b/packages/containers-shared/src/process-output.ts @@ -0,0 +1,38 @@ +const DEFAULT_MAX_OUTPUT_BYTES = 64 * 1024; +const DOCKER_DEBUG_HINT = + "Set WRANGLER_LOG=debug to stream complete Docker output."; + +/** Keep the most recent subprocess output without allowing logs to grow unbounded. */ +export function createBoundedOutputCollector( + maxBytes = DEFAULT_MAX_OUTPUT_BYTES +) { + let output: Buffer = Buffer.alloc(0); + + return { + append(chunk: unknown) { + const next = Buffer.isBuffer(chunk) ? chunk : Buffer.from(String(chunk)); + if (next.byteLength >= maxBytes) { + output = next.subarray(next.byteLength - maxBytes); + return; + } + + const retainedBytes = Math.min( + output.byteLength, + maxBytes - next.byteLength + ); + output = Buffer.concat([ + output.subarray(output.byteLength - retainedBytes), + next, + ]); + }, + read() { + return output.toString("utf8").trim(); + }, + }; +} + +export function withDockerDebugHint(message: string): string { + return message.includes(DOCKER_DEBUG_HINT) + ? message + : `${message}\n\n${DOCKER_DEBUG_HINT}`; +} diff --git a/packages/containers-shared/src/utils.ts b/packages/containers-shared/src/utils.ts index abd2c9f21b6..d5053abf31d 100644 --- a/packages/containers-shared/src/utils.ts +++ b/packages/containers-shared/src/utils.ts @@ -9,6 +9,10 @@ import { existsSync } from "node:fs"; import { release } from "node:os"; import { UserError } from "@cloudflare/workers-utils/errors"; import { dockerImageInspect } from "./inspect"; +import { + createBoundedOutputCollector, + withDockerDebugHint, +} from "./process-output"; import type { ContainerDevOptions } from "./types"; /** helper for simple docker command call that don't require any io handling */ @@ -41,6 +45,9 @@ export const runDockerCmd = ( // This is a no-op on non-Windows platforms. windowsHide: true, }); + const capturedOutput = createBoundedOutputCollector(); + child.stdout?.on("data", capturedOutput.append); + child.stderr?.on("data", capturedOutput.append); let errorHandled = false; child.on("close", (code) => { @@ -48,20 +55,31 @@ export const runDockerCmd = ( resolve({ aborted }); } else if (!errorHandled) { errorHandled = true; + const details = capturedOutput.read(); + const message = details + ? `Docker command failed with exit code ${code}:\n${details}` + : `Docker command exited with code: ${code}`; reject( - new UserError(`Docker command exited with code: ${code}`, { - telemetryMessage: false, - }) + new UserError( + stdio === undefined || stdio === "inherit" + ? message + : withDockerDebugHint(message), + { telemetryMessage: false } + ) ); } }); child.on("error", (err) => { if (!errorHandled) { errorHandled = true; + const message = `Docker command failed: ${err.message}`; reject( - new UserError(`Docker command failed: ${err.message}`, { - telemetryMessage: false, - }) + new UserError( + stdio === undefined || stdio === "inherit" + ? message + : withDockerDebugHint(message), + { telemetryMessage: false } + ) ); } }); diff --git a/packages/containers-shared/tests/build-and-push.test.ts b/packages/containers-shared/tests/build-and-push.test.ts index 415785709fb..bf29adc900e 100644 --- a/packages/containers-shared/tests/build-and-push.test.ts +++ b/packages/containers-shared/tests/build-and-push.test.ts @@ -204,6 +204,7 @@ describe("buildCommand", () => { }); afterEach(() => { + delete logger.loggerLevel; vi.restoreAllMocks(); vi.unstubAllEnvs(); for (const dir of tempDirs) { @@ -226,6 +227,8 @@ describe("buildCommand", () => { expectSpawnWith([ "build", + "--progress", + "plain", "--load", "-t", "test-app:tag", @@ -238,6 +241,49 @@ describe("buildCommand", () => { ]); }); + it("streams Docker build output when debug logging is enabled", async () => { + logger.loggerLevel = "debug"; + const { dir, args } = createBuildArgs(); + tempDirs.push(dir); + + await buildCommand({ + PATH: dir, + tag: args.tag, + pathToDocker: "docker", + push: false, + }); + + expectSpawnWith([ + "build", + "--load", + "-t", + "test-app:tag", + "--platform", + "linux/amd64", + "--provenance=false", + "-f", + "-", + dir, + ]); + }); + + it("does not emit image progress when log output is disabled", async ({ + expect, + }) => { + logger.loggerLevel = "warn"; + const { dir, args } = createBuildArgs(); + tempDirs.push(dir); + + await buildCommand({ + PATH: dir, + tag: args.tag, + pathToDocker: "docker", + push: false, + }); + + expect(logger.log).not.toHaveBeenCalled(); + }); + it("tags and pushes new images, returning the pushed digest", async ({ expect, }) => { @@ -280,6 +326,7 @@ describe("buildCommand", () => { ]); expectSpawnWith([ "push", + "--quiet", `${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`, ]); }); @@ -309,6 +356,7 @@ describe("buildCommand", () => { expectSpawnWith(["image", "rm", "test-app:tag"]); expectNoSpawnWith([ "push", + "--quiet", `${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`, ]); }); @@ -450,6 +498,8 @@ describe("deploy container image build and push", () => { expectSpawnWith([ "build", + "--progress", + "plain", "--load", "-t", "test-app:wrangler-11111111-1111-4111-8111-111111111111", @@ -515,6 +565,7 @@ describe("deploy container image build and push", () => { ]); expectSpawnWith([ "push", + "--quiet", `${getCloudflareContainerRegistry()}/some-account-id/test-app:Galaxy`, ]); const tagCallIndex = vi @@ -546,6 +597,7 @@ describe("deploy container image build and push", () => { JSON.stringify(args) === JSON.stringify([ "push", + "--quiet", `${getCloudflareContainerRegistry()}/some-account-id/test-app:Galaxy`, ]) ); @@ -638,6 +690,8 @@ describe("buildCommand arguments", () => { expectSpawnCommandWith("/custom/docker", [ "build", + "--progress", + "plain", "--load", "-t", "test-app:tag", @@ -668,6 +722,8 @@ describe("buildCommand arguments", () => { expectSpawnCommandWith("/env/docker", [ "build", + "--progress", + "plain", "--load", "-t", "test-app:tag", @@ -725,6 +781,7 @@ describe("pushCommand", () => { ]); expectSpawnWith([ "push", + "--quiet", `${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`, ]); }); @@ -748,6 +805,7 @@ describe("pushCommand", () => { ]); expectSpawnCommandWith("/env/docker", [ "push", + "--quiet", `${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`, ]); }); @@ -763,6 +821,7 @@ describe("pushCommand", () => { ).rejects.toThrow("Unsupported platform"); expectNoSpawnWith([ "push", + "--quiet", `${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`, ]); }); diff --git a/packages/containers-shared/tests/build.test.ts b/packages/containers-shared/tests/build.test.ts index 23d1429a510..3ac34feb823 100644 --- a/packages/containers-shared/tests/build.test.ts +++ b/packages/containers-shared/tests/build.test.ts @@ -12,13 +12,31 @@ vi.mock("node:fs"); * @param exitCode - The exit code the fake process should emit. * @returns A minimal child-process-like object accepted by `spawn` callers. */ -function createFakeChildProcess(exitCode: number): ReturnType { +function createFakeChildProcess( + exitCode: number, + { + stdout = "", + stderr = "", + }: { + stdout?: string; + stderr?: string; + } = {} +): ReturnType { const emitter = new EventEmitter(); + const stdoutStream = new EventEmitter(); + const stderrStream = new EventEmitter(); + const stdin = { write: vi.fn(), end: vi.fn() }; // Simulate async close so listeners are registered before the event fires. - process.nextTick(() => emitter.emit("close", exitCode)); + process.nextTick(() => { + stdoutStream.emit("data", Buffer.from(stdout)); + stderrStream.emit("data", Buffer.from(stderr)); + emitter.emit("close", exitCode); + }); return Object.assign(emitter, { pid: 1234, - stdin: null, + stdin, + stdout: stdoutStream, + stderr: stderrStream, unref: vi.fn(), }) as unknown as ReturnType; } @@ -53,18 +71,9 @@ describe("dockerBuild", () => { it("skips Docker verification when verifyDockerIsRunning is false", async ({ expect, }) => { - const fakeProcess = new EventEmitter(); - const fakeStdin = { write: vi.fn(), end: vi.fn() }; - Object.assign(fakeProcess, { - pid: 1234, - stdin: fakeStdin, - unref: vi.fn(), - }); - vi.mocked(spawn).mockReturnValue( - fakeProcess as unknown as ReturnType - ); + vi.mocked(spawn).mockReturnValue(createFakeChildProcess(0)); - const resultPromise = dockerBuild("docker", { + const result = await dockerBuild("docker", { buildCmd: ["build", "-t", "test"], dockerfile: "FROM node:18", verifyDockerIsRunning: false, @@ -75,13 +84,67 @@ describe("dockerBuild", () => { expect(spawn).toHaveBeenCalledTimes(1); expect(spawn).toHaveBeenCalledWith( "docker", - ["build", "-t", "test"], - expect.any(Object) + ["build", "--progress", "plain", "-t", "test"], + { + detached: process.platform !== "win32", + stdio: ["pipe", "pipe", "pipe"], + windowsHide: true, + } ); + await result.ready; + }); + + it("inherits live output when requested", async ({ expect }) => { + vi.mocked(spawn).mockReturnValue(createFakeChildProcess(0)); + + const result = await dockerBuild("docker", { + buildCmd: ["build", "-t", "test"], + dockerfile: "FROM node:18", + verifyDockerIsRunning: false, + outputMode: "inherit", + }); + + expect(spawn).toHaveBeenCalledWith("docker", ["build", "-t", "test"], { + detached: process.platform !== "win32", + stdio: ["pipe", "inherit", "inherit"], + windowsHide: true, + }); + await result.ready; + }); + + it("preserves an explicitly configured progress mode", async ({ expect }) => { + vi.mocked(spawn).mockReturnValue(createFakeChildProcess(0)); + + const result = await dockerBuild("docker", { + buildCmd: ["build", "--progress=rawjson", "-t", "test"], + dockerfile: "FROM node:18", + verifyDockerIsRunning: false, + }); - // Simulate successful build - process.nextTick(() => fakeProcess.emit("exit", 0)); - const result = await resultPromise; + expect(spawn).toHaveBeenCalledWith( + "docker", + ["build", "--progress=rawjson", "-t", "test"], + expect.any(Object) + ); await result.ready; }); + + it("includes bounded captured output when a build fails", async ({ + expect, + }) => { + vi.mocked(spawn).mockReturnValue( + createFakeChildProcess(1, { + stderr: `discarded diagnostic\n${"x".repeat(64 * 1024)}\nuseful final error`, + }) + ); + + const result = await dockerBuild("docker", { + buildCmd: ["build", "-t", "test"], + dockerfile: "FROM node:18", + verifyDockerIsRunning: false, + }); + + await expect(result.ready).rejects.toThrow("useful final error"); + await expect(result.ready).rejects.not.toThrow("discarded diagnostic"); + }); }); diff --git a/packages/containers-shared/tests/login.test.ts b/packages/containers-shared/tests/login.test.ts index 2c631febd49..43b863a7332 100644 --- a/packages/containers-shared/tests/login.test.ts +++ b/packages/containers-shared/tests/login.test.ts @@ -1,12 +1,51 @@ -import { afterEach, describe, it } from "vitest"; +import { spawn } from "node:child_process"; +import { EventEmitter } from "node:events"; +import { afterEach, beforeEach, describe, it, vi } from "vitest"; +import { ImageRegistriesService } from "../src/client"; import { OpenAPI } from "../src/client/core/OpenAPI"; -import { configureOpenAPIForContainerPull } from "../src/login"; +import { + configureOpenAPIForContainerPull, + dockerLoginImageRegistry, +} from "../src/login"; + +vi.mock("node:child_process"); + +function createFakeChildProcess({ + exitCode, + stdout = "", + stderr = "", + error, +}: { + exitCode: number; + stdout?: string; + stderr?: string; + error?: Error; +}): ReturnType { + const emitter = new EventEmitter(); + const stdoutStream = new EventEmitter(); + const stderrStream = new EventEmitter(); + const stdin = { end: vi.fn() }; + process.nextTick(() => { + stdoutStream.emit("data", Buffer.from(stdout)); + stderrStream.emit("data", Buffer.from(stderr)); + if (error) { + emitter.emit("error", error); + } + emitter.emit("close", exitCode); + }); + return Object.assign(emitter, { + stdin, + stdout: stdoutStream, + stderr: stderrStream, + }) as unknown as ReturnType; +} describe("configureOpenAPIForContainerPull", () => { afterEach(() => { OpenAPI.BASE = ""; OpenAPI.HEADERS = undefined; OpenAPI.CREDENTIALS = "include"; + vi.restoreAllMocks(); }); it("sets BASE, HEADERS, and CREDENTIALS", ({ expect }) => { @@ -31,3 +70,79 @@ describe("configureOpenAPIForContainerPull", () => { ); }); }); + +describe("dockerLoginImageRegistry", () => { + beforeEach(() => { + vi.mocked(spawn).mockReset(); + vi.spyOn( + ImageRegistriesService, + "generateImageRegistryCredentials" + ).mockResolvedValue({ + account_id: "account-id", + registry_host: "registry.example.com", + username: "user", + password: "secret", + }); + }); + + it("captures Docker's successful login output", async ({ expect }) => { + const child = createFakeChildProcess({ + exitCode: 0, + stdout: "Login Succeeded\n", + }); + vi.mocked(spawn).mockReturnValue(child); + + await dockerLoginImageRegistry("docker", "registry.example.com", "capture"); + + expect(spawn).toHaveBeenCalledWith( + "docker", + [ + "login", + "--password-stdin", + "--username", + "user", + "registry.example.com", + ], + { stdio: ["pipe", "pipe", "pipe"] } + ); + expect(child.stdin?.end).toHaveBeenCalledWith("secret"); + }); + + it("inherits Docker output when requested", async ({ expect }) => { + vi.mocked(spawn).mockReturnValue(createFakeChildProcess({ exitCode: 0 })); + + await dockerLoginImageRegistry("docker", "registry.example.com", "inherit"); + + expect(spawn).toHaveBeenCalledWith("docker", expect.any(Array), { + stdio: ["pipe", "inherit", "inherit"], + }); + }); + + it("includes captured diagnostics when login fails", async ({ expect }) => { + vi.mocked(spawn).mockReturnValue( + createFakeChildProcess({ + exitCode: 1, + stderr: "Error response from daemon: denied", + }) + ); + + await expect( + dockerLoginImageRegistry("docker", "registry.example.com", "capture") + ).rejects.toThrow( + "Docker login failed with exit code 1:\nError response from daemon: denied" + ); + }); + + it("rejects cleanly when Docker cannot be spawned", async ({ expect }) => { + vi.mocked(spawn).mockReturnValue( + createFakeChildProcess({ + exitCode: -2, + error: new Error("spawn docker ENOENT"), + }) + ); + + await expect( + dockerLoginImageRegistry("docker", "registry.example.com", "capture") + ).rejects.toThrow("Docker login failed: spawn docker ENOENT"); + }); +}); diff --git a/packages/containers-shared/tests/utils.test.ts b/packages/containers-shared/tests/utils.test.ts index 3f8ce2b1828..6bd4d794163 100644 --- a/packages/containers-shared/tests/utils.test.ts +++ b/packages/containers-shared/tests/utils.test.ts @@ -12,6 +12,7 @@ import { checkExposedPorts, cleanupDuplicateImageTags, containerPrivilegesAllowed, + runDockerCmd, verifyDockerInstalled, } from "./../src/utils"; import type { ContainerDevOptions } from "../src/types"; @@ -261,17 +262,60 @@ describe("containerPrivilegesAllowed", () => { * @param exitCode - The exit code the fake process should emit. * @returns A minimal child-process-like object accepted by `runDockerCmd`. */ -function createFakeChildProcess(exitCode: number): ReturnType { +function createFakeChildProcess( + exitCode: number, + { + stdout = "", + stderr = "", + }: { + stdout?: string; + stderr?: string; + } = {} +): ReturnType { const emitter = new EventEmitter(); + const stdoutStream = new EventEmitter(); + const stderrStream = new EventEmitter(); // Simulate async close so listeners are registered before the event fires. - process.nextTick(() => emitter.emit("close", exitCode)); + process.nextTick(() => { + stdoutStream.emit("data", Buffer.from(stdout)); + stderrStream.emit("data", Buffer.from(stderr)); + emitter.emit("close", exitCode); + }); return Object.assign(emitter, { pid: 1234, stdin: null, + stdout: stdoutStream, + stderr: stderrStream, unref: vi.fn(), }) as unknown as ReturnType; } +describe("runDockerCmd", () => { + beforeEach(() => { + vi.mocked(spawn).mockReset(); + }); + + it("includes captured diagnostics when a command fails", async ({ + expect, + }) => { + vi.mocked(spawn).mockReturnValue( + createFakeChildProcess(1, { + stderr: "denied: requested access to the resource is denied", + }) + ); + + const command = runDockerCmd( + "docker", + ["push", "example"], + ["ignore", "pipe", "pipe"] + ); + + await expect(command.ready).rejects.toThrow( + "Docker command failed with exit code 1:\ndenied: requested access to the resource is denied" + ); + }); +}); + describe("verifyDockerInstalled", () => { beforeEach(() => { vi.mocked(spawn).mockReset(); diff --git a/packages/deploy-helpers/src/deploy/helpers/durable-object-container-applications.ts b/packages/deploy-helpers/src/deploy/helpers/durable-object-container-applications.ts index 9199c26b5f7..6757142b4be 100644 --- a/packages/deploy-helpers/src/deploy/helpers/durable-object-container-applications.ts +++ b/packages/deploy-helpers/src/deploy/helpers/durable-object-container-applications.ts @@ -332,6 +332,7 @@ async function buildOrResolveImage( accountId, complianceConfig: config, cleanupSourceTag: true, + displayName: `${container.class_name}/${imageName}`, }); builtImage.localTagCleaned = true; diff --git a/packages/deploy-helpers/tests/durable-object-container-applications.test.ts b/packages/deploy-helpers/tests/durable-object-container-applications.test.ts index b79339ece29..592d5295f40 100644 --- a/packages/deploy-helpers/tests/durable-object-container-applications.test.ts +++ b/packages/deploy-helpers/tests/durable-object-container-applications.test.ts @@ -210,6 +210,7 @@ describe("Container image preparation", () => { accountId: "account", complianceConfig: config, cleanupSourceTag: true, + displayName: "Sandbox/tools", }); expect(builtImage.localTagCleaned).toBe(true); }); diff --git a/packages/wrangler/src/__tests__/containers/deploy.test.ts b/packages/wrangler/src/__tests__/containers/deploy.test.ts index 17b7bb7fe53..6d516f469d0 100644 --- a/packages/wrangler/src/__tests__/containers/deploy.test.ts +++ b/packages/wrangler/src/__tests__/containers/deploy.test.ts @@ -986,7 +986,8 @@ describe("wrangler deploy with containers", () => { " ⛅️ wrangler x.x.x ────────────────── - Building image my-container:wrangler-11111111-1111-4111-8111-111111111111 + Building image my-container... + Built image my-container (TIMINGS) Total Upload: xx KiB / gzip: xx KiB Worker Startup Time: 100 ms Your Worker has access to the following bindings: @@ -997,7 +998,8 @@ describe("wrangler deploy with containers", () => { - my-container (/Dockerfile) Uploaded test-name (TIMINGS) - Image does not exist remotely, pushing: registry.cloudflare.com/some-account-id/my-container:Galaxy + Pushing image my-container... + Pushed image my-container (TIMINGS) Deployed test-name triggers (TIMINGS) https://test-name.test-sub-domain.workers.dev Current Version ID: Galaxy-Class" @@ -1480,7 +1482,8 @@ describe("wrangler deploy with containers", () => { " ⛅️ wrangler x.x.x ────────────────── - Building image my-container:wrangler-11111111-1111-4111-8111-111111111111 + Building image my-container... + Built image my-container (TIMINGS) Total Upload: xx KiB / gzip: xx KiB Worker Startup Time: 100 ms Your Worker has access to the following bindings: @@ -1491,7 +1494,8 @@ describe("wrangler deploy with containers", () => { - my-container (/Dockerfile) Uploaded test-name (TIMINGS) - Image does not exist remotely, pushing: registry.cloudflare.com/some-account-id/my-container:Galaxy + Pushing image my-container... + Pushed image my-container (TIMINGS) Deployed test-name triggers (TIMINGS) https://test-name.test-sub-domain.workers.dev Current Version ID: Galaxy-Class" @@ -1549,7 +1553,8 @@ describe("wrangler deploy with containers", () => { " ⛅️ wrangler x.x.x ────────────────── - Building image my-container:wrangler-11111111-1111-4111-8111-111111111111 + Building image my-container... + Built image my-container (TIMINGS) Total Upload: xx KiB / gzip: xx KiB Worker Startup Time: 100 ms Your Worker has access to the following bindings: @@ -1560,7 +1565,8 @@ describe("wrangler deploy with containers", () => { - my-container (/Dockerfile) Uploaded test-name (TIMINGS) - Image does not exist remotely, pushing: registry.cloudflare.com/some-account-id/my-container:Galaxy + Pushing image my-container... + Pushed image my-container (TIMINGS) Deployed test-name triggers (TIMINGS) https://test-name.test-sub-domain.workers.dev Current Version ID: Galaxy-Class" @@ -3424,7 +3430,9 @@ describe("wrangler deploy with containers", () => { await runWrangler("deploy index.js"); - expect(std.out).toContain("Image already exists remotely, skipping push"); + expect(std.out).toContain( + "Image my-container is unchanged; reusing existing upload." + ); expect(cliStd.stdout).toMatchInlineSnapshot(` "╭ Deploy a container application deploy changes to your application │ @@ -3539,7 +3547,9 @@ describe("wrangler deploy with containers", () => { await runWrangler("deploy index.js"); - expect(std.out).toContain("Image already exists remotely, skipping push"); + expect(std.out).toContain( + "Image my-container is unchanged; reusing existing upload." + ); expect(cliStd.stdout).toMatchInlineSnapshot(` "╭ Deploy a container application deploy changes to your application │ @@ -4318,7 +4328,8 @@ describe("wrangler deploy with containers dry run", () => { " ⛅️ wrangler x.x.x ────────────────── - Building image my-container:wrangler-11111111-1111-4111-8111-111111111111 + Building image my-container... + Built image my-container (TIMINGS) Total Upload: xx KiB / gzip: xx KiB Your Worker has access to the following bindings: Binding Resource @@ -5049,9 +5060,11 @@ function mockGetVersionNotFoundOnce( } function defaultChildProcess() { + const stdout = new PassThrough(); + const stderr = new PassThrough(); return { - stderr: Buffer.from([]), - stdout: Buffer.from("i promise I am a successful process"), + stderr, + stdout, on: function (reason: string, cbPassed: (code: number) => unknown) { if (reason === "close") { cbPassed(0); @@ -5227,6 +5240,8 @@ function mockDockerBuild( expect(cmd).toBe("/usr/bin/docker"); expect(args).toEqual([ "build", + "--progress", + "plain", "--load", "-t", `${containerName}:${tag}`, @@ -5248,14 +5263,14 @@ function mockDockerBuild( return { pid: -1, error: undefined, - stderr: Buffer.from([]), - stdout: Buffer.from("i promise I am a successful docker build"), + stderr: new PassThrough(), + stdout: new PassThrough(), stdin: readable, status: 0, signal: null, output: [null], on: (reason: string, cbPassed: (code: number) => unknown) => { - if (reason === "exit") { + if (reason === "close") { expect(dockerfile).toEqual(expectedDockerfile); cbPassed(0); } @@ -5415,12 +5430,15 @@ function mockDockerLogin(expect: ExpectStatic, expectedPassword: string) { final() {}, }); return { - stdout: Buffer.from("i promise I am a successful docker login"), + stdout: new PassThrough(), + stderr: new PassThrough(), stdin: readable, on: function (reason: string, cbPassed: (code: number) => unknown) { if (reason === "close") { - expect(password).toEqual(expectedPassword); - cbPassed(0); + setImmediate(() => { + expect(password).toEqual(expectedPassword); + cbPassed(0); + }); } return this; }, @@ -5437,6 +5455,7 @@ function mockDockerPush( expect(cmd).toBe("/usr/bin/docker"); expect(args).toEqual([ "push", + "--quiet", `${getCloudflareContainerRegistry()}/${containerName}:${tag}`, ]); return defaultChildProcess(); diff --git a/packages/wrangler/src/__tests__/deployment-bundle/build-container-images.test.ts b/packages/wrangler/src/__tests__/deployment-bundle/build-container-images.test.ts index 7fc0ec4e247..032c92e8168 100644 --- a/packages/wrangler/src/__tests__/deployment-bundle/build-container-images.test.ts +++ b/packages/wrangler/src/__tests__/deployment-bundle/build-container-images.test.ts @@ -72,7 +72,9 @@ describe("buildDurableObjectContainerImages", () => { expect.any(String), false, undefined, - false + false, + undefined, + { displayName: "Sandbox/tools" } ); expect(result).toEqual([ { diff --git a/packages/wrangler/src/__tests__/preview/containers.test.ts b/packages/wrangler/src/__tests__/preview/containers.test.ts index 80a9cb27b6f..826c39ea361 100644 --- a/packages/wrangler/src/__tests__/preview/containers.test.ts +++ b/packages/wrangler/src/__tests__/preview/containers.test.ts @@ -95,7 +95,8 @@ describe("deployPreviewContainers", () => { true, expect.objectContaining({ name: "test-worker_my-feature_mycontainer" }), false, - config + config, + { displayName: "test-worker_my-feature_mycontainer" } ); expect(vi.mocked(apply).mock.calls[0]?.[1]).toMatchObject({ name: PREVIEW_APP_NAME, diff --git a/packages/wrangler/src/deployment-bundle/build-container-images.ts b/packages/wrangler/src/deployment-bundle/build-container-images.ts index 1d49163766c..e840756c05f 100644 --- a/packages/wrangler/src/deployment-bundle/build-container-images.ts +++ b/packages/wrangler/src/deployment-bundle/build-container-images.ts @@ -138,7 +138,9 @@ export async function buildDurableObjectContainerImages( dockerPath, false, undefined, - false + false, + undefined, + { displayName: `${container.class_name}/${imageName}` } ); builtImages.push({ className: container.class_name, diff --git a/packages/wrangler/src/preview/containers.ts b/packages/wrangler/src/preview/containers.ts index a2a40786198..ad0b3c5ada2 100644 --- a/packages/wrangler/src/preview/containers.ts +++ b/packages/wrangler/src/preview/containers.ts @@ -189,7 +189,6 @@ async function buildContainer( complianceConfig?: ComplianceConfig ): Promise { const imageFullName = `${containerConfig.name}:${imageTag.split("-")[0]}`; - logger.log("Building image", imageFullName); return await buildAndMaybePush( { @@ -202,6 +201,7 @@ async function buildContainer( !dryRun, containerConfig, verifyDockerIsRunning, - complianceConfig + complianceConfig, + { displayName: containerConfig.name } ); }