fix up loop in build command and config handling in ensureDiskSize

Author: emily-shen

packages/wrangler/src/__tests__/cloudchamber/build.test.ts

@@ -7,8 +7,6 @@ import {
 	runDockerCmd,
 } from "@cloudflare/containers-shared";
 import { ensureDiskLimits } from "../../cloudchamber/build";
-import { resolveAppDiskSize } from "../../cloudchamber/common";
-import { type ContainerApp } from "../../config/environment";
 import { UserError } from "../../errors";
 import { mockAccountId, mockApiToken } from "../helpers/mock-account-id";
 import { runInTempDir } from "../helpers/run-in-tmp";
@@ -17,12 +15,7 @@ import { mockAccountV4 as mockAccount } from "./utils";
 import type { CompleteAccountCustomer } from "@cloudflare/containers-shared";
 
 const MiB = 1024 * 1024;
-const defaultConfiguration: ContainerApp = {
-	name: "abc",
-	class_name: "",
-	instances: 0,
-	image: "",
-};
+
 vi.mock("@cloudflare/containers-shared", async (importOriginal) => {
 	const actual = await importOriginal();
 	return Object.assign({}, actual, {
@@ -255,13 +248,7 @@ describe("buildAndMaybePush", () => {
 				ensureDiskLimits({
 					requiredSize: 333 * MiB, // 333MiB
 					account: accountBase,
-					containerApp: {
-						...defaultConfiguration,
-						configuration: {
-							image: "",
-							disk: { size: "3GB" }, // This exceeds the account limit of 2GB
-						},
-					},
+					configDiskSize: 3000 * MiB, // This exceeds the account limit of 2GB
 				})
 			).rejects.toThrow("Exceeded account limits");
 		});
@@ -271,7 +258,7 @@ describe("buildAndMaybePush", () => {
 				ensureDiskLimits({
 					requiredSize: 3000 * MiB, // 3GiB
 					account: accountBase,
-					containerApp: undefined,
+					configDiskSize: undefined,
 				})
 			).rejects.toThrow("Image too large");
 		});
@@ -280,32 +267,10 @@ describe("buildAndMaybePush", () => {
 			const result = await ensureDiskLimits({
 				requiredSize: 256 * MiB, // 256MiB
 				account: accountBase,
-				containerApp: undefined,
+				configDiskSize: undefined,
 			});
 
 			expect(result).toEqual(undefined);
 		});
 	});
-
-	describe("resolveAppDiskSize", () => {
-		it("should return parsed app disk size", () => {
-			const result = resolveAppDiskSize({
-				...defaultConfiguration,
-				configuration: { image: "", disk: { size: "500MB" } },
-			});
-			expect(result).toBeCloseTo(500 * 1000 * 1000, -5);
-		});
-
-		it("should return default size when disk size not set", () => {
-			const result = resolveAppDiskSize({
-				...defaultConfiguration,
-				configuration: { image: "" },
-			});
-			expect(result).toBeCloseTo(2 * 1000 * 1000 * 1000, -5);
-		});
-
-		it("should return undefined if app is not passed", () => {
-			expect(resolveAppDiskSize(undefined)).toBeUndefined();
-		});
-	});
 });

packages/wrangler/src/cloudchamber/build.ts

@@ -18,10 +18,8 @@ import {
 import { UserError } from "../errors";
 import { logger } from "../logger";
 import { getAccountId } from "../user";
-import { resolveAppDiskSize } from "./common";
 import { loadAccount } from "./locations";
 import type { Config } from "../config";
-import type { ContainerApp } from "../config/environment";
 import type {
 	CommonYargsArgv,
 	StrictYargsOptionsToInterface,
@@ -80,7 +78,7 @@ export async function buildAndMaybePush(
 	args: BuildArgs,
 	pathToDocker: string,
 	push: boolean,
-	containerConfig?: ContainerApp
+	configDiskSize: number | undefined
 ): Promise<{ image: string; pushed: boolean }> {
 	try {
 		const imageTag = `${getCloudflareContainerRegistry()}/${args.tag}`;
@@ -122,7 +120,7 @@ export async function buildAndMaybePush(
 			await ensureDiskLimits({
 				requiredSize,
 				account,
-				containerApp: containerConfig,
+				configDiskSize,
 			});
 
 			await dockerLoginManagedRegistry(pathToDocker);
@@ -214,32 +212,31 @@ export async function buildAndMaybePush(
 }
 
 export async function buildCommand(
-	args: StrictYargsOptionsToInterface<typeof buildYargs>,
-	config: Config
+	args: StrictYargsOptionsToInterface<typeof buildYargs>
 ) {
 	// TODO: merge args with Wrangler config if available
 	if (existsSync(args.PATH) && !isDir(args.PATH)) {
 		throw new UserError(
 			`${args.PATH} is not a directory. Please specify a valid directory path.`
 		);
 	}
-	// if containers are not defined, the build should still work.
-	const containers = config.containers ?? [undefined];
+
 	const pathToDockerfile = join(args.PATH, "Dockerfile");
-	for (const container of containers) {
-		await buildAndMaybePush(
-			{
-				tag: args.tag,
-				pathToDockerfile,
-				buildContext: args.PATH,
-				platform: args.platform,
-				// no option to add env vars at build time...?
-			},
-			getDockerPath() ?? args.pathToDocker,
-			args.push,
-			container
-		);
-	}
+
+	await buildAndMaybePush(
+		{
+			tag: args.tag,
+			pathToDockerfile,
+			buildContext: args.PATH,
+			platform: args.platform,
+			// no option to add env vars at build time...?
+		},
+		getDockerPath() ?? args.pathToDocker,
+		args.push,
+		// this means we wont be able to read the disk size from the config, but that option is deprecated anyway at least for containers.
+		// and this never actually worked for cloudchamber as this command was previously reading it from config.containers not config.cloudchamber
+		undefined
+	);
 }
 
 export async function pushCommand(
@@ -269,23 +266,22 @@ export async function pushCommand(
 export async function ensureDiskLimits(options: {
 	requiredSize: number;
 	account: CompleteAccountCustomer;
-	containerApp: ContainerApp | undefined;
+	configDiskSize: number | undefined;
 }): Promise<void> {
 	const MB = 1000 * 1000;
 	const MiB = 1024 * 1024;
-	const appDiskSize = resolveAppDiskSize(options.containerApp);
 	const accountDiskSize =
 		(options.account.limits.disk_mb_per_deployment ?? 2000) * MB;
 	// if appDiskSize is defined and configured to be more than the accountDiskSize, error
-	if (appDiskSize && appDiskSize > accountDiskSize) {
+	if (options.configDiskSize && options.configDiskSize > accountDiskSize) {
 		throw new UserError(
-			`Exceeded account limits: Your container is configured to use a disk size of ${appDiskSize / MB} MB. However, that exceeds the account limit of ${accountDiskSize / MB}`
+			`Exceeded account limits: Your container is configured to use a disk size of ${options.configDiskSize / MB} MB. However, that exceeds the account limit of ${accountDiskSize / MB}`
 		);
 	}
-	const maxAllowedImageSizeBytes = appDiskSize ?? accountDiskSize;
+	const maxAllowedImageSizeBytes = options.configDiskSize ?? accountDiskSize;
 
 	logger.debug(
-		`Disk size limits when building the container: appDiskSize:${appDiskSize}, accountDiskSize:${accountDiskSize}, maxAllowedImageSizeBytes=${maxAllowedImageSizeBytes}(${maxAllowedImageSizeBytes / MB} MB), requiredSized=${options.requiredSize}(${Math.ceil(options.requiredSize / MiB)}MiB)`
+		`Disk size limits when building the container: appDiskSize:${options.configDiskSize}, accountDiskSize:${accountDiskSize}, maxAllowedImageSizeBytes=${maxAllowedImageSizeBytes}(${maxAllowedImageSizeBytes / MB} MB), requiredSized=${options.requiredSize}(${Math.ceil(options.requiredSize / MiB)}MiB)`
 	);
 	if (maxAllowedImageSizeBytes < options.requiredSize) {
 		throw new UserError(

packages/wrangler/src/cloudchamber/common.ts

@@ -614,20 +614,6 @@ export function resolveMemory(
 	return undefined;
 }
 
-// Return the amount of disk size in (MB) for an application, falls back to the account limits if the app config doesn't exist
-// sometimes the user wants to just build a container here, we should allow checking those based on the account limits if
-// app.configuration is not set
-// ordering: app.configuration.disk.size -> account.limits.disk_mb_per_deployment -> default fallback to 2GB in bytes
-export function resolveAppDiskSize(
-	app: ContainerApp | undefined
-): number | undefined {
-	if (app === undefined) {
-		return undefined;
-	}
-	const disk = app.configuration?.disk?.size ?? "2GB";
-	return Math.round(parseByteSize(disk));
-}
-
 // Checks that instance type is one of 'dev', 'basic', or 'standard' and that it is not being set alongside memory or vcpu.
 // Returns the instance type to use if correctly set.
 export function checkInstanceType(