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, }, }); }