naming changes and other minor pr feedback

Author: emily-shen

packages/containers-shared/src/images.ts

@@ -10,7 +10,7 @@ import {
 	runDockerCmd,
 	verifyDockerInstalled,
 } from "./utils";
-import type { ContainerNormalisedConfig, RegistryLinkConfig } from "./types";
+import type { ContainerNormalizedConfig, RegistryLinkConfig } from "./types";
 
 export async function pullImage(
 	dockerPath: string,
@@ -20,15 +20,15 @@ export async function pullImage(
 	await dockerLoginManagedRegistry(dockerPath);
 	const pull = runDockerCmd(dockerPath, [
 		"pull",
-		options.registry_link,
+		options.image_uri,
 		// All containers running on our platform need to be built for amd64 architecture, but by default docker pull seems to look for an image matching the host system, so we need to specify this here
 		"--platform",
 		"linux/amd64",
 	]);
 	const ready = pull.ready.then(async ({ aborted }: { aborted: boolean }) => {
 		if (!aborted) {
 			// re-tag image with the expected dev-formatted image tag for consistency
-			await runDockerCmd(dockerPath, ["tag", options.registry_link, tag]);
+			await runDockerCmd(dockerPath, ["tag", options.image_uri, tag]);
 		}
 	});
 
@@ -52,13 +52,13 @@ export async function pullImage(
  */
 export async function prepareContainerImagesForDev(options: {
 	dockerPath: string;
-	containerOptions: ContainerNormalisedConfig[];
+	containerOptions: ContainerNormalizedConfig[];
 	onContainerImagePreparationStart: (args: {
-		containerOptions: ContainerNormalisedConfig;
+		containerOptions: ContainerNormalizedConfig;
 		abort: () => void;
 	}) => void;
 	onContainerImagePreparationEnd: (args: {
-		containerOptions: ContainerNormalisedConfig[];
+		containerOptions: ContainerNormalizedConfig[];
 	}) => void;
 }) {
 	const {
@@ -90,9 +90,9 @@ export async function prepareContainerImagesForDev(options: {
 				containerOptions: options,
 			});
 		} else {
-			if (!isCloudflareRegistryLink(options.registry_link)) {
+			if (!isCloudflareRegistryLink(options.image_uri)) {
 				throw new Error(
-					`Image "${options.registry_link}" is a registry link but does not point to the Cloudflare container registry.\n` +
+					`Image "${options.image_uri}" is a registry link but does not point to the Cloudflare container registry.\n` +
 						`To use an existing image from another repository, see https://developers.cloudflare.com/containers/image-management/#using-existing-images`
 				);
 			}

packages/containers-shared/src/types.ts

@@ -22,16 +22,16 @@ export type BuildArgs = {
 	setNetworkToHost?: boolean;
 };
 
-export type ContainerNormalisedConfig = RegistryLinkConfig | DockerfileConfig;
+export type ContainerNormalizedConfig = RegistryLinkConfig | DockerfileConfig;
 export type DockerfileConfig = SharedContainerConfig & {
-	/**  absolute path, resolved relative to the wrangler config file */
+	/** absolute path, resolved relative to the wrangler config file */
 	dockerfile: string;
 	/** absolute path, resolved relative to the wrangler config file. defaults to the directory of the dockerfile */
 	image_build_context: string;
 	image_vars?: Record<string, string>;
 };
 export type RegistryLinkConfig = SharedContainerConfig & {
-	registry_link: string;
+	image_uri: string;
 };
 
 export type InstanceTypeOrLimits =

packages/containers-shared/src/utils.ts

@@ -2,7 +2,7 @@ import { execFile, spawn } from "child_process";
 import { existsSync, statSync } from "fs";
 import path from "path";
 import { dockerImageInspect } from "./inspect";
-import type { ContainerNormalisedConfig } from "./types";
+import type { ContainerNormalizedConfig } from "./types";
 import type { StdioOptions } from "child_process";
 
 /** helper for simple docker command call that don't require any io handling */
@@ -229,7 +229,7 @@ export const getContainerIdsFromImage = async (
  */
 export async function checkExposedPorts(
 	dockerPath: string,
-	options: ContainerNormalisedConfig,
+	options: ContainerNormalizedConfig,
 	imageTag: string
 ) {
 	const output = await dockerImageInspect(dockerPath, {

packages/containers-shared/tests/utils.test.ts

@@ -1,7 +1,7 @@
 import { mkdirSync, writeFileSync } from "fs";
-import { ContainerNormalisedConfig } from "../src/types";
 import { checkExposedPorts, isDockerfile } from "./../src/utils";
 import { runInTempDir } from "./helpers/run-in-tmp-dir";
+import type { ContainerNormalizedConfig } from "../src/types";
 
 describe("isDockerfile", () => {
 	const dockerfile =
@@ -75,7 +75,7 @@ vi.mock("../src/inspect", async (importOriginal) => {
 const containerConfig = {
 	dockerfile: "",
 	class_name: "MyContainer",
-} as ContainerNormalisedConfig;
+} as ContainerNormalizedConfig;
 describe("checkExposedPorts", () => {
 	beforeEach(() => {
 		docketImageInspectResult = "1";
@@ -90,8 +90,8 @@ describe("checkExposedPorts", () => {
 
 	it("should error, with an appropriate message when no ports are exported", async () => {
 		docketImageInspectResult = "0";
-		expect(checkExposedPorts("docker", containerConfig, "image:tag")).rejects
-			.toThrowErrorMatchingInlineSnapshot(`
+		await expect(checkExposedPorts("docker", containerConfig, "image:tag"))
+			.rejects.toThrowErrorMatchingInlineSnapshot(`
 				[Error: The container "MyContainer" does not expose any ports. In your Dockerfile, please expose any ports you intend to connect to.
 				For additional information please see: https://developers.cloudflare.com/containers/local-dev/#exposing-ports.
 				]

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

@@ -281,7 +281,7 @@ describe("buildAndMaybePush", () => {
 					configDiskInBytes: 3000 * MiB, // ie 3GB - this exceeds the account limit of 2GB
 				})
 			).rejects.toThrowErrorMatchingInlineSnapshot(
-				`[Error: Exceeded account limits: Your container is configured to use a disk size of 3146 MB. However, that exceeds the account limit of 2000MB]`
+				`[Error: Exceeded account limits: Your container is configured to use a disk size of 3146MB. However, that exceeds the account limit of 2000MB]`
 			);
 		});
 
@@ -293,7 +293,7 @@ describe("buildAndMaybePush", () => {
 					configDiskInBytes: undefined,
 				})
 			).rejects.toThrowErrorMatchingInlineSnapshot(
-				`[Error: Image too large: needs 3146 MB, but your app is limited to images with size 2000 MB. Your account needs more disk size per instance to run this container. The default disk size is 2GB.]`
+				`[Error: Image too large: needs 3146MB, but your app is limited to images with size 2000MB. Your account needs more disk size per instance to run this container. The default disk size is 2GB.]`
 			);
 		});
 

packages/wrangler/src/__tests__/containers/config.test.ts

@@ -1,15 +1,14 @@
-import { isDockerfile } from "@cloudflare/containers-shared";
+import { mkdirSync, writeFileSync } from "fs";
+import { getCloudflareContainerRegistry } from "@cloudflare/containers-shared";
 import { vi } from "vitest";
 import { getNormalizedContainerOptions } from "../../containers/config";
 import { UserError } from "../../errors";
+import { mockAccountId, mockApiToken } from "../helpers/mock-account-id";
+import { runInTempDir } from "../helpers/run-in-tmp";
 import type { Config } from "../../config";
 
-// Mock dependencies using vi.hoisted
-vi.mock("@cloudflare/containers-shared");
-
-const mockIsDockerfile = vi.mocked(isDockerfile);
-
 describe("getNormalizedContainerOptions", () => {
+	runInTempDir();
 	beforeEach(() => {
 		vi.clearAllMocks();
 	});
@@ -99,18 +98,19 @@ describe("getNormalizedContainerOptions", () => {
 		await expect(getNormalizedContainerOptions(config)).rejects.toThrow(
 			UserError
 		);
-		await expect(getNormalizedContainerOptions(config)).rejects.toThrow(
-			"The container class_name TestContainer does not match any durable object class_name defined in your Wrangler config file"
+		await expect(
+			getNormalizedContainerOptions(config)
+		).rejects.toThrowErrorMatchingInlineSnapshot(
+			`[Error: The container test-container is referencing the durable object TestContainer, which appears to be defined on the other-script Worker instead (via the 'script_name' field). You cannot configure a container on a Durable Object that is defined in another Worker.]`
 		);
 	});
 
 	it("should normalize and set defaults for container with dockerfile", async () => {
-		mockIsDockerfile.mockReturnValue(true);
+		writeFileSync("Dockerfile", "FROM scratch");
 
 		const config: Config = {
 			name: "test-worker",
-			configPath: "/test/wrangler.toml",
-			userConfigPath: "/test/wrangler.toml",
+			configPath: "./wrangler.toml",
 			topLevelName: "test-worker",
 			containers: [
 				{
@@ -132,24 +132,22 @@ describe("getNormalizedContainerOptions", () => {
 		const result = await getNormalizedContainerOptions(config);
 
 		expect(result).toHaveLength(1);
-		expect(result[0]).toEqual({
+		expect(result[0]).toMatchObject({
 			name: "test-container",
 			class_name: "TestContainer",
 			max_instances: 0,
 			scheduling_policy: "default",
 			rollout_step_percentage: 25,
 			rollout_kind: "full_auto",
 			instance_type: "dev",
-			dockerfile: "/test/Dockerfile",
-			image_build_context: "/test",
+			dockerfile: expect.stringMatching(/[/\\]Dockerfile$/),
+			image_build_context: expect.stringMatching(/[/\\][^/\\]*$/),
 			image_vars: undefined,
 			constraints: undefined,
 		});
 	});
 
 	it("should normalize and set defaults for container with registry image", async () => {
-		mockIsDockerfile.mockReturnValue(false);
-
 		const config: Config = {
 			name: "test-worker",
 			configPath: "/test/wrangler.toml",
@@ -174,22 +172,20 @@ describe("getNormalizedContainerOptions", () => {
 		const result = await getNormalizedContainerOptions(config);
 
 		expect(result).toHaveLength(1);
-		expect(result[0]).toEqual({
+		expect(result[0]).toMatchObject({
 			name: "test-container",
 			class_name: "TestContainer",
 			max_instances: 0,
 			scheduling_policy: "default",
 			rollout_step_percentage: 25,
 			rollout_kind: "full_auto",
 			instance_type: "dev",
-			registry_link: "registry.example.com/test:latest",
 			constraints: undefined,
+			image_uri: "registry.cloudflare.com/some-account-id/test:latest",
 		});
 	});
 
 	it("should handle custom limit configuration", async () => {
-		mockIsDockerfile.mockReturnValue(false);
-
 		const config: Config = {
 			name: "test-worker",
 			configPath: "/test/wrangler.toml",
@@ -219,7 +215,7 @@ describe("getNormalizedContainerOptions", () => {
 
 		const result = await getNormalizedContainerOptions(config);
 
-		expect(result[0]).toEqual({
+		expect(result[0]).toMatchObject({
 			name: "test-container",
 			class_name: "TestContainer",
 			max_instances: 0,
@@ -229,14 +225,12 @@ describe("getNormalizedContainerOptions", () => {
 			disk_bytes: 5_000_000_000, // 5000 MB in bytes
 			memory_mib: 1024,
 			vcpu: 2,
-			registry_link: "registry.example.com/test:latest",
 			constraints: undefined,
+			image_uri: "registry.example.com/test:latest",
 		});
 	});
 
 	it("should handle instance type configuration", async () => {
-		mockIsDockerfile.mockReturnValue(false);
-
 		const config: Config = {
 			name: "test-worker",
 			configPath: "/test/wrangler.toml",
@@ -262,22 +256,20 @@ describe("getNormalizedContainerOptions", () => {
 
 		const result = await getNormalizedContainerOptions(config);
 
-		expect(result[0]).toEqual({
+		expect(result[0]).toMatchObject({
 			name: "test-container",
 			class_name: "TestContainer",
 			max_instances: 0,
 			scheduling_policy: "default",
 			rollout_step_percentage: 25,
 			rollout_kind: "full_auto",
 			instance_type: "standard",
-			registry_link: "registry.example.com/test:latest",
 			constraints: undefined,
+			image_uri: "registry.example.com/test:latest",
 		});
 	});
 
 	it("should handle all custom configuration options", async () => {
-		mockIsDockerfile.mockReturnValue(false);
-
 		const config: Config = {
 			name: "test-worker",
 			configPath: "/test/wrangler.toml",
@@ -320,7 +312,7 @@ describe("getNormalizedContainerOptions", () => {
 			rollout_step_percentage: 50,
 			rollout_kind: "full_manual",
 			instance_type: "basic",
-			registry_link: "registry.example.com/test:latest",
+			image_uri: "registry.example.com/test:latest",
 			constraints: {
 				regions: ["us-east-1", "us-west-2"],
 				cities: ["NYC", "SF"],
@@ -330,17 +322,17 @@ describe("getNormalizedContainerOptions", () => {
 	});
 
 	it("should handle dockerfile with default build context", async () => {
-		mockIsDockerfile.mockReturnValue(true);
+		mkdirSync("nested", { recursive: true });
+		writeFileSync("nested/Dockerfile", "FROM scratch");
 
 		const config: Config = {
 			name: "test-worker",
-			configPath: "/test/wrangler.toml",
-			userConfigPath: "/test/wrangler.toml",
+			configPath: "./wrangler.toml",
 			topLevelName: "test-worker",
 			containers: [
 				{
 					class_name: "TestContainer",
-					image: "./path/to/Dockerfile",
+					image: "./nested/Dockerfile",
 					name: "test-container",
 				},
 			],
@@ -356,24 +348,22 @@ describe("getNormalizedContainerOptions", () => {
 
 		const result = await getNormalizedContainerOptions(config);
 
-		expect(result[0]).toEqual({
+		expect(result[0]).toMatchObject({
 			name: "test-container",
 			class_name: "TestContainer",
 			max_instances: 0,
 			scheduling_policy: "default",
 			rollout_step_percentage: 25,
 			rollout_kind: "full_auto",
 			instance_type: "dev",
-			dockerfile: "/test/path/to/Dockerfile",
-			image_build_context: "/test/path/to",
+			dockerfile: expect.stringMatching(/[/\\]nested[/\\]Dockerfile$/),
+			image_build_context: expect.stringMatching(/[/\\]nested$/),
 			image_vars: undefined,
 			constraints: undefined,
 		});
 	});
 
 	it("should handle multiple containers", async () => {
-		mockIsDockerfile.mockReturnValue(false);
-
 		const config: Config = {
 			name: "test-worker",
 			configPath: "/test/wrangler.toml",
@@ -413,8 +403,7 @@ describe("getNormalizedContainerOptions", () => {
 	});
 
 	it("should handle config with no configPath", async () => {
-		mockIsDockerfile.mockReturnValue(true);
-
+		writeFileSync("Dockerfile", "FROM scratch");
 		const config: Config = {
 			name: "test-worker",
 			configPath: undefined,
@@ -439,9 +428,9 @@ describe("getNormalizedContainerOptions", () => {
 
 		const result = await getNormalizedContainerOptions(config);
 
-		// Check that it has dockerfile properties (not registry_link)
+		// Check that it has dockerfile properties (not image_uri)
 		expect(result[0]).toHaveProperty("dockerfile");
 		expect(result[0]).toHaveProperty("image_build_context");
-		expect(result[0]).not.toHaveProperty("registry_link");
+		expect(result[0]).not.toHaveProperty("image_uri");
 	});
 });

packages/wrangler/src/__tests__/containers/deploy.test.ts

@@ -26,7 +26,7 @@ import { writeWranglerConfig } from "../helpers/write-wrangler-config";
 import type {
 	AccountRegistryToken,
 	Application,
-	ContainerNormalisedConfig,
+	ContainerNormalizedConfig,
 	CreateApplicationRequest,
 	ImageRegistryCredentialsConfiguration,
 	InstanceType,
@@ -37,9 +37,9 @@ vi.mock("node:child_process");
 describe("maybeBuildContainer", () => {
 	it("Should return imageUpdate: true if using an image URI", async () => {
 		const config = {
-			registry_link: "registry.cloudflare.com/some-image:uri",
+			image_uri: "registry.cloudflare.com/some-image:uri",
 			class_name: "Test",
-		} as ContainerNormalisedConfig;
+		} as ContainerNormalizedConfig;
 		const result = await maybeBuildContainer(
 			config,
 			"some-tag:thing",