diff --git a/.changeset/cyan-tools-show.md b/.changeset/cyan-tools-show.md new file mode 100644 index 000000000000..855934743690 --- /dev/null +++ b/.changeset/cyan-tools-show.md @@ -0,0 +1,9 @@ +--- +"wrangler": patch +--- + +Offer to update the local Wrangler configuration file to match remote configuration when running `wrangler deploy` + +When running `wrangler deploy`, with `--x-remote-diff-check`, Wrangler will display the difference between local and remote configuration. +If there would be a destructive change to the remote configuration, the user is given the option to cancel the deployment. +In the case where the user does cancel deployment, Wrangler will now also offer to update the local Wrangler configuration file to match the remote configuration. diff --git a/packages/wrangler/src/__tests__/deploy/get-config-patch.test.ts b/packages/wrangler/src/__tests__/deploy/get-config-patch.test.ts new file mode 100644 index 000000000000..12b0363d28d5 --- /dev/null +++ b/packages/wrangler/src/__tests__/deploy/get-config-patch.test.ts @@ -0,0 +1,253 @@ +import { getConfigPatch } from "../../deploy/config-diffs"; + +// Note: __old (as well as *__deleted) is the value in the remote config, __new is the value in the local one, so we do want the +// __old one to override the __new + +describe("getConfigPatch", () => { + test("top level config updated", () => { + expect( + getConfigPatch({ + preview_urls: { + __old: false, + __new: true, + }, + }) + ).toEqual({ + preview_urls: false, + }); + }); + + test("env var present remotely but deleted locally", () => { + expect( + getConfigPatch({ + vars: { + MY_VAR__deleted: "ABC", + }, + }) + ).toEqual({ + vars: { + MY_VAR: "ABC", + }, + }); + }); + + test("updated value of env var", () => { + expect( + getConfigPatch({ + vars: { + MY_VAR: { + __old: "ABC", + __new: "123", + }, + }, + }) + ).toEqual({ + vars: { + MY_VAR: "ABC", + }, + }); + }); + + test("env var renamed", () => { + expect( + getConfigPatch({ + vars: { + MY_VAR__deleted: "ABC", + VAR__added: "ABC", + }, + }) + ).toEqual({ + vars: { + MY_VAR: "ABC", + }, + }); + }); + + test("deleted version metadata binding", () => { + expect( + getConfigPatch({ + version_metadata: { + __old: { + binding: "VERSION_METADATA", + }, + __new: undefined, + }, + }) + ).toEqual({ + version_metadata: { + binding: "VERSION_METADATA", + }, + }); + }); + + test("deleted KV binding (only one KV)", () => { + expect( + getConfigPatch({ + kv_namespaces: [ + [ + "-", + { + id: "", + binding: "MY_KV", + }, + ], + ], + }) + ).toEqual({ + kv_namespaces: [ + { + id: "", + binding: "MY_KV", + }, + ], + }); + }); + + test("deleted second KV binding in the kv_namespaces array", () => { + expect( + getConfigPatch({ + kv_namespaces: [ + [" "], + [ + "-", + { + id: "", + binding: "MY_KV_A", + }, + ], + ], + }) + ).toEqual({ + kv_namespaces: [ + { + /* unmodified kv */ + }, + { + id: "", + binding: "MY_KV_A", + }, + ], + }); + }); + + test("modified KV binding", () => { + expect( + getConfigPatch({ + kv_namespaces: [ + [ + "~", + { + id: { + __old: "", + __new: "", + }, + }, + ], + ], + }) + ).toEqual({ + kv_namespaces: [ + { + id: "", + }, + ], + }); + }); + + test("deleted second KV binding in the kv_namespaces array and modified first one", () => { + expect( + getConfigPatch({ + kv_namespaces: [ + [" "], + [ + "-", + { + id: "", + binding: "MY_KV_A", + }, + ], + ], + }) + ).toEqual({ + kv_namespaces: [ + { + /* unmodified kv */ + }, + { + id: "", + binding: "MY_KV_A", + }, + ], + }); + }); + + test("deleted KV binding from the middle of the kv_namespaces array", () => { + expect( + getConfigPatch({ + kv_namespaces: [ + [" "], + [ + "-", + { + id: "", + binding: "MY_KV_A", + }, + ], + [" "], + ], + }) + ).toEqual({ + kv_namespaces: [ + { + /* unmodified kv */ + }, + { + /* unmodified kv */ + }, + { + /* deleted kv put back */ + id: "", + binding: "MY_KV_A", + }, + ], + }); + }); + + test("flipped observability.logs.invocation_logs off (nested field)", () => { + expect( + getConfigPatch({ + observability: { + logs: { + invocation_logs: { + __old: true, + __new: false, + }, + }, + }, + }) + ).toEqual({ + observability: { + logs: { + invocation_logs: true, + }, + }, + }); + }); + + test("renamed version metadata binding", () => { + expect( + getConfigPatch({ + version_metadata: { + binding: { + __old: "VERSION_METADATA", + __new: "VERSION_META", + }, + }, + }) + ).toEqual({ + version_metadata: { + binding: "VERSION_METADATA", + }, + }); + }); +}); diff --git a/packages/wrangler/src/__tests__/deploy/config-diffs.test.ts b/packages/wrangler/src/__tests__/deploy/get-remote-config-diff.test.ts similarity index 92% rename from packages/wrangler/src/__tests__/deploy/config-diffs.test.ts rename to packages/wrangler/src/__tests__/deploy/get-remote-config-diff.test.ts index 8d809f903d17..da555cf3efb0 100644 --- a/packages/wrangler/src/__tests__/deploy/config-diffs.test.ts +++ b/packages/wrangler/src/__tests__/deploy/get-remote-config-diff.test.ts @@ -47,6 +47,39 @@ describe("getRemoteConfigsDiff", () => { expect(nonDestructive).toBe(true); }); + it("should handle a very simple diffing scenario where there is only an addition to an array (specifically in `kv_namespaces`)", () => { + const { diff, nonDestructive } = getRemoteConfigDiff( + { + name: "silent-firefly-dbe3", + main: "/tmp/src/index.js", + workers_dev: true, + kv_namespaces: [{ binding: "MY_KV", id: "" }], + preview_urls: true, + }, + { + name: "silent-firefly-dbe3", + main: "/tmp/src/index.js", + workers_dev: true, + preview_urls: true, + kv_namespaces: [], + } as unknown as Config + ); + + assert(diff); + expect(normalizeDiff(diff.toString())).toMatchInlineSnapshot(` + " { + kv_namespaces: [ + - { + - binding: \\"MY_KV\\" + - id: \\"\\" + - } + ] + } + " + `); + expect(nonDestructive).toBe(false); + }); + it("should handle a very simple diffing scenario (some diffs, random order)", () => { const { diff, nonDestructive } = getRemoteConfigDiff( { diff --git a/packages/wrangler/src/deploy/config-diffs.ts b/packages/wrangler/src/deploy/config-diffs.ts index 18663e79a396..c0705c2c2084 100644 --- a/packages/wrangler/src/deploy/config-diffs.ts +++ b/packages/wrangler/src/deploy/config-diffs.ts @@ -1,14 +1,19 @@ +import assert from "node:assert"; import { getSubdomainValuesAPIMock } from "../triggers/deploy"; -import { diffJsonObjects, isNonDestructive } from "../utils/diff-json"; +import { + diffJsonObjects, + isModifiedDiffValue, + isNonDestructive, +} from "../utils/diff-json"; import type { Config, RawConfig } from "../config"; -import type { DiffJson, Json } from "../utils/diff-json"; +import type { JsonLike } from "../utils/diff-json"; /** * Object representing the difference of two configuration objects. */ type ConfigDiff = { /** The actual (raw) computed diff of the two objects */ - diff: Record | null; + diff: Record | null; /** * Flag indicating whether the difference includes some destructive changes. * @@ -36,8 +41,8 @@ export function getRemoteConfigDiff( ); const diff = diffJsonObjects( - normalizedRemoteConfig as unknown as Record, - normalizedLocalConfig as unknown as Record + normalizedRemoteConfig as unknown as Record, + normalizedLocalConfig as unknown as Record ); return { @@ -280,3 +285,121 @@ function orderObjectFields>( return orderedSource; } + +/** + * Given a config diff generates a patch object that can be passed to `experimental_patchConfig` to revert the + * changes in the config object that are described by the config diff. + * + * @param configDiff The target config diff + * @returns The patch object to pass to `experimental_patchConfig` to revert the changes + */ +export function getConfigPatch(configDiff: ConfigDiff["diff"]): RawConfig { + const patchObj: RawConfig = {}; + + populateConfigPatch(configDiff, patchObj as Record); + + return patchObj; +} + +/** + * Recursive call for `getConfigPatch`, it side-effectfully populates the patch object at the current level + * + * @param diff The current section of the config diff that is being analyzed + * @param patchObj The current section of the patch object that is being populated + */ +function populateConfigPatch( + diff: JsonLike, + patchObj: Record | JsonLike[] +): void { + if (!diff || typeof diff !== "object") { + return; + } + + if (Array.isArray(diff)) { + // This is a recursive call since we're populating the + // patchObj we know that it is an array + assert(Array.isArray(patchObj)); + return populateConfigPatchArray(diff, patchObj); + } + + // We know that patchObj is not an array here + assert(!Array.isArray(patchObj)); + return populateConfigPatchObject(diff, patchObj); +} + +/** + * Recursive call for `getConfigPatch`, it side-effectfully populates the array present at the config patch level + * + * @param diff The current section of the config diff that is being analyzed + * @param patchArray The current section of the patch object that is being populated + */ +function populateConfigPatchArray(diff: JsonLike[], patchArray: JsonLike[]) { + // We create a temporary array since removed elements should be pushed back at the end + const elementsToAppend: JsonLike[] = []; + + Object.values(diff).forEach((element) => { + if (!Array.isArray(element)) { + return; + } + + if (element.length === 1 && element[0] === " ") { + // An array with a single element equal to a simple space indicates + // that the element hasn't been modified + patchArray.push({}); + return; + } + + if (element.length === 2) { + if (element[0] === "-") { + elementsToAppend.push(element[1]); + return; + } + + if (element[0] === "~" && element[1]) { + const patchEl = {}; + populateConfigPatch(element[1], patchEl); + patchArray.push(patchEl); + return; + } + } + }); + elementsToAppend.forEach((el) => patchArray.push(el)); +} + +/** + * Recursive call for `getConfigPatch`, it side-effectfully populates the object present at the config patch level + * + * @param diff The current section of the config diff that is being analyzed + * @param patchObj The current section of the patch object that is being populated + */ +function populateConfigPatchObject( + diff: { [id: string]: JsonLike }, + patchObj: Record +) { + Object.keys(diff) + .filter((key) => diff[key] && typeof diff[key] === "object") + .forEach((key) => { + if (isModifiedDiffValue(diff[key])) { + patchObj[key] = diff[key].__old; + return; + } + + patchObj[key] = Array.isArray(diff[key]) ? [] : {}; + + Object.entries(diff[key] as Record).forEach( + ([entryKey, entryValue]) => { + if (entryKey.endsWith("__deleted")) { + (patchObj[key] as Record)[ + entryKey.replace("__deleted", "") + ] = entryValue; + return; + } + } + ); + + if (diff[key] && typeof diff[key] === "object") { + populateConfigPatch(diff[key], patchObj[key]); + return; + } + }); +} diff --git a/packages/wrangler/src/deploy/deploy.ts b/packages/wrangler/src/deploy/deploy.ts index 5453d7e8a6ef..86d071d758a4 100644 --- a/packages/wrangler/src/deploy/deploy.ts +++ b/packages/wrangler/src/deploy/deploy.ts @@ -10,6 +10,7 @@ import { syncAssets } from "../assets"; import { fetchListResult, fetchResult } from "../cfetch"; import { buildContainer, deployContainers } from "../cloudchamber/deploy"; import { configFileName, formatConfigSnippet } from "../config"; +import { experimental_patchConfig } from "../config/patch-config"; import { getNormalizedContainerOptions } from "../containers/config"; import { getBindings, provisionBindings } from "../deployment-bundle/bindings"; import { bundleWorker } from "../deployment-bundle/bundle"; @@ -63,9 +64,9 @@ import { } from "../versions/api"; import { confirmLatestDeploymentOverwrite } from "../versions/deploy"; import { getZoneForRoute } from "../zones"; -import { getRemoteConfigDiff } from "./config-diffs"; +import { getConfigPatch, getRemoteConfigDiff } from "./config-diffs"; import type { AssetsOptions } from "../assets"; -import type { Config } from "../config"; +import type { Config, RawConfig } from "../config"; import type { CustomDomainRoute, Route, @@ -422,6 +423,35 @@ export default async function deploy(props: Props): Promise<{ "Deploying the Worker will override the remote configuration with your local one." ); if (!(await deployConfirm("Would you like to continue?"))) { + if ( + config.userConfigPath && + /\.jsonc?$/.test(config.userConfigPath) + ) { + const targetingEnvironment = !!props.env; + + if ( + // TODO: Currently if the user is targeting an environment we don't offer them to update + // their config file since that is fairly nuanced, we should also support environments + // (the best we can) here + !targetingEnvironment && + (await confirm( + "Would you like to update the local config file with the conflicting remote values?", + { + defaultValue: true, + fallbackValue: true, + } + )) + ) { + const patchObj: RawConfig = getConfigPatch(configDiff.diff); + + experimental_patchConfig( + config.userConfigPath, + patchObj, + false + ); + } + } + return { versionId, workerTag }; } } diff --git a/packages/wrangler/src/utils/diff-json.ts b/packages/wrangler/src/utils/diff-json.ts index 83c13149be26..af546ccaf8eb 100644 --- a/packages/wrangler/src/utils/diff-json.ts +++ b/packages/wrangler/src/utils/diff-json.ts @@ -1,14 +1,13 @@ import jsonDiff from "json-diff"; -export type Json = +export type JsonLike = | string | number | boolean | null - | Json[] - | { [id: string]: Json }; - -export type DiffJson = Json | (Json & { __old: DiffJson; __new: DiffJson }); + | JsonLike[] + | undefined // undefined is not a JSON type but it needs to be included here since it is present in the diff objects + | { [id: string]: JsonLike }; /** * Given two objects A and B that are Json serializable this function computes the difference between them @@ -26,9 +25,9 @@ export type DiffJson = Json | (Json & { __old: DiffJson; __new: DiffJson }); * @returns An object representing the diff between the two objects, or null if the objects are equal */ export function diffJsonObjects( - jsonObjA: Record, - jsonObjB: Record -): Record | null { + jsonObjA: Record, + jsonObjB: Record +): Record | null { const result = jsonDiff.diff(jsonObjA, jsonObjB); if (result) { @@ -47,7 +46,7 @@ export function diffJsonObjects( * @param diff The difference object to use (generated by `diffJsonObjects`) * @returns `true` if the difference is non-destructive, `false` if it is */ -export function isNonDestructive(diff: DiffJson): boolean { +export function isNonDestructive(diff: JsonLike): boolean { if (diff === null || typeof diff !== "object") { return true; } @@ -61,8 +60,32 @@ export function isNonDestructive(diff: DiffJson): boolean { } if (Array.isArray(diff)) { - for (const field of diff) { - if (!isNonDestructive(field)) { + for (const element of diff) { + if (Array.isArray(element) && element.length === 2) { + if (element[0] === "-") { + // json-diff shows a removed element by representing it as the following array: + // ["-", ], so if the first value here is "-" we assume that this is + // a removal + return false; + } else if (element[0] === "~") { + // json-diff shows a modified element by representing it as the following array: + // ["~", ], so if the first value here is "~" we assume that this is + // a modification + return false; + } else if (element[0] !== "+") { + // json-diff shows an added element by representing it as the following array: + // ["+", ], so if the first value here is "+" we assume that this is + // an addition (so we skip this) + continue; + } + + // Otherwise we check all the elements in the array + for (const innerElement of element) { + if (!isNonDestructive(innerElement)) { + return false; + } + } + } else if (!isNonDestructive(element)) { return false; } } @@ -76,3 +99,25 @@ export function isNonDestructive(diff: DiffJson): boolean { return true; } + +/** + * A modified value in json-diff is represented as an object with two properties: + * `__old` and `__new`. Where the former contains the old version of the value and + * the latter the new one. + * This utility, given an arbitrary value, discerns whether the value represents the + * diff of a modified value. + * + * @param value The target value to check + * @returns True if the value represents a value modified, false otherwise + */ +export function isModifiedDiffValue( + value: unknown +): value is { __old: T; __new: T } { + return !!( + value && + typeof value === "object" && + Object.keys(value).length === 2 && + "__old" in value && + "__new" in value + ); +}