diff --git a/CHANGELOG.md b/CHANGELOG.md index 3087b55b9..c631dfb1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,11 @@ `coder update` non-interactively) now passes newly-required template parameters into the REST-API fallback build, instead of silently omitting them and letting the server reject the build. +- Updating a workspace now re-prompts for an option or multi-select + parameter whose stored value is no longer one of the new template + version's options, instead of carrying a stale value forward and + failing the build. Immutable parameters without a stored value are + now prompted as well, closing a gap with the web dashboard. - Updating a workspace from VS Code no longer hangs when the new template version requires parameters. The extension now prompts for any missing required values through VS Code input boxes and passes them to diff --git a/src/api/updateParameters.ts b/src/api/updateParameters.ts index 8e24ba736..eabfe0593 100644 --- a/src/api/updateParameters.ts +++ b/src/api/updateParameters.ts @@ -16,9 +16,9 @@ export class WorkspaceUpdateCancelledError extends Error { } /** - * Prompts the user for any newly-required template parameters and returns the - * collected `{ name, value }` pairs. Throws `WorkspaceUpdateCancelledError` if - * the user dismisses a prompt. + * Prompts the user for any template parameters that the new version needs + * answered, and returns the collected `{ name, value }` pairs. Throws + * `WorkspaceUpdateCancelledError` if the user dismisses a prompt. */ export async function collectUpdateParameters( restClient: Api, @@ -30,16 +30,18 @@ export async function collectUpdateParameters( ), restClient.getWorkspaceBuildParameters(workspace.latest_build.id), ]); - const candidates = newParams.filter((p) => p.required && !p.default_value); - if (candidates.length === 0) return []; - - const existing = new Set(currentValues.map((p) => p.name)); - const toPrompt = candidates.filter((p) => !existing.has(p.name)); + const stored = new Map(currentValues.map((p) => [p.name, p.value])); + const toPrompt = newParams.filter((p) => needsPrompt(p, stored.get(p.name))); const collected: WorkspaceBuildParameter[] = []; for (let i = 0; i < toPrompt.length; i++) { const param = toPrompt[i]; - const value = await promptForParameter(param, i + 1, toPrompt.length); + const value = await promptForParameter( + param, + i + 1, + toPrompt.length, + stored.get(param.name), + ); if (value === undefined) { throw new WorkspaceUpdateCancelledError(); } @@ -48,10 +50,48 @@ export async function collectUpdateParameters( return collected; } +/** Based on the dashboard's `getMissingParameters` (coder/site/src/api/api.ts). */ +function needsPrompt( + param: TemplateVersionParameter, + storedValue: string | undefined, +): boolean { + if (storedValue === undefined) { + // Accepting the default locks it in. + if (!param.mutable) return true; + // Deviation: dashboard prompts mutable+required even when a default + // exists; we skip so the server can apply the default during + // auto-update without surfacing a modal. + return param.required && !param.default_value; + } + if (param.options.length === 0) return false; + + const validValues = new Set(param.options.map((o) => o.value)); + if (param.form_type === "multi-select") { + // Beyond dashboard: detect multi-select drift too. + const picks = parseMultiSelectValue(storedValue); + return picks === null || picks.some((v) => !validValues.has(v)); + } + return !validValues.has(storedValue); +} + +/** Multi-select values are stored as a JSON-encoded string array. */ +function parseMultiSelectValue(raw: string): string[] | null { + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return null; + } + return Array.isArray(parsed) && parsed.every((v) => typeof v === "string") + ? parsed + : null; +} + function promptForParameter( param: TemplateVersionParameter, step: number, totalSteps: number, + storedValue: string | undefined, ): Promise { const title = param.display_name || param.name; const items = quickPickItems(param); @@ -66,11 +106,16 @@ function promptForParameter( qp.items = items; qp.canSelectMany = multi; qp.ignoreFocusOut = true; + if (multi && storedValue !== undefined) { + const previous = new Set(parseMultiSelectValue(storedValue) ?? []); + qp.selectedItems = items.filter((item) => previous.has(item.value)); + } return collectInput(qp, () => { if (multi) { - return qp.selectedItems.length > 0 - ? JSON.stringify(qp.selectedItems.map((i) => i.value)) - : undefined; + if (qp.selectedItems.length === 0) { + return param.required ? undefined : "[]"; + } + return JSON.stringify(qp.selectedItems.map((i) => i.value)); } return qp.selectedItems[0]?.value; }); diff --git a/test/unit/api/updateParameters.test.ts b/test/unit/api/updateParameters.test.ts index c00c5a33c..a11b697d2 100644 --- a/test/unit/api/updateParameters.test.ts +++ b/test/unit/api/updateParameters.test.ts @@ -222,6 +222,266 @@ describe("collectUpdateParameters", () => { expect(vscode.window.createInputBox).not.toHaveBeenCalled(); }); + it("prompts an immutable param that has no stored value (even if not required)", async () => { + const { restClient, workspace } = createCollectCtx([ + { name: "zone", required: false, mutable: false }, + ]); + const qi = mockCreateInputBox(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ value: "us-east" }); + + await expect(result).resolves.toEqual([{ name: "zone", value: "us-east" }]); + }); + + it("prompts an immutable param even when a template default exists", async () => { + const { restClient, workspace } = createCollectCtx([ + { name: "zone", mutable: false, default_value: "us-east" }, + ]); + const qi = mockCreateInputBox(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ value: "us-west" }); + + await expect(result).resolves.toEqual([{ name: "zone", value: "us-west" }]); + }); + + it("skips an option param whose stored value is still in the new options", async () => { + const optionParam: Partial = { + name: "size", + options: [ + { name: "Small", description: "", value: "s", icon: "" }, + { name: "Large", description: "", value: "l", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [optionParam], + [{ name: "size", value: "l" }], + ); + + await expect( + collectUpdateParameters(restClient, workspace), + ).resolves.toEqual([]); + expect(vscode.window.createQuickPick).not.toHaveBeenCalled(); + }); + + it("re-prompts when a stored option value drifted out of the new options", async () => { + const optionParam: Partial = { + name: "size", + options: [ + { name: "Small", description: "", value: "s", icon: "" }, + { name: "Large", description: "", value: "l", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [optionParam], + [{ name: "size", value: "xl-retired" }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "l" }] }); + + await expect(result).resolves.toEqual([{ name: "size", value: "l" }]); + }); + + it("re-prompts on drift even when the template parameter has a default value", async () => { + const optionParam: Partial = { + name: "size", + default_value: "s", + options: [ + { name: "Small", description: "", value: "s", icon: "" }, + { name: "Large", description: "", value: "l", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [optionParam], + [{ name: "size", value: "xl-retired" }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "l" }] }); + + await expect(result).resolves.toEqual([{ name: "size", value: "l" }]); + }); + + it("re-prompts on drift for an immutable param too", async () => { + const optionParam: Partial = { + name: "zone", + mutable: false, + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [optionParam], + [{ name: "zone", value: "eu-retired" }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "us" }] }); + + await expect(result).resolves.toEqual([{ name: "zone", value: "us" }]); + }); + + it("re-prompts a multi-select when any stored pick drifted out", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [ + { name: "US", description: "", value: "us", icon: "" }, + { name: "EU", description: "", value: "eu", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["us","apac-retired"]' }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "us" }] }); + + await expect(result).resolves.toEqual([ + { name: "regions", value: '["us"]' }, + ]); + }); + + it("re-prompts a multi-select when the stored value isn't a JSON string array", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: "not-json" }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [{ value: "us" }] }); + + await expect(result).resolves.toEqual([ + { name: "regions", value: '["us"]' }, + ]); + }); + + it("skips a multi-select when every stored pick is still in the new options", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [ + { name: "US", description: "", value: "us", icon: "" }, + { name: "EU", description: "", value: "eu", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["us","eu"]' }], + ); + + await expect( + collectUpdateParameters(restClient, workspace), + ).resolves.toEqual([]); + expect(vscode.window.createQuickPick).not.toHaveBeenCalled(); + }); + + it("skips a multi-select when the stored value is an empty JSON array", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: "[]" }], + ); + + await expect( + collectUpdateParameters(restClient, workspace), + ).resolves.toEqual([]); + expect(vscode.window.createQuickPick).not.toHaveBeenCalled(); + }); + + it("pre-selects still-valid picks on a multi-select drift re-prompt", async () => { + const multiParam: Partial = { + name: "regions", + form_type: "multi-select", + options: [ + { name: "US", description: "", value: "us", icon: "" }, + { name: "EU", description: "", value: "eu", icon: "" }, + ], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["us","apac-retired"]' }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + expect(qi.mock.selectedItems).toEqual([ + { label: "US", description: "", value: "us" }, + ]); + qi.accept({ selectedItems: [{ value: "us" }, { value: "eu" }] }); + + await expect(result).resolves.toEqual([ + { name: "regions", value: '["us","eu"]' }, + ]); + }); + + it("keeps the prompt open on an empty submission for a required multi-select", async () => { + const multiParam: Partial = { + name: "regions", + required: true, + form_type: "multi-select", + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["apac-retired"]' }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [] }); + expect(qi.mock.dispose).not.toHaveBeenCalled(); + qi.accept({ selectedItems: [{ value: "us" }] }); + + await expect(result).resolves.toEqual([ + { name: "regions", value: '["us"]' }, + ]); + }); + + it("accepts an empty selection on a non-required multi-select re-prompt", async () => { + const multiParam: Partial = { + name: "regions", + required: false, + form_type: "multi-select", + options: [{ name: "US", description: "", value: "us", icon: "" }], + }; + const { restClient, workspace } = createCollectCtx( + [multiParam], + [{ name: "regions", value: '["apac-retired"]' }], + ); + const qi = mockCreateQuickPick(); + + const result = collectUpdateParameters(restClient, workspace); + await waitShown(qi); + qi.accept({ selectedItems: [] }); + + await expect(result).resolves.toEqual([{ name: "regions", value: "[]" }]); + }); + it("throws WorkspaceUpdateCancelledError when the prompt is dismissed", async () => { const { restClient, workspace } = createCollectCtx([{}]); const qi = mockCreateInputBox();