From 67da34668f4436aa58f148604b1ae831d2a39122 Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:08:24 +0200 Subject: [PATCH] feat(cli): sync variable expiry and the workspace expiration handler `wmill sync` carries `value_expires_at` on variables in both directions, and pushes the workspace handler alongside the error and success handlers in the grouped settings format. Dropping the key from a variable's yaml clears the remote expiry, so the file stays canonical rather than accumulating fields that can only ever be added. The handler follows the same rule, with the caveat recorded where the type is declared: an omitted handler only reaches the clear when some other setting differs, because the push short-circuits on a comparison that walks the local object's keys and so never sees a missing one. That gate is shared with the two existing handlers and is left as it is here. Co-Authored-By: Claude Opus 5 (1M context) --- cli/src/commands/variable/variable.ts | 13 +++- cli/src/core/settings.ts | 47 +++++++++++++- cli/test/deploy_variable_expiry_unit.test.ts | 64 ++++++++++++++++++++ cli/test/variable_resource_push.test.ts | 62 +++++++++++++++++++ cli/windmill-utils-internal/src/deploy.ts | 5 ++ 5 files changed, 188 insertions(+), 3 deletions(-) create mode 100644 cli/test/deploy_variable_expiry_unit.test.ts diff --git a/cli/src/commands/variable/variable.ts b/cli/src/commands/variable/variable.ts index 2d885c9dd5..bb7d0dffb1 100644 --- a/cli/src/commands/variable/variable.ts +++ b/cli/src/commands/variable/variable.ts @@ -97,6 +97,7 @@ export interface VariableFile { description: string; account?: number; is_oauth?: boolean; + value_expires_at?: string; } /** @@ -153,7 +154,16 @@ export async function pushVariable( } if (variable) { - if (isSuperset(localVariable, variable)) { + // The server reads an absent `value_expires_at` as "leave the stored date alone", so + // dropping the key from the file has to be sent as an explicit `null`. `isSuperset` + // only compares keys the file still has, so a removed expiry is also invisible to it: + // without both halves the clear never applies and every later push re-reports it. + const clearsValueExpiresAt = + localVariable.value_expires_at === undefined && + variable.value_expires_at !== undefined && + variable.value_expires_at !== null; + + if (!clearsValueExpiresAt && isSuperset(localVariable, variable)) { log.debug(`Variable ${remotePath} is up-to-date`); return; } @@ -181,6 +191,7 @@ export async function pushVariable( ...localVariable, is_secret: nextIsSecret, ...(wsSpecific !== undefined ? { ws_specific: wsSpecific } : {}), + ...(clearsValueExpiresAt ? { value_expires_at: null } : {}), }, }); } else { diff --git a/cli/src/core/settings.ts b/cli/src/core/settings.ts index 66fc77d230..552c57cf6b 100644 --- a/cli/src/core/settings.ts +++ b/cli/src/core/settings.ts @@ -34,12 +34,21 @@ export interface SuccessHandlerConfig { extra_args?: any; } +export interface VariableExpirationHandlerConfig { + path?: string; + extra_args?: any; + muted_on_user_path?: boolean; +} + export interface SimplifiedSettings { - // Grouped format (current). Explicit `null` on error_handler / success_handler - // signals "clear the remote value"; `undefined` means "not managed by git". + // Grouped format (current). On push the YAML is canonical: a handler that is + // absent or explicitly `null` clears the remote value, a present object upserts. + // An omitted handler reaches that clear only when some other setting differs: + // `isSuperset` compares local's own keys, so it never sees the missing one. auto_invite?: AutoInviteConfig; error_handler?: ErrorHandlerConfig | null; success_handler?: SuccessHandlerConfig | null; + variable_expiration_handler?: VariableExpirationHandlerConfig | null; // Other fields webhook?: string; @@ -150,6 +159,17 @@ export function migrateToGroupedFormat(settings: any): SimplifiedSettings { }; } + // Handle variable_expiration_handler: same null-clears semantics. It has no flat + // legacy form — the setting postdates the grouped format. + if (settings.variable_expiration_handler === null) { + result.variable_expiration_handler = null; + } else if ( + settings.variable_expiration_handler && + typeof settings.variable_expiration_handler === "object" + ) { + result.variable_expiration_handler = settings.variable_expiration_handler; + } + return result; } @@ -192,6 +212,9 @@ export async function pushWorkspaceSettings( auto_invite: remoteSettings.auto_invite as AutoInviteConfig | undefined, error_handler: remoteSettings.error_handler as ErrorHandlerConfig | undefined, success_handler: remoteSettings.success_handler as SuccessHandlerConfig | undefined, + variable_expiration_handler: remoteSettings.variable_expiration_handler as + | VariableExpirationHandlerConfig + | undefined, webhook: remoteSettings.webhook, ai_config: remoteSettings.ai_config, large_file_storage: remoteSettings.large_file_storage, @@ -311,6 +334,26 @@ export async function pushWorkspaceSettings( }); } + // Handle variable_expiration_handler using grouped format. Same semantics as error_handler. + if ( + !deepEqual( + localSettings.variable_expiration_handler, + settings.variable_expiration_handler + ) + ) { + log.debug(`Updating variable expiration handler...`); + const localVariableExpirationHandler = localSettings.variable_expiration_handler; + await wmill.editVariableExpirationHandler({ + workspace, + requestBody: { + path: localVariableExpirationHandler?.path, + extra_args: localVariableExpirationHandler?.extra_args, + muted_on_user_path: + localVariableExpirationHandler?.muted_on_user_path ?? false, + }, + }); + } + if ( !deepEqual(localSettings.large_file_storage, settings.large_file_storage) ) { diff --git a/cli/test/deploy_variable_expiry_unit.test.ts b/cli/test/deploy_variable_expiry_unit.test.ts new file mode 100644 index 0000000000..96c7441d8c --- /dev/null +++ b/cli/test/deploy_variable_expiry_unit.test.ts @@ -0,0 +1,64 @@ +import { expect, test } from "bun:test"; +import { deployItem } from "../windmill-utils-internal/src/deploy.ts"; + +// Workspace deploy builds the variable body field by field, so a field the variable +// carries but the body forgets is dropped on promotion — silently, since the deploy +// still reports success. The clear half matters as much as the carry: the server reads +// an absent `value_expires_at` as "leave the stored date alone", so a source with no +// expiry has to say so explicitly or the target keeps a date the source no longer has. +function recordingProvider( + captured: [string, any][], + exists: boolean, + value_expires_at: string | null, +) { + return { + existsVariable: async () => exists, + getVariable: async () => ({ + path: "f/x/v", + value: "secret", + is_secret: true, + description: "d", + value_expires_at, + }), + createVariable: async (p: any) => + void captured.push(["createVariable", p.requestBody]), + updateVariable: async (p: any) => + void captured.push(["updateVariable", p.requestBody]), + } as any; +} + +test("deployItem: carries value_expires_at, and clears it explicitly", async () => { + const captured: [string, any][] = []; + + // The field is written out once per branch, so exercise both: a variable absent from + // the target (create) and one already there (update). + for (const exists of [false, true]) { + await deployItem( + recordingProvider(captured, exists, "2027-03-15T08:00:00Z"), + "variable" as any, + "f/x/v", + "src", + "dst", + ); + await deployItem( + recordingProvider(captured, exists, null), + "variable" as any, + "f/x/v", + "src", + "dst", + ); + } + + expect(captured.map(([fn]) => fn)).toEqual([ + "createVariable", + "createVariable", + "updateVariable", + "updateVariable", + ]); + expect(captured.map(([, body]) => body.value_expires_at)).toEqual([ + "2027-03-15T08:00:00Z", + null, + "2027-03-15T08:00:00Z", + null, + ]); +}); diff --git a/cli/test/variable_resource_push.test.ts b/cli/test/variable_resource_push.test.ts index e6bc84c326..82b7bd7f36 100644 --- a/cli/test/variable_resource_push.test.ts +++ b/cli/test/variable_resource_push.test.ts @@ -133,6 +133,68 @@ describe("variable", () => { }); }); + test("push clears value_expires_at when the key is dropped from the file", async () => { + await withTestBackend(async (backend, tempDir) => { + await setupWorkspaceProfile(backend); + + const uniqueId = Date.now(); + const path = `f/test/expiry_var_${uniqueId}`; + + const createResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/variables/create`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + path, + value: "original_value", + is_secret: false, + description: "Expiring", + value_expires_at: "2027-03-15T08:00:00Z", + }), + } + ); + expect(createResp.status).toBeLessThan(300); + await createResp.text(); + + await writeFile( + join(tempDir, "wmill.yaml"), + `defaultTs: bun\nincludes:\n - "**"\nexcludes: []\n`, + "utf-8" + ); + await mkdir(join(tempDir, "f", "test"), { recursive: true }); + // No value_expires_at: the server reads an absent field as "leave the stored + // date alone", so the clear only lands if the CLI sends an explicit null. + await writeFile( + join(tempDir, `${path}.variable.yaml`), + `value: "original_value"\nis_secret: false\ndescription: "Expiring"\n`, + "utf-8" + ); + + const pushResult = await backend.runCLICommand( + ["sync", "push", "--yes", "--includes", `${path}**`], + tempDir + ); + expect(pushResult.code).toEqual(0); + + const apiResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/variables/get/${path}` + ); + expect(apiResp.status).toEqual(200); + const varData = await apiResp.json(); + expect(varData.value_expires_at ?? null).toBeNull(); + + // A clear that does not apply leaves the same diff pending forever, so the + // push reporting success is not on its own evidence that it landed. + const secondPush = await backend.runCLICommand( + ["sync", "push", "--yes", "--includes", `${path}**`], + tempDir + ); + expect(secondPush.code).toEqual(0); + expect(secondPush.stdout).toContain("0 changes to apply"); + }); + }); + test("add creates secret by default and preserves fields on update", async () => { await withTestBackend(async (backend, tempDir) => { await setupWorkspaceProfile(backend); diff --git a/cli/windmill-utils-internal/src/deploy.ts b/cli/windmill-utils-internal/src/deploy.ts index 047d50ee78..fb16a0b068 100644 --- a/cli/windmill-utils-internal/src/deploy.ts +++ b/cli/windmill-utils-internal/src/deploy.ts @@ -598,6 +598,10 @@ export async function deployItem( value: variable.value ?? "", is_secret: variable.is_secret, description: variable.description ?? "", + // Sent as an explicit null when the source has no expiry: an absent key means + // "leave the stored date alone", which would strand the target on a date the + // source has already moved or cleared. + value_expires_at: variable.value_expires_at ?? null, }, alreadyEncrypted: false, }); @@ -609,6 +613,7 @@ export async function deployItem( value: variable.value ?? "", is_secret: variable.is_secret, description: variable.description ?? "", + value_expires_at: variable.value_expires_at ?? null, }, }); }