diff --git a/packages/cdktn-cli/src/bin/cmds/handlers.ts b/packages/cdktn-cli/src/bin/cmds/handlers.ts index 2bd396eb4..dcc66b43e 100644 --- a/packages/cdktn-cli/src/bin/cmds/handlers.ts +++ b/packages/cdktn-cli/src/bin/cmds/handlers.ts @@ -200,7 +200,9 @@ export async function deploy(argv: any) { let outputsPath: string | undefined = undefined; - let onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void = () => {}; + let onOutputsRetrieved: ( + outputs: NestedTerraformOutputs, + ) => void | Promise = () => {}; if (argv.outputsFile) { outputsPath = normalizeOutputPath(argv.outputsFile); @@ -523,7 +525,9 @@ export async function output(argv: any) { const skipProviderLock = argv.skipProviderLock; let outputsPath: string | undefined = undefined; - let onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void = () => {}; + let onOutputsRetrieved: ( + outputs: NestedTerraformOutputs, + ) => void | Promise = () => {}; if (argv.outputsFile) { outputsPath = normalizeOutputPath(argv.outputsFile); diff --git a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts index e2627ab05..ade7ad2cd 100644 --- a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts +++ b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts @@ -264,6 +264,122 @@ describe("runDeploy output rendering is non-fatal", () => { }); }); +describe("runDeploy --outputs-file write failures are fatal", () => { + const outputsByConstructId = { + db: { host: { sensitive: false, type: "string", value: "db.example.com" } }, + }; + + beforeEach(() => { + mockRunCdktfProject.mockImplementation(async () => ({ + returnValue: undefined, + project: { outputsByConstructId }, + })); + }); + + it("rejects with a clean Usage error when the write fails with ENOENT (bad path)", async () => { + // Mirrors handlers.ts wiring: `onOutputsRetrieved` is `saveOutputs`, an async function. If this + // call is not awaited, a rejection here becomes a floating, unhandled promise rejection instead + // of something `runDeploy` itself rejects with - this assertion (runDeploy REJECTS) would fail + // against code that doesn't await the call, since that code resolves normally instead. + const err: NodeJS.ErrnoException = new Error( + "ENOENT: no such file or directory, open '/missing-dir/out.json'", + ); + err.code = "ENOENT"; + const onOutputsRetrieved = jest.fn().mockRejectedValue(err); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + let caught: any; + try { + await runDeploy({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + // A bad --outputs-file path is a usage mistake, not something outside our control: it must be + // typed "Usage", not "External" (see cdktn.ts's `.fail()` handler: External/Usage errors print + // just `error.message`, everything else prints message + stack + "Collecting Debug + // Information..."; Usage errors are also excluded from Sentry crash reporting). + expect(caught.__type).toBe("Usage"); + expect(caught.message).toContain("ENOENT: no such file or directory"); + // The fs error already names the path; the prefix must not repeat it. + expect(caught.message).not.toContain("to /missing-dir/out.json:"); + expect(mockStreamStop).toHaveBeenCalledTimes(1); + + logSpy.mockRestore(); + }); + + it("rejects with a clean External error when the write fails with EACCES (permission denied)", async () => { + const err: NodeJS.ErrnoException = new Error( + "EACCES: permission denied, open '/missing-dir/out.json'", + ); + err.code = "EACCES"; + const onOutputsRetrieved = jest.fn().mockRejectedValue(err); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + let caught: any; + try { + await runDeploy({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + expect(caught.__type).toBe("External"); + expect(caught.message).toContain("EACCES: permission denied"); + + logSpy.mockRestore(); + }); + + it("does not print the 'written to' line when the write fails", async () => { + const onOutputsRetrieved = jest.fn().mockRejectedValue(new Error("boom")); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runDeploy({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any), + ).rejects.toBeDefined(); + + const printed = logSpy.mock.calls.map((call) => call[0]).join("\n"); + expect(printed).not.toContain("The outputs have been written to"); + + logSpy.mockRestore(); + }); + + it("still prints the outputs table before rejecting on a write failure", async () => { + // The deploy succeeded and the outputs already exist in memory; only persistence failed. The + // table must reach the user before the rejection, not be swallowed by the failing write. + const onOutputsRetrieved = jest.fn().mockRejectedValue(new Error("boom")); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runDeploy({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any), + ).rejects.toBeDefined(); + + const printed = stripAnsi( + logSpy.mock.calls.map((call) => call[0]).join("\n"), + ); + expect(printed).toContain("host = db.example.com"); + + logSpy.mockRestore(); + }); +}); + describe("runDeploy sentinel override routing", () => { it("routes an 'override' answer to status.override()", async () => { const override = jest.fn(); diff --git a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts new file mode 100644 index 000000000..14ae9eb16 --- /dev/null +++ b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts @@ -0,0 +1,211 @@ +// Copyright (c) HashiCorp, Inc +// SPDX-License-Identifier: MPL-2.0 +import stripAnsi from "strip-ansi"; + +// Mock the project runner so we can drive a synthetic result without spawning a real CdktfProject. +// Unlike deploy.ts (which reads `project.outputsByConstructId` after the run), output.ts's +// `runOutput` gets its outputs from `runCdktfProject`'s `returnValue`, which is whatever the +// callback passed to `runCdktfProject` resolves to - here, `project.fetchOutputs(...)`. +const mockRunCdktfProject = jest.fn(); +jest.mock("../../helper/project-runner", () => ({ + runCdktfProject: (opts: unknown, cb: unknown) => + mockRunCdktfProject(opts, cb), +})); + +// Suppress noise from the StreamRenderer in unit tests, but keep a handle on `stop` so tests can +// assert it always runs regardless of which path (fatal save error / non-fatal render error) is hit. +const mockStreamStop = jest.fn(); +jest.mock("../../helper/tty-stream", () => ({ + StreamRenderer: jest.fn().mockImplementation(() => ({ + start: jest.fn(), + stop: mockStreamStop, + setBar: jest.fn(), + clearBar: jest.fn(), + appendLog: jest.fn(), + })), +})); + +// renderOutputs defaults to the real implementation; individual tests override it via +// mockRenderOutputs.mockImplementationOnce(...) to simulate a rendering failure. +const actualFormat = jest.requireActual("../../helper/format"); +const mockRenderOutputs = jest.fn(actualFormat.renderOutputs); +jest.mock("../../helper/format", () => { + const actual = jest.requireActual("../../helper/format"); + return { + ...actual, + renderOutputs: (...args: unknown[]) => mockRenderOutputs(...args), + }; +}); + +import { runOutput } from "../output"; + +const baseConfig = { + outDir: "out", + synthCommand: "noop", + onOutputsRetrieved: () => {}, +}; + +const outputsByConstructId = { + db: { host: { sensitive: false, type: "string", value: "db.example.com" } }, +}; + +beforeEach(() => { + mockRunCdktfProject.mockReset(); + mockStreamStop.mockReset(); + mockRenderOutputs.mockReset(); + mockRenderOutputs.mockImplementation(actualFormat.renderOutputs); + // Actually invoke the project callback so `project.fetchOutputs` is exercised: runOutput's + // returnValue is whatever that callback resolves to, and a mock that skips it would still pass + // if runOutput stopped fetching outputs altogether. + mockRunCdktfProject.mockImplementation(async (_opts, projectCallback) => ({ + returnValue: await projectCallback({ + fetchOutputs: jest.fn().mockResolvedValue(outputsByConstructId), + }), + project: {}, + })); +}); + +describe("runOutput --outputs-file write failures are fatal", () => { + it("rejects with a clean Usage error when the write fails with ENOENT (bad path)", async () => { + // Mirrors handlers.ts wiring: `onOutputsRetrieved` is `saveOutputs`, an async function. If this + // call is not awaited, a rejection here becomes a floating, unhandled promise rejection instead + // of something `runOutput` itself rejects with - this assertion (runOutput REJECTS) would fail + // against code that doesn't await the call, since that code resolves normally instead. + const err: NodeJS.ErrnoException = new Error( + "ENOENT: no such file or directory, open '/missing-dir/out.json'", + ); + err.code = "ENOENT"; + const onOutputsRetrieved = jest.fn().mockRejectedValue(err); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + let caught: any; + try { + await runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + expect(caught.__type).toBe("Usage"); + expect(caught.message).toContain("ENOENT: no such file or directory"); + // The fs error already names the path; the prefix must not repeat it. + expect(caught.message).not.toContain("to /missing-dir/out.json:"); + expect(mockStreamStop).toHaveBeenCalledTimes(1); + + logSpy.mockRestore(); + }); + + it("rejects with a clean External error when the write fails with EACCES (permission denied)", async () => { + const err: NodeJS.ErrnoException = new Error( + "EACCES: permission denied, open '/missing-dir/out.json'", + ); + err.code = "EACCES"; + const onOutputsRetrieved = jest.fn().mockRejectedValue(err); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + let caught: any; + try { + await runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + expect(caught.__type).toBe("External"); + expect(caught.message).toContain("EACCES: permission denied"); + + logSpy.mockRestore(); + }); + + it("does not print the 'written to' line when the write fails", async () => { + const onOutputsRetrieved = jest.fn().mockRejectedValue(new Error("boom")); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any), + ).rejects.toBeDefined(); + + const printed = logSpy.mock.calls.map((call) => call[0]).join("\n"); + expect(printed).not.toContain("The outputs have been written to"); + + logSpy.mockRestore(); + }); + + it("still prints the outputs table before rejecting on a write failure", async () => { + // The fetch succeeded and the outputs already exist in memory; only persistence failed. The + // table must reach the user before the rejection, not be swallowed by the failing write. + const onOutputsRetrieved = jest.fn().mockRejectedValue(new Error("boom")); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any), + ).rejects.toBeDefined(); + + const printed = stripAnsi( + logSpy.mock.calls.map((call) => call[0]).join("\n"), + ); + expect(printed).toContain("host = db.example.com"); + + logSpy.mockRestore(); + }); + + it("resolves and prints the 'written to' line when the write succeeds", async () => { + const onOutputsRetrieved = jest.fn().mockResolvedValue(undefined); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/tmp/out.json", + } as any), + ).resolves.toBeUndefined(); + + expect(onOutputsRetrieved).toHaveBeenCalledWith(outputsByConstructId); + const printed = logSpy.mock.calls.map((call) => call[0]).join("\n"); + expect(printed).toContain("The outputs have been written to /tmp/out.json"); + expect(mockStreamStop).toHaveBeenCalledTimes(1); + + logSpy.mockRestore(); + }); +}); + +describe("runOutput output rendering is non-fatal", () => { + it("does not fail the command when rendering the outputs throws", async () => { + mockRenderOutputs.mockImplementationOnce(() => { + throw new Error("render boom"); + }); + const onOutputsRetrieved = jest.fn(); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + const errSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + await expect( + runOutput({ ...baseConfig, onOutputsRetrieved } as any), + ).resolves.toBeUndefined(); + + expect(onOutputsRetrieved).toHaveBeenCalledWith(outputsByConstructId); + expect(errSpy).toHaveBeenCalledWith( + expect.stringContaining("Outputs fetched, but rendering them failed"), + ); + expect(mockStreamStop).toHaveBeenCalledTimes(1); + + logSpy.mockRestore(); + errSpy.mockRestore(); + }); +}); diff --git a/packages/cdktn-cli/src/bin/cmds/ui/deploy.ts b/packages/cdktn-cli/src/bin/cmds/ui/deploy.ts index 253eaf6bd..942862f99 100644 --- a/packages/cdktn-cli/src/bin/cmds/ui/deploy.ts +++ b/packages/cdktn-cli/src/bin/cmds/ui/deploy.ts @@ -1,5 +1,6 @@ // Copyright (c) HashiCorp, Inc // SPDX-License-Identifier: MPL-2.0 +import { Errors } from "@cdktn/commons"; import { NestedTerraformOutputs } from "@cdktn/cli-core"; import { runCdktfProject, Status } from "../helper/project-runner"; import { StreamRenderer } from "../helper/tty-stream"; @@ -16,7 +17,7 @@ export interface DeployConfig { targetStacks?: string[]; synthCommand: string; autoApprove: boolean; - onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void; + onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void | Promise; outputsPath?: string; ignoreMissingStackDependencies?: boolean; parallelism?: number; @@ -38,10 +39,14 @@ export interface DeployConfig { * `status.stop()` so cli-core halts cleanly rather than hanging. * * @param config - All deploy options, forwarded near-verbatim to `CdktfProject.deploy`. `onOutputsRetrieved` is - * invoked once the run completes (or is stopped) with the final outputs map. + * invoked once the run completes (or is stopped) with the final outputs map, after the outputs + * table has already been rendered; it may return a promise (e.g. writing --outputs-file to + * disk), which is awaited and, if it rejects, surfaced as a fatal error since a broken outputs + * write is a broken promise to the caller. * @returns Promise that resolves when the deploy completes, is stopped, or is dismissed. Rejects with whatever - * cli-core rejects with for real failures (terraform error, abort signal, etc.). A failure *rendering* - * the fetched outputs is non-fatal and does not cause a rejection. + * cli-core rejects with for real failures (terraform error, abort signal, etc.), or with a Usage error + * (bad --outputs-file path, e.g. ENOENT/ENOTDIR) or External error (any other `onOutputsRetrieved` + * failure). A failure only *rendering* the fetched outputs is non-fatal and does not cause a rejection. */ export async function runDeploy({ outDir, @@ -160,8 +165,9 @@ export async function runDeploy({ const outputs = project.outputsByConstructId; - onOutputsRetrieved(outputs); - + // Render the outputs table first (still non-fatal): the deploy already succeeded, so a + // failure here is purely cosmetic, and rendering before the --outputs-file write below means + // the user still sees their outputs even if that write fails. let rendered = ""; let renderFailed = false; try { @@ -180,19 +186,38 @@ export async function runDeploy({ if (rendered) { console.log(rendered); - } - - if (rendered || renderFailed) { - if (outputsPath) { - console.log(`The outputs have been written to ${outputsPath}`); - } - } else { + } else if (!renderFailed) { // Either there were no declared outputs at all, or every one of them was dropped upstream // (e.g. all missing from `terraform output`, the empty-group case renderOutputs collapses // to ""). Either way there is nothing to show the user, so say so plainly instead of // printing a stray blank line. console.log("No outputs found."); } + + // A failed --outputs-file write is a broken promise to the user and must be fatal: await it + // and rethrow as a clean error, which cdktn.ts's top-level `.fail()` handler prints as a + // single clean line rather than a stack trace, since the deploy itself already succeeded (and + // its outputs were already rendered above, so the write failure below does not hide them). + // ENOENT/ENOTDIR means the user pointed --outputs-file at a path that doesn't exist - a usage + // mistake, not something outside our control - so it is reported as a Usage error (excluded + // from Sentry crash reporting) rather than External. + try { + await onOutputsRetrieved(outputs); + } catch (e) { + const code = (e as NodeJS.ErrnoException)?.code; + const ErrorCtor = + code === "ENOENT" || code === "ENOTDIR" + ? Errors.Usage + : Errors.External; + throw ErrorCtor( + `Failed to write outputs: ${e instanceof Error ? e.message : e}`, + e instanceof Error ? e : undefined, + ); + } + + if (outputsPath) { + console.log(`The outputs have been written to ${outputsPath}`); + } } finally { stream.stop(); } diff --git a/packages/cdktn-cli/src/bin/cmds/ui/output.ts b/packages/cdktn-cli/src/bin/cmds/ui/output.ts index 57ae5a866..a60399771 100644 --- a/packages/cdktn-cli/src/bin/cmds/ui/output.ts +++ b/packages/cdktn-cli/src/bin/cmds/ui/output.ts @@ -1,5 +1,6 @@ // Copyright (c) HashiCorp, Inc // SPDX-License-Identifier: MPL-2.0 +import { Errors } from "@cdktn/commons"; import { NestedTerraformOutputs } from "@cdktn/cli-core"; import { runCdktfProject, Status } from "../helper/project-runner"; import { StreamRenderer } from "../helper/tty-stream"; @@ -10,7 +11,7 @@ export interface OutputConfig { outDir: string; targetStacks?: string[]; synthCommand: string; - onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void; + onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void | Promise; outputsPath?: string; skipSynth?: boolean; skipProviderLock?: boolean; @@ -39,8 +40,13 @@ function statusBar(status: Status): string { * Drive a `cdktn output` invocation. Fetches Terraform outputs (optionally skipping synth/provider-lock), prints them * in nested form, and writes them to disk when `outputsPath` is provided. * - * @param config - Output options. `onOutputsRetrieved` is called with the fetched outputs before they are printed. - * @returns Promise that resolves when outputs have been fetched and printed, rejects on failure. + * @param config - Output options. `onOutputsRetrieved` is called with the fetched outputs, after they have already + * been printed; it may return a promise (e.g. writing --outputs-file to disk), which is awaited + * and, if it rejects, surfaced as a fatal error (see the matching guard in `runDeploy`). + * @returns Promise that resolves when outputs have been fetched and printed, rejects on failure. Rejects with a + * Usage error (bad --outputs-file path, e.g. ENOENT/ENOTDIR) or External error (any other + * `onOutputsRetrieved` failure). A failure only *rendering* the fetched outputs is non-fatal and does + * not cause a rejection. */ export async function runOutput({ outDir, @@ -55,7 +61,7 @@ export async function runOutput({ stream.start(); try { - const { returnValue } = await runCdktfProject( + const { returnValue: outputs } = await runCdktfProject( { outDir, synthCommand, @@ -68,26 +74,62 @@ export async function runOutput({ }); }, }, - async (project) => { - const outputs = await project.fetchOutputs({ + (project) => + project.fetchOutputs({ stackNames: targetStacks, skipSynth, skipProviderLock, - }); - onOutputsRetrieved(outputs); - return outputs; - }, + }), ); stream.clearBar(); - if (returnValue && Object.keys(returnValue).length > 0) { - console.log(renderOutputs(returnValue)); - if (outputsPath) { - console.log(`The outputs have been written to ${outputsPath}`); - } - } else { + + // Render the outputs table first (still non-fatal): the fetch already succeeded, so a + // failure here is purely cosmetic, and rendering before the --outputs-file write below means + // the user still sees their outputs even if that write fails. + let rendered = ""; + let renderFailed = false; + try { + rendered = outputs ? renderOutputs(outputs) : ""; + } catch (e) { + // The fetch already succeeded at this point; a failure rendering the outputs table is + // purely cosmetic and must not fail the command. Log it so the user still learns something + // went wrong, but do not rethrow. + console.error( + `\nOutputs fetched, but rendering them failed: ${ + e instanceof Error ? e.message : e + }`, + ); + renderFailed = true; + } + + if (rendered) { + console.log(rendered); + } else if (!renderFailed) { console.log("No outputs found."); } + + // See runDeploy() in ./deploy.ts for the rationale: a failed --outputs-file write must be + // fatal, wrapped as a clean error so cdktn.ts's top-level `.fail()` handler prints a single + // clean line instead of a stack trace, since the outputs were already rendered above. A bad + // path (ENOENT/ENOTDIR) is a usage mistake and reported as Usage, not External. + try { + await onOutputsRetrieved(outputs); + } catch (e) { + const code = (e as NodeJS.ErrnoException)?.code; + const ErrorCtor = + code === "ENOENT" || code === "ENOTDIR" + ? Errors.Usage + : Errors.External; + throw ErrorCtor( + `Failed to write outputs: ${e instanceof Error ? e.message : e}`, + e instanceof Error ? e : undefined, + ); + } + + if (outputsPath) { + console.log(`The outputs have been written to ${outputsPath}`); + } } finally { stream.stop(); }