PR feedback

Author: emily-shen

packages/containers-shared/src/types.ts

@@ -69,7 +69,7 @@ export type SharedContainerConfig = {
 		cities?: string[];
 		tier: number;
 	};
-	observability: { logsEnabled: boolean };
+	observability: { logs_enabled: boolean };
 } & InstanceTypeOrLimits;
 
 /** build/pull agnostic container options */

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

@@ -1,5 +1,5 @@
-import { mkdirSync, writeFileSync } from "fs";
-import path from "path";
+import { mkdirSync, writeFileSync } from "node:fs";
+import path from "node:path";
 import { getCloudflareContainerRegistry } from "@cloudflare/containers-shared";
 import { vi } from "vitest";
 import { getNormalizedContainerOptions } from "../../containers/config";
@@ -149,7 +149,7 @@ describe("getNormalizedContainerOptions", () => {
 			image_vars: undefined,
 			constraints: { tier: 1 },
 			observability: {
-				logsEnabled: false,
+				logs_enabled: false,
 			},
 		});
 	});
@@ -190,7 +190,7 @@ describe("getNormalizedContainerOptions", () => {
 			image_uri: "registry.cloudflare.com/some-account-id/test:latest",
 			constraints: { tier: 1 },
 			observability: {
-				logsEnabled: false,
+				logs_enabled: false,
 			},
 		});
 	});
@@ -224,7 +224,7 @@ describe("getNormalizedContainerOptions", () => {
 		} as Partial<Config> as Config;
 
 		const result = await getNormalizedContainerOptions(config);
-
+		expect(result).toHaveLength(1);
 		expect(result[0]).toMatchObject({
 			name: "test-container",
 			class_name: "TestContainer",
@@ -265,7 +265,7 @@ describe("getNormalizedContainerOptions", () => {
 		} as Partial<Config> as Config;
 
 		const result = await getNormalizedContainerOptions(config);
-
+		expect(result).toHaveLength(1);
 		expect(result[0]).toMatchObject({
 			name: "test-container",
 			class_name: "TestContainer",
@@ -316,7 +316,7 @@ describe("getNormalizedContainerOptions", () => {
 		} as Partial<Config> as Config;
 
 		const result = await getNormalizedContainerOptions(config);
-
+		expect(result).toHaveLength(1);
 		expect(result[0]).toEqual({
 			name: "custom-name",
 			class_name: "TestContainer",
@@ -332,7 +332,7 @@ describe("getNormalizedContainerOptions", () => {
 				cities: ["nyc", "sf"],
 			},
 			observability: {
-				logsEnabled: true,
+				logs_enabled: true,
 			},
 		});
 	});
@@ -363,7 +363,7 @@ describe("getNormalizedContainerOptions", () => {
 		} as Partial<Config> as Config;
 
 		const result = await getNormalizedContainerOptions(config);
-
+		expect(result).toHaveLength(1);
 		expect(result[0]).toMatchObject({
 			name: "test-container",
 			class_name: "TestContainer",
@@ -377,7 +377,7 @@ describe("getNormalizedContainerOptions", () => {
 			image_vars: undefined,
 			constraints: { tier: 1 },
 			observability: {
-				logsEnabled: false,
+				logs_enabled: false,
 			},
 		});
 	});
@@ -446,7 +446,7 @@ describe("getNormalizedContainerOptions", () => {
 		} as Partial<Config> as Config;
 
 		const result = await getNormalizedContainerOptions(config);
-
+		expect(result).toHaveLength(1);
 		// Check that it has dockerfile properties (not image_uri)
 		expect(result[0]).toHaveProperty("dockerfile");
 		expect(result[0]).toHaveProperty("image_build_context");

packages/wrangler/src/cloudchamber/build.ts

@@ -244,7 +244,7 @@ export async function buildCommand(
 		},
 		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.
+		// this means we won't 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
 	);

packages/wrangler/src/containers/config.ts

@@ -15,7 +15,6 @@ import type {
 	SharedContainerConfig,
 } from "@cloudflare/containers-shared";
 
-// TODO: allow merging in args from commands with config. e.g. for `wrangler containers build`
 /**
  * This normalises config into an intermediate shape for building or pulling.
  * We set defaults here too, because we can assume that if the value is undefined,
@@ -70,7 +69,7 @@ export const getNormalizedContainerOptions = async (
 			rollout_step_percentage: container.rollout_step_percentage ?? 25,
 			rollout_kind: container.rollout_kind ?? "full_auto",
 			observability: {
-				logsEnabled:
+				logs_enabled:
 					config.observability?.logs?.enabled ??
 					config.observability?.enabled === true,
 			},

packages/wrangler/src/containers/deploy.ts

@@ -97,8 +97,13 @@ function cleanupObservability(
 	}
 }
 
+/**
+ * Resolves current configuration based on previous deployment.
+ */
 function observabilityToConfiguration(
+	/** Taken from current wrangler config */
 	observabilityFromConfig: boolean,
+	/** From previous deployment */
 	existingObservabilityConfig: ObservabilityConfiguration | undefined
 ): ObservabilityConfiguration | undefined {
 	// Let's use logs for the sake of simplicity of explanation.
@@ -134,12 +139,10 @@ function observabilityToConfiguration(
 
 	if (observabilityFromConfig) {
 		return { logs: { enabled: true } };
+	} else if (logsAlreadyEnabled === undefined) {
+		return undefined;
 	} else {
-		if (logsAlreadyEnabled === undefined) {
-			return undefined;
-		} else {
-			return { logs: { enabled: false } };
-		}
+		return { logs: { enabled: false } };
 	}
 }
 
@@ -172,7 +175,7 @@ function containerConfigToCreateRequest(
 						vcpu: containerApp.vcpu,
 					}),
 			observability: observabilityToConfiguration(
-				containerApp.observability.logsEnabled,
+				containerApp.observability.logs_enabled,
 				prevApp?.configuration.observability
 			),
 		},
@@ -191,7 +194,7 @@ export async function apply(
 		imageUpdateRequired?: boolean;
 		/**
 		 * If the image was built and pushed, or is a registry link, we have to update the image ref and this will be defined
-		 * If it is undefined, the image has not change, and we do not need to update the image ref
+		 * If it is undefined, the image has not changed, and we do not need to update the image ref
 		 */
 		newImageLink: string | undefined;
 		durable_object_namespace_id: string;
@@ -261,7 +264,9 @@ export async function apply(
 
 		// we need to sort the objects (by key) because the diff algorithm works with lines
 		const normalisedPrevApp = sortObjectRecursive<ModifyApplicationRequestBody>(
-			stripUndefined(cleanPrevious(prevApp, containerConfig, accountId))
+			stripUndefined(
+				cleanApplicationFromAPI(prevApp, containerConfig, accountId)
+			)
 		);
 
 		const modifyReq = createApplicationToModifyApplication(appConfig);
@@ -487,7 +492,7 @@ const doAction = async (
 /**
  * clean up application object received from API so that we get a nicer diff when comparing it to the current config.
  */
-export function cleanPrevious(
+export function cleanApplicationFromAPI(
 	prev: Application,
 	currentConfig: ContainerNormalizedConfig,
 	accountId: string