From ef4dc46d4bfd00e583a39e5c053c0022f1ad3abd Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 18 Aug 2026 12:26:41 +0200 Subject: [PATCH] fix(cli): keep script settings on push and repair the up-to-date check (#10741) * fix(cli): keep script retention, debounce and cache settings on push Co-Authored-By: Claude Opus 5 (1M context) * test(cli): surface the create response when the fixture fails Co-Authored-By: Claude Opus 5 (1M context) * test(cli): drop debounce settings the CI build refuses to accept Co-Authored-By: Claude Opus 5 (1M context) * repair the script push up-to-date comparison (#10743) * test: settle the backlog before the capped audit-export drain (#10737) * test: settle the backlog before the capped audit-export drain * chore: update ee-repo-ref to bd4de74eb37b32a2b6c7c69f6dedac031ef8436b This commit updates the EE repository reference after PR #730 was merged in windmill-ee-private. Previous ee-repo-ref: b5a5f9114df26088cfe976d91f10e55ba8bfcaa6 New ee-repo-ref: bd4de74eb37b32a2b6c7c69f6dedac031ef8436b Automated by sync-ee-ref workflow. --------- Co-authored-by: windmill-internal-app[bot] * fix(cli): repair the script push up-to-date comparison Co-Authored-By: Claude Opus 5 (1M context) * test(cli): drain dependency jobs and pin a non-1 priority skip Co-Authored-By: Claude Opus 5 (1M context) * test(cli): describe the priority fixture without the old comparison Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: windmill-internal-app[bot] Co-authored-by: Claude Opus 5 (1M context) * refactor(cli): read cache_ignore_s3_path off the typed response Co-Authored-By: Claude Opus 5 (1M context) * fix(cli): stop redeploying bunnative scripts on every push Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: windmill-internal-app[bot] --- backend/windmill-api/src/workspaces_export.rs | 3 + cli/src/commands/script/script.ts | 25 ++- cli/test/script_push_up_to_date.test.ts | 145 ++++++++++++++++++ cli/test/script_runtime_settings_sync.test.ts | 126 +++++++++++++++ 4 files changed, 298 insertions(+), 1 deletion(-) create mode 100644 cli/test/script_push_up_to_date.test.ts create mode 100644 cli/test/script_runtime_settings_sync.test.ts diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index 037971b19e..4e7aaf0f48 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -78,6 +78,8 @@ struct ScriptMetadata { #[serde(skip_serializing_if = "Option::is_none")] cache_ttl: Option, #[serde(skip_serializing_if = "Option::is_none")] + cache_ignore_s3_path: Option, + #[serde(skip_serializing_if = "Option::is_none")] dedicated_worker: Option, #[serde(skip_serializing_if = "is_none_or_false")] ws_error_handler_muted: Option, @@ -826,6 +828,7 @@ pub(crate) async fn tarball_workspace( concurrency_settings: script.runnable_settings.concurrency_settings, debouncing_settings: script.runnable_settings.debouncing_settings, cache_ttl: script.cache_ttl, + cache_ignore_s3_path: script.cache_ignore_s3_path, dedicated_worker: script.dedicated_worker, ws_error_handler_muted: script.ws_error_handler_muted, priority: script.priority, diff --git a/cli/src/commands/script/script.ts b/cli/src/commands/script/script.ts index a30c8d70d4..8c93bdb963 100644 --- a/cli/src/commands/script/script.ts +++ b/cli/src/commands/script/script.ts @@ -592,6 +592,7 @@ export async function handleFile( ws_error_handler_muted: typed?.ws_error_handler_muted, dedicated_worker: typed?.dedicated_worker, cache_ttl: typed?.cache_ttl, + cache_ignore_s3_path: typed?.cache_ignore_s3_path, concurrency_time_window_s: normConcurrencyTimeWindowS, concurrent_limit: normConcurrentLimit, deployment_message: message, @@ -602,8 +603,14 @@ export async function handleFile( concurrency_key: typed?.concurrency_key, debounce_key: typed?.debounce_key, debounce_delay_s: typed?.debounce_delay_s, + debounce_args_to_accumulate: typed?.debounce_args_to_accumulate, + max_total_debouncing_time: typed?.max_total_debouncing_time, + max_total_debounces_amount: typed?.max_total_debounces_amount, codebase: await codebase?.getDigest(forceTar), timeout: nonePositiveInt(typed?.timeout), + // 0 means "delete immediately after completion", so it must survive as 0 + // rather than being folded into "unset" the way the positive-only settings are. + delete_after_secs: typed?.delete_after_secs, on_behalf_of_email: typed?.on_behalf_of_email, envs: typed?.envs, modules: modules, @@ -635,6 +642,12 @@ export async function handleFile( (typed.description === remote.description && typed.summary === remote.summary && typed.kind == remote.kind && + // A `.ts` file changes language when defaultTs flips, content untouched. + // bun and bunnative share that extension, so the inferred language is always + // bun; the server derives bunnative back from the `//native` annotation in + // the content, which is compared above. + language == + (remote.language === "bunnative" ? "bun" : remote.language) && !remote.archived && (Array.isArray(remote?.lock) ? remote?.lock?.join("\n") @@ -646,6 +659,8 @@ export async function handleFile( remote.ws_error_handler_muted && typed.dedicated_worker == remote.dedicated_worker && typed.cache_ttl == remote.cache_ttl && + Boolean(typed.cache_ignore_s3_path) == + Boolean(remote.cache_ignore_s3_path) && normConcurrencyTimeWindowS == normalizeConcurrency( remote.concurrent_limit, @@ -659,15 +674,23 @@ export async function handleFile( Boolean(remote.visible_to_runner_only) && Boolean(typed.has_preprocessor) == Boolean(remote.has_preprocessor) && - typed.priority == Boolean(remote.priority) && + typed.priority == remote.priority && nonePositiveInt(typed.timeout) == nonePositiveInt(remote.timeout) && + typed.delete_after_secs == remote.delete_after_secs && //@ts-ignore typed.concurrency_key == remote["concurrency_key"] && typed.debounce_key == remote["debounce_key"] && typed.debounce_delay_s == remote["debounce_delay_s"] && + deepEqual( + typed.debounce_args_to_accumulate ?? null, + remote.debounce_args_to_accumulate ?? null + ) && + typed.max_total_debouncing_time == remote.max_total_debouncing_time && + typed.max_total_debounces_amount == remote.max_total_debounces_amount && typed.codebase == remote.codebase && (hasOnBehalfOf ? true : typed.on_behalf_of_email == remote.on_behalf_of_email) && deepEqual(typed.envs, remote.envs) && + deepEqual(typed.labels ?? null, remote.labels ?? null) && deepEqual(modules ?? null, remote.modules ?? null)) ) { log.info(colors.green(`Script ${remotePath} is up to date`)); diff --git a/cli/test/script_push_up_to_date.test.ts b/cli/test/script_push_up_to_date.test.ts new file mode 100644 index 0000000000..e4a3e91268 --- /dev/null +++ b/cli/test/script_push_up_to_date.test.ts @@ -0,0 +1,145 @@ +/** + * `wmill script push` short-circuits when the local script already matches the + * remote. The comparison has to hold in both directions: an untouched script + * deploys nothing, and every field the push body carries (labels and the language + * inferred from defaultTs included) still counts as a change. + */ + +import { expect, test } from "bun:test"; +import { writeFile, readFile, mkdir } from "node:fs/promises"; +import { withTestBackend } from "./test_backend.ts"; +import { waitForDeploymentJobs } from "./new_commands_helpers.ts"; + +test("Integration: script push skips an unchanged script and deploys a changed one", async () => { + await withTestBackend(async (backend, tempDir) => { + const uniqueId = Date.now(); + const scriptPath = `f/test/uptodate_${uniqueId}`; + const getScript = async () => + await ( + await backend.apiRequest!( + `/api/w/${backend.workspace}/scripts/get/p/${scriptPath}`, + ) + ).json(); + const push = async () => + await backend.runCLICommand(["script", "push", `${scriptPath}.ts`], tempDir); + const wmillYaml = (defaultTs: string) => + `defaultTs: ${defaultTs}\nincludes:\n - "${scriptPath}**"\nexcludes: []\n`; + + await mkdir(`${tempDir}/f/test`, { recursive: true }); + await backend.apiRequest!(`/api/w/${backend.workspace}/folders/create`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ name: "test" }), + }); + const createResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/scripts/create`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + path: scriptPath, + content: `export async function main() {\n return "Hello world";\n}`, + summary: "Test up to date", + description: "", + language: "bun", + kind: "script", + schema: { + $schema: "https://json-schema.org/draft/2020-12/schema", + type: "object", + properties: {}, + required: [], + }, + labels: ["l1"], + }), + }, + ); + expect(createResp.ok).toEqual(true); + + await writeFile(`${tempDir}/wmill.yaml`, wmillYaml("bun"), "utf-8"); + // The lock a deploy's dependency job writes is part of the comparison, so every + // pull has to happen after that job has landed or the skip races it. + await waitForDeploymentJobs(backend); + expect((await backend.runCLICommand(["sync", "pull", "--yes"], tempDir)).code).toEqual(0); + + const hashBefore = (await getScript()).hash; + expect((await push()).stdout).toContain("is up to date"); + expect((await getScript()).hash).toEqual(hashBefore); + + const metadataPath = `${tempDir}/${scriptPath}.script.yaml`; + await writeFile( + metadataPath, + (await readFile(metadataPath, "utf-8")).replace("- l1", "- l2"), + "utf-8", + ); + expect((await push()).stdout).not.toContain("is up to date"); + expect((await getScript()).labels).toEqual(["l2"]); + + // 2, not 0 or 1: those two are the values a truthiness comparison would also + // call equal, so they cannot pin that priority is compared by value. + await waitForDeploymentJobs(backend); + expect((await backend.runCLICommand(["sync", "pull", "--yes"], tempDir)).code).toEqual(0); + await writeFile(metadataPath, (await readFile(metadataPath, "utf-8")) + "priority: 2\n", "utf-8"); + expect((await push()).stdout).not.toContain("is up to date"); + expect((await getScript()).priority).toEqual(2); + await waitForDeploymentJobs(backend); + expect((await backend.runCLICommand(["sync", "pull", "--yes"], tempDir)).code).toEqual(0); + expect((await push()).stdout).toContain("is up to date"); + + await writeFile(`${tempDir}/wmill.yaml`, wmillYaml("deno"), "utf-8"); + expect((await push()).stdout).not.toContain("is up to date"); + expect((await getScript()).language).toEqual("deno"); + }); +}); + +test("Integration: an unchanged bunnative script is not redeployed", async () => { + await withTestBackend(async (backend, tempDir) => { + const uniqueId = Date.now(); + const scriptPath = `f/test/native_${uniqueId}`; + + await mkdir(`${tempDir}/f/test`, { recursive: true }); + await backend.apiRequest!(`/api/w/${backend.workspace}/folders/create`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ name: "test" }), + }); + const createResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/scripts/create`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + path: scriptPath, + // The server stores this as bunnative: the language is derived from the + // annotation, and no file extension can express it back. + content: `//native\nexport async function main() {\n return "Hello world";\n}`, + summary: "Test bunnative", + description: "", + language: "bun", + kind: "script", + }), + }, + ); + expect(createResp.ok).toEqual(true); + + await writeFile( + `${tempDir}/wmill.yaml`, + `defaultTs: bun\nincludes:\n - "${scriptPath}**"\nexcludes: []\n`, + "utf-8", + ); + await waitForDeploymentJobs(backend); + expect((await backend.runCLICommand(["sync", "pull", "--yes"], tempDir)).code).toEqual(0); + + const remote = await ( + await backend.apiRequest!( + `/api/w/${backend.workspace}/scripts/get/p/${scriptPath}`, + ) + ).json(); + expect(remote.language).toEqual("bunnative"); + + const result = await backend.runCLICommand( + ["script", "push", `${scriptPath}.ts`], + tempDir, + ); + expect(result.stdout).toContain("is up to date"); + }); +}); diff --git a/cli/test/script_runtime_settings_sync.test.ts b/cli/test/script_runtime_settings_sync.test.ts new file mode 100644 index 0000000000..2b3272b20a --- /dev/null +++ b/cli/test/script_runtime_settings_sync.test.ts @@ -0,0 +1,126 @@ +/** + * Runtime settings that live only in the script metadata file (the retention + * delay, the debouncing bounds, the cache s3-path flag) must survive a sync + * pull/push cycle. A field missing from the create_script body the CLI builds + * lands as NULL on the deployed version; one missing from its up-to-date + * comparison makes a change to it alone report as up to date and never deploy. + */ + +import { expect, test } from "bun:test"; +import { writeFile, readFile, mkdir } from "node:fs/promises"; +import { withTestBackend } from "./test_backend.ts"; +import { waitForDeploymentJobs } from "./new_commands_helpers.ts"; + +// The debounce bounds this PR also restores cannot be asserted here: a build without +// git tags reports a bare commit as its version, GIT_SEM_VERSION then falls back to +// 0.1.0, and every version-gated feature (debouncing wants 1.566.0) is refused. That is +// what CI builds, so a fixture that sets any debounce field fails at create. +const SETTINGS = { + delete_after_secs: 900, + cache_ignore_s3_path: true, +}; + +test("Integration: script runtime settings survive a sync pull/push cycle", async () => { + await withTestBackend(async (backend, tempDir) => { + const uniqueId = Date.now(); + const scriptPath = `f/test/settings_${uniqueId}`; + const getScript = async () => { + const resp = await backend.apiRequest!( + `/api/w/${backend.workspace}/scripts/get/p/${scriptPath}`, + ); + expect(resp.ok).toEqual(true); + return await resp.json(); + }; + + await mkdir(`${tempDir}/f/test`, { recursive: true }); + const folderResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/folders/create`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ name: "test" }), + }, + ); + const folderStatus = `${folderResp.status} ${await folderResp.text()}`; + + const createResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/scripts/create`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + path: scriptPath, + content: `export async function main() {\n return "Hello world";\n}`, + summary: "Test runtime settings", + description: "", + language: "bun", + kind: "script", + schema: { + $schema: "https://json-schema.org/draft/2020-12/schema", + type: "object", + properties: {}, + required: [], + }, + ...SETTINGS, + }), + }, + ); + if (!createResp.ok) { + throw new Error( + `scripts/create failed: ${createResp.status} ${await createResp.text()} ` + + `(folders/create: ${folderStatus})`, + ); + } + + await writeFile( + `${tempDir}/wmill.yaml`, + `defaultTs: bun\nincludes:\n - "${scriptPath}**"\nexcludes: []\n`, + "utf-8", + ); + + // The lock a deploy's dependency job writes is compared before the settings are, + // so a pull taken before that job lands makes the next push deploy over the lock + // instead of over the setting under test. + await waitForDeploymentJobs(backend); + const pullResult = await backend.runCLICommand(["sync", "pull", "--yes"], tempDir); + expect(pullResult.code).toEqual(0); + + const metadataPath = `${tempDir}/${scriptPath}.script.yaml`; + const pulledMetadata = await readFile(metadataPath, "utf-8"); + for (const key of Object.keys(SETTINGS)) { + expect(pulledMetadata).toContain(key); + } + + // A content-only edit must carry the settings through to the new version. + const scriptFilePath = `${tempDir}/${scriptPath}.ts`; + const originalContent = await readFile(scriptFilePath, "utf-8"); + await writeFile( + scriptFilePath, + originalContent.replace("Hello world", "Hello world modified"), + "utf-8", + ); + expect((await backend.runCLICommand(["sync", "push", "--yes"], tempDir)).code).toEqual(0); + + const afterContentPush = await getScript(); + expect(afterContentPush.content).toContain("Hello world modified"); + for (const [key, value] of Object.entries(SETTINGS)) { + expect(afterContentPush[key]).toEqual(value); + } + + // A settings-only edit must reach the remote rather than be skipped as up to + // date. 0 is "delete immediately after completion", not "unset". + await waitForDeploymentJobs(backend); + expect((await backend.runCLICommand(["sync", "pull", "--yes"], tempDir)).code).toEqual(0); + await writeFile( + metadataPath, + (await readFile(metadataPath, "utf-8")).replace( + `delete_after_secs: ${SETTINGS.delete_after_secs}`, + "delete_after_secs: 0", + ), + "utf-8", + ); + expect((await backend.runCLICommand(["sync", "push", "--yes"], tempDir)).code).toEqual(0); + + expect((await getScript()).delete_after_secs).toEqual(0); + }); +});