diff --git a/packages/cli/e2e/__tests__/trigger.spec.ts b/packages/cli/e2e/__tests__/trigger.spec.ts index 787b9b4a3..998478ff4 100644 --- a/packages/cli/e2e/__tests__/trigger.spec.ts +++ b/packages/cli/e2e/__tests__/trigger.spec.ts @@ -10,21 +10,21 @@ import { runCheckly } from '../run-checkly' describe('trigger', () => { let fixt: FixtureSandbox const executionId = uuid.v4() + // The fixture config derives its logicalId from EXECUTION_ID, and + // runCheckly runs with extendEnv: false — every invocation must pass the + // variable or the config is invalid, which aborts the command. + const runOptions = { env: { EXECUTION_ID: executionId } } beforeAll(async () => { fixt = await FixtureSandbox.create({ source: path.join(__dirname, 'fixtures', 'trigger-project'), }) - await runCheckly(fixt, ['deploy', '--force'], { - env: { EXECUTION_ID: executionId }, - }) + await runCheckly(fixt, ['deploy', '--force'], runOptions) }, 180_000) afterAll(async () => { try { - await runCheckly(fixt, ['destroy', '--force'], { - env: { EXECUTION_ID: executionId }, - }) + await runCheckly(fixt, ['destroy', '--force'], runOptions) } catch { // cleanup best-effort } @@ -42,7 +42,7 @@ describe('trigger', () => { `production,backend,${executionId}`, '--tags', `production,frontend,${executionId}`, - ]) + ], runOptions) expect(stdout).toContain(secretEnv) expect(stdout).toContain('Prod Backend Check') @@ -56,7 +56,7 @@ describe('trigger', () => { 'trigger', '--tags', 'no-checks-match-this-tag', - ]) + ], runOptions) expect.unreachable('Expected command to fail') } catch (err) { if (err instanceof ExecaError) { @@ -75,7 +75,7 @@ describe('trigger', () => { '--tags', 'no-checks-match-this-tag', '--fail-on-no-matching', - ]) + ], runOptions) expect.unreachable('Expected command to fail') } catch (err) { if (err instanceof ExecaError) { @@ -93,7 +93,7 @@ describe('trigger', () => { '--tags', 'no-checks-match-this-tag', '--no-fail-on-no-matching', - ]) + ], runOptions) expect(stdout).toContain('No matching checks were found.') }) diff --git a/packages/cli/src/commands/__tests__/base-command-diagnostics.spec.ts b/packages/cli/src/commands/__tests__/base-command-diagnostics.spec.ts new file mode 100644 index 000000000..b9e3898dd --- /dev/null +++ b/packages/cli/src/commands/__tests__/base-command-diagnostics.spec.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, vi } from 'vitest' + +import { BaseCommand } from '../baseCommand.js' +import { CommandStyle } from '../../helpers/command-style.js' +import { ConfigFileDiagnostics, InvalidConfigError } from '../../services/config-diagnostics.js' +import { Diagnostics, ErrorDiagnostic, NoticeDiagnostic, WarningDiagnostic } from '../../constructs/diagnostics.js' + +function createCommandContext () { + let exitCodeValue: number | undefined + return { + style: { + diagnostics: CommandStyle.prototype.diagnostics, + actionFailure: vi.fn(), + longError: vi.fn(), + longWarning: vi.fn(), + longInfo: vi.fn(), + shortError: vi.fn(), + }, + exit: vi.fn((code: number) => { + exitCodeValue = code + throw new Error(`EXIT_${code}`) + }), + catch: (BaseCommand.prototype as any).catch, + get exitCodeValue () { + return exitCodeValue + }, + } +} + +describe('CommandStyle', () => { + describe('diagnostics()', () => { + it('dispatches diagnostics by severity', () => { + const ctx = createCommandContext() + + const diagnostics = new Diagnostics() + diagnostics.add(new ErrorDiagnostic({ + title: 'error title', + message: 'error message', + error: new Error('error message'), + })) + diagnostics.add(new WarningDiagnostic({ + title: 'warning title', + message: 'warning message', + })) + diagnostics.add(new NoticeDiagnostic({ + title: 'notice title', + message: 'notice message', + })) + + ctx.style.diagnostics(diagnostics) + + expect(ctx.style.longError).toHaveBeenCalledWith('error title', 'error message') + expect(ctx.style.longWarning).toHaveBeenCalledWith('warning title', 'warning message') + expect(ctx.style.longInfo).toHaveBeenCalledWith('notice title', 'notice message') + }) + }) +}) + +describe('BaseCommand', () => { + describe('catch()', () => { + it('renders config diagnostics and exits 1 on InvalidConfigError', () => { + const ctx = createCommandContext() + + const diagnostics = new ConfigFileDiagnostics('checkly.config.ts') + diagnostics.add(new ErrorDiagnostic({ + title: 'Invalid property value', + message: 'The value is not valid.', + error: new Error('The value is not valid.'), + })) + + expect(() => ctx.catch.call(ctx, new InvalidConfigError(diagnostics))) + .toThrow('EXIT_1') + + expect(ctx.style.longError).toHaveBeenCalledWith( + '[checkly.config.ts] Invalid property value', + 'The value is not valid.', + ) + expect(ctx.style.shortError).toHaveBeenCalledWith('Your Checkly configuration file is not valid.') + expect(ctx.exitCodeValue).toBe(1) + }) + }) +}) diff --git a/packages/cli/src/commands/__tests__/confirm-flow-deploy.spec.ts b/packages/cli/src/commands/__tests__/confirm-flow-deploy.spec.ts index ce37f1c1f..9bc4179c2 100644 --- a/packages/cli/src/commands/__tests__/confirm-flow-deploy.spec.ts +++ b/packages/cli/src/commands/__tests__/confirm-flow-deploy.spec.ts @@ -11,17 +11,21 @@ vi.mock('../../rest/api', () => ({ validateAuthentication: vi.fn().mockResolvedValue({ name: 'Test Account' }), })) -vi.mock('../../services/checkly-config-loader', () => ({ - loadChecklyConfig: vi.fn().mockResolvedValue({ - config: { - logicalId: 'my-project', - projectName: 'My Project', - repoUrl: 'https://github.com/checkly/checkly-cli', - checks: {}, - }, - constructs: [], - }), -})) +vi.mock('../../services/checkly-config-loader', async () => { + const { Diagnostics } = await import('../../constructs/diagnostics.js') + return { + loadChecklyConfig: vi.fn().mockResolvedValue({ + config: { + logicalId: 'my-project', + projectName: 'My Project', + repoUrl: 'https://github.com/checkly/checkly-cli', + checks: {}, + }, + constructs: [], + diagnostics: new Diagnostics(), + }), + } +}) vi.mock('../../services/project-parser', () => ({ parseProject: vi.fn(), diff --git a/packages/cli/src/commands/__tests__/confirm-flow-destroy.spec.ts b/packages/cli/src/commands/__tests__/confirm-flow-destroy.spec.ts index bb41fce53..cca2eee17 100644 --- a/packages/cli/src/commands/__tests__/confirm-flow-destroy.spec.ts +++ b/packages/cli/src/commands/__tests__/confirm-flow-destroy.spec.ts @@ -10,11 +10,15 @@ vi.mock('../../rest/api', () => ({ validateAuthentication: vi.fn().mockResolvedValue({ name: 'Test Account' }), })) -vi.mock('../../services/checkly-config-loader', () => ({ - loadChecklyConfig: vi.fn().mockResolvedValue({ - config: { logicalId: 'my-project', projectName: 'My Project' }, - }), -})) +vi.mock('../../services/checkly-config-loader', async () => { + const { Diagnostics } = await import('../../constructs/diagnostics.js') + return { + loadChecklyConfig: vi.fn().mockResolvedValue({ + config: { logicalId: 'my-project', projectName: 'My Project' }, + diagnostics: new Diagnostics(), + }), + } +}) vi.mock('../../services/util', () => ({ splitConfigFilePath: vi.fn().mockReturnValue({ @@ -31,6 +35,8 @@ import { detectCliMode } from '../../helpers/cli-mode.js' import prompts from 'prompts' import * as api from '../../rest/api.js' import { buildConfirmCommand } from '../../helpers/command-preview.js' +import { loadChecklyConfig } from '../../services/checkly-config-loader.js' +import { Diagnostics, WarningDiagnostic } from '../../constructs/diagnostics.js' import { AuthCommand } from '../authCommand.js' import Destroy from '../destroy.js' @@ -60,6 +66,7 @@ function createCommandContext (parsed: { flags: Record, metadat confirmOrAbort: AuthCommand.prototype.confirmOrAbort, style: { outputFormat: undefined, + diagnostics: vi.fn(), longError: vi.fn(), actionStart: vi.fn(), actionStatus: vi.fn(), @@ -98,6 +105,29 @@ describe('destroy confirmation flow', () => { expect(api.projects.deleteProject).not.toHaveBeenCalled() }) + it('renders non-fatal config diagnostics right after loading the config', async () => { + vi.mocked(detectCliMode).mockReturnValue('agent') + const configDiagnostics = new Diagnostics() + configDiagnostics.add(new WarningDiagnostic({ + title: 'Config warning', + message: 'A config-level warning.', + })) + vi.mocked(loadChecklyConfig).mockResolvedValueOnce({ + config: { logicalId: 'my-project', projectName: 'My Project' }, + diagnostics: configDiagnostics, + } as any) + const ctx = createCommandContext({ + flags: { force: false }, + }) + + // The confirmation gate exits 2, but the diagnostics render before it. + await expect( + Destroy.prototype.run.call(ctx as any), + ).rejects.toThrow('EXIT_2') + + expect(ctx.style.diagnostics).toHaveBeenCalledWith(configDiagnostics) + }) + it('executes with --force in agent mode', async () => { vi.mocked(detectCliMode).mockReturnValue('agent') const ctx = createCommandContext({ diff --git a/packages/cli/src/commands/__tests__/deploy-config-diagnostics.spec.ts b/packages/cli/src/commands/__tests__/deploy-config-diagnostics.spec.ts new file mode 100644 index 000000000..cc3063065 --- /dev/null +++ b/packages/cli/src/commands/__tests__/deploy-config-diagnostics.spec.ts @@ -0,0 +1,126 @@ +import { describe, expect, it, vi } from 'vitest' + +vi.mock('../../helpers/cli-mode', () => ({ + detectCliMode: vi.fn(() => 'agent'), +})) + +vi.mock('../../rest/api', () => ({ + runtimes: { getAll: vi.fn().mockResolvedValue([]) }, + validateAuthentication: vi.fn().mockResolvedValue({ name: 'Test Account' }), +})) + +vi.mock('../../services/checkly-config-loader', () => ({ + loadChecklyConfig: vi.fn(), +})) + +vi.mock('../../services/project-parser', () => ({ + parseProject: vi.fn(), +})) + +vi.mock('../../services/util', () => ({ + splitConfigFilePath: vi.fn().mockReturnValue({ + configDirectory: '.', + configFilenames: ['checkly.config.ts'], + }), + getGitInformation: vi.fn(), +})) + +import { loadChecklyConfig } from '../../services/checkly-config-loader.js' +import { parseProject } from '../../services/project-parser.js' +import { ConfigFileDiagnostics } from '../../services/config-diagnostics.js' +import { Diagnostics, ErrorDiagnostic, WarningDiagnostic } from '../../constructs/diagnostics.js' +import { CommandStyle } from '../../helpers/command-style.js' +import { AuthCommand } from '../authCommand.js' +import Deploy from '../deploy.js' + +function createCommandContext (parsed: unknown) { + let exitCodeValue: number | undefined + return { + parse: vi.fn().mockResolvedValue(parsed), + exit: vi.fn((code: number) => { + exitCodeValue = code + throw new Error(`EXIT_${code}`) + }), + style: { + outputFormat: undefined, + diagnostics: CommandStyle.prototype.diagnostics, + actionStart: vi.fn(), + actionSuccess: vi.fn(), + actionFailure: vi.fn(), + longError: vi.fn(), + longWarning: vi.fn(), + longInfo: vi.fn(), + shortError: vi.fn(), + }, + validateProject: (AuthCommand.prototype as any).validateProject, + constructor: Deploy, + account: { name: 'Test Account', runtimeId: 'runtime-default' }, + get exitCodeValue () { + return exitCodeValue + }, + } +} + +const deployFlags = { + flags: { + 'force': true, + 'preview': true, + 'output': false, + 'verbose': false, + 'config': undefined, + 'schedule-on-deploy': true, + 'verify-runtime-dependencies': true, + 'debug-bundle': false, + 'debug-bundle-output-file': './debug-bundle.json', + }, + metadata: { flags: {} }, +} + +describe('deploy config diagnostics', () => { + it('renders config diagnostics ahead of project diagnostics', async () => { + const configDiagnostics = new ConfigFileDiagnostics('checkly.config.ts') + configDiagnostics.add(new WarningDiagnostic({ + title: 'Config warning', + message: 'A config-level warning.', + })) + vi.mocked(loadChecklyConfig).mockResolvedValue({ + config: { + logicalId: 'my-project', + projectName: 'My Project', + checks: {}, + }, + constructs: [], + diagnostics: configDiagnostics, + } as any) + + const project = { + repoUrl: undefined, + validate: vi.fn((diagnostics: Diagnostics) => { + diagnostics.add(new ErrorDiagnostic({ + title: 'Project error', + message: 'A project-level error.', + error: new Error('A project-level error.'), + })) + return Promise.resolve() + }), + } + vi.mocked(parseProject).mockResolvedValue(project as any) + + const ctx = createCommandContext(deployFlags) + + await expect(Deploy.prototype.run.call(ctx as any)).rejects.toThrow('EXIT_1') + + expect(ctx.style.longWarning).toHaveBeenCalledWith( + '[checkly.config.ts] Config warning', + 'A config-level warning.', + ) + expect(ctx.style.longError).toHaveBeenCalledWith('Project error', 'A project-level error.') + + // Config diagnostics must render before project diagnostics. + const warningOrder = ctx.style.longWarning.mock.invocationCallOrder[0] + const errorOrder = ctx.style.longError.mock.invocationCallOrder[0] + expect(warningOrder).toBeLessThan(errorOrder) + + expect(ctx.exitCodeValue).toBe(1) + }) +}) diff --git a/packages/cli/src/commands/__tests__/trigger-config-errors.spec.ts b/packages/cli/src/commands/__tests__/trigger-config-errors.spec.ts new file mode 100644 index 000000000..e2ece3888 --- /dev/null +++ b/packages/cli/src/commands/__tests__/trigger-config-errors.spec.ts @@ -0,0 +1,86 @@ +import { describe, expect, it, vi } from 'vitest' + +vi.mock('../../services/checkly-config-loader', async importOriginal => { + // Keep the real ConfigNotFoundError so instanceof checks work. + const actual = await importOriginal() + return { + ...actual, + loadChecklyConfig: vi.fn(), + } +}) + +vi.mock('../../services/util', () => ({ + splitConfigFilePath: vi.fn().mockReturnValue({ + configDirectory: '.', + configFilenames: ['checkly.config.ts'], + }), + getEnvs: vi.fn().mockResolvedValue({}), + getGitInformation: vi.fn(), + getCiInformation: vi.fn(), +})) + +import { loadChecklyConfig, ConfigNotFoundError } from '../../services/checkly-config-loader.js' +import { ConfigFileDiagnostics, InvalidConfigError } from '../../services/config-diagnostics.js' +import { ErrorDiagnostic } from '../../constructs/diagnostics.js' +import Trigger from '../trigger.js' + +// The harness stops the run right after the config-load step, which is the +// behavior under test: which load failures are tolerated and which abort. +class StopError extends Error {} + +function createCommandContext () { + return { + parse: vi.fn().mockResolvedValue({ + flags: { + 'config': undefined, + 'env': [], + 'env-file': undefined, + 'reporter': undefined, + 'retries': undefined, + 'detach': false, + }, + }), + style: { + diagnostics: vi.fn(), + }, + prepareRunLocation: vi.fn(() => { + throw new StopError('reached run preparation') + }), + constructor: Trigger, + } +} + +describe('trigger config-load failures', () => { + it('tolerates a missing config file', async () => { + vi.mocked(loadChecklyConfig).mockRejectedValue( + new ConfigNotFoundError(['.'], ['checkly.config.ts']), + ) + const ctx = createCommandContext() + + // Reaching run preparation means the missing config was swallowed. + await expect(Trigger.prototype.run.call(ctx as any)).rejects.toThrow(StopError) + }) + + it('tolerates a config that fails to load', async () => { + vi.mocked(loadChecklyConfig).mockRejectedValue( + new Error(`Error loading file 'checkly.config.ts'`), + ) + const ctx = createCommandContext() + + await expect(Trigger.prototype.run.call(ctx as any)).rejects.toThrow(StopError) + }) + + it('fails on an invalid config', async () => { + const diagnostics = new ConfigFileDiagnostics('checkly.config.ts') + diagnostics.add(new ErrorDiagnostic({ + title: 'Missing required property', + message: 'Property "logicalId" is required and must be set.', + error: new Error('Property "logicalId" is required and must be set.'), + })) + vi.mocked(loadChecklyConfig).mockRejectedValue(new InvalidConfigError(diagnostics)) + const ctx = createCommandContext() + + await expect(Trigger.prototype.run.call(ctx as any)).rejects.toBeInstanceOf(InvalidConfigError) + expect(ctx.prepareRunLocation).not.toHaveBeenCalled() + }) +}) diff --git a/packages/cli/src/commands/authCommand.ts b/packages/cli/src/commands/authCommand.ts index d93a0acaf..d7b276b3b 100644 --- a/packages/cli/src/commands/authCommand.ts +++ b/packages/cli/src/commands/authCommand.ts @@ -3,7 +3,9 @@ import { BaseCommand } from './baseCommand.js' import * as api from '../rest/api.js' import { Account } from '../rest/accounts.js' import { Session } from '../constructs/session.js' +import { Diagnostics } from '../constructs/diagnostics.js' import { detectCliMode } from '../helpers/cli-mode.js' +import type { Project } from '../constructs/project.js' import type { CommandPreview } from '../helpers/command-preview.js' import { formatPreviewForAgent, formatPreviewForTerminal } from '../helpers/command-preview.js' @@ -28,6 +30,37 @@ export abstract class AuthCommand extends BaseCommand { Session.accountFeatures = this.#account?.features ?? [] } + protected async validateProject ( + project: Project, + options: { + configDiagnostics: Diagnostics + failureMessage?: string + }, + ): Promise { + const { + configDiagnostics, + failureMessage = `Unable to continue due to unresolved validation errors.`, + } = options + + this.style.actionStart('Validating project resources') + + const diagnostics = new Diagnostics() + // Config diagnostics come first so that they render before any + // project-level diagnostics. + diagnostics.extend(configDiagnostics) + await project.validate(diagnostics) + + this.style.diagnostics(diagnostics) + + if (diagnostics.isFatal()) { + this.style.actionFailure() + this.style.shortError(failureMessage) + this.exit(1) + } + + this.style.actionSuccess() + } + protected async confirmOrAbort ( preview: CommandPreview, options: { force: boolean, dryRun?: boolean, interactiveConfirm?: () => Promise }, diff --git a/packages/cli/src/commands/baseCommand.ts b/packages/cli/src/commands/baseCommand.ts index 6919ce6ea..6c3ba716e 100644 --- a/packages/cli/src/commands/baseCommand.ts +++ b/packages/cli/src/commands/baseCommand.ts @@ -6,6 +6,7 @@ import { dirname, relative } from 'node:path' import { api } from '../rest/api.js' import { assignProxy } from '../services/proxy.js' import { CommandStyle } from '../helpers/command-style.js' +import { InvalidConfigError } from '../services/config-diagnostics.js' import { findStaleSkills } from '../services/skills.js' import { PackageJsonFile } from '../services/check-parser/package-files/package-json-file.js' import { detectNearestPackageJson } from '../services/check-parser/package-files/package-manager.js' @@ -152,6 +153,14 @@ export abstract class BaseCommand extends Command { } protected catch (err: Error & { exitCode?: number }): Promise { + if (err instanceof InvalidConfigError) { + // Stops the spinner if one is running (e.g. `validate` starts one + // before loading the config); also emits a blank separator line. + this.style.actionFailure() + this.style.diagnostics(err.diagnostics) + this.style.shortError(`Your Checkly configuration file is not valid.`) + return this.exit(1) + } // TODO: we can add Sentry here and log critical errors. return super.catch(err) } diff --git a/packages/cli/src/commands/debug/__tests__/parse-project-config.spec.ts b/packages/cli/src/commands/debug/__tests__/parse-project-config.spec.ts new file mode 100644 index 000000000..8ab14e362 --- /dev/null +++ b/packages/cli/src/commands/debug/__tests__/parse-project-config.spec.ts @@ -0,0 +1,69 @@ +import path from 'node:path' + +import { describe, it, expect, beforeAll, afterAll } from 'vitest' + +import { FixtureSandbox } from '../../../testing/fixture-sandbox.js' +import { ParseProjectOutput } from '../parse-project.js' + +const DEFAULT_FIXT_TIMEOUT = 180_000 + +describe('debug parse-project config handling', () => { + describe('invalid config', () => { + let fixt: FixtureSandbox + + beforeAll(async () => { + fixt = await FixtureSandbox.create({ + source: path.join(__dirname, 'parse-project-fixtures', 'invalid-config'), + }) + }, DEFAULT_FIXT_TIMEOUT) + + afterAll(async () => { + await fixt?.destroy() + }) + + it('emits the machine-readable diagnostics shape with a null payload', async () => { + const result = await fixt.run('pnpm', ['checkly', 'debug', 'parse-project']) + + expect(result.exitCode).toBe(0) + + const output: ParseProjectOutput = JSON.parse(result.stdout) + expect(output.payload).toBeNull() + expect(output.diagnostics.fatal).toBe(true) + expect(output.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringContaining('Missing required property'), + fatal: true, + }), + expect.objectContaining({ + title: expect.stringContaining('Invalid property value'), + fatal: true, + }), + ])) + }) + }) + + describe('missing config', () => { + let fixt: FixtureSandbox + + beforeAll(async () => { + fixt = await FixtureSandbox.create({ + source: path.join(__dirname, 'parse-project-fixtures', 'missing-config'), + }) + }, DEFAULT_FIXT_TIMEOUT) + + afterAll(async () => { + await fixt?.destroy() + }) + + it('fails with a non-zero exit code', async () => { + // fixt.run() rejects on a non-zero exit; the execa error carries the + // exit code and output. An unexpected success resolves with exit code + // 0 and fails the assertion below. + const result = await fixt.run('pnpm', ['checkly', 'debug', 'parse-project']) + .catch((err: any) => err) + + expect(result.exitCode).not.toBe(0) + expect(result.stderr).toContain('Unable to detect a Checkly configuration file') + }) + }) +}) diff --git a/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/checkly.config.js b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/checkly.config.js new file mode 100644 index 000000000..f41028933 --- /dev/null +++ b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/checkly.config.js @@ -0,0 +1,9 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. +const config = { + projectName: 'invalid-config', + // logicalId is missing + bundle: 5, +} + +export default config diff --git a/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/package.json b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/package.json new file mode 100644 index 000000000..5b7f4c96a --- /dev/null +++ b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/package.json @@ -0,0 +1,6 @@ +{ + "name": "invalid-config", + "type": "module", + "version": "1.0.0", + "private": true +} diff --git a/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/missing-config/package.json b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/missing-config/package.json new file mode 100644 index 000000000..30dbef4dd --- /dev/null +++ b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/missing-config/package.json @@ -0,0 +1,6 @@ +{ + "name": "missing-config", + "type": "module", + "version": "1.0.0", + "private": true +} diff --git a/packages/cli/src/commands/debug/parse-project.ts b/packages/cli/src/commands/debug/parse-project.ts index 0d17446c0..94e6ad183 100644 --- a/packages/cli/src/commands/debug/parse-project.ts +++ b/packages/cli/src/commands/debug/parse-project.ts @@ -2,6 +2,7 @@ import { Command, Flags } from '@oclif/core' import { parseProject } from '../../services/project-parser.js' import { loadChecklyConfig } from '../../services/checkly-config-loader.js' +import { InvalidConfigError } from '../../services/config-diagnostics.js' import { Diagnostics, Session, @@ -37,6 +38,21 @@ export type ParseProjectOutput = { member: boolean payload: unknown }[] + } | null +} + +function serializeDiagnostics (diagnostics: Diagnostics): ParseProjectOutput['diagnostics'] { + return { + fatal: diagnostics.isFatal(), + benign: diagnostics.isBenign(), + observations: diagnostics.observations.map(diag => { + return { + title: diag.title, + message: diag.message, + fatal: diag.isFatal(), + benign: diag.isBenign(), + } + }), } } @@ -111,10 +127,33 @@ export default class ParseProjectCommand extends Command { : undefined sampler?.unref() const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) + + let loaded + try { + loaded = await loadChecklyConfig(configDirectory, configFilenames) + } catch (err) { + if (err instanceof InvalidConfigError) { + // Keep the machine-readable diagnostics contract for a fatally + // invalid config instead of degrading to a generic error. + const output: ParseProjectOutput = { + diagnostics: serializeDiagnostics(err.diagnostics), + payload: null, + } + + // eslint-disable-next-line no-console + console.log(JSON.stringify(output, null, 2)) + return + } + + // Any other load failure (e.g. a missing config file) escapes to + // oclif's default handler, preserving its non-zero exit code. + throw err + } const { config: checklyConfig, constructs: checklyConfigConstructs, - } = await loadChecklyConfig(configDirectory, configFilenames) + diagnostics: configDiagnostics, + } = loaded const availableRuntimes = await loadSnapshot() try { @@ -157,6 +196,8 @@ export default class ParseProjectCommand extends Command { const parseMs = performance.now() - parseStartedAt const diagnostics = new Diagnostics() + // Config diagnostics come first so that they lead the output. + diagnostics.extend(configDiagnostics) await project.validate(diagnostics) let bundleMs = 0 @@ -188,18 +229,7 @@ export default class ParseProjectCommand extends Command { })() const output = { - diagnostics: { - fatal: diagnostics.isFatal(), - benign: diagnostics.isBenign(), - observations: diagnostics.observations.map(diag => { - return { - title: diag.title, - message: diag.message, - fatal: diag.isFatal(), - benign: diag.isBenign(), - } - }), - }, + diagnostics: serializeDiagnostics(diagnostics), payload, } diff --git a/packages/cli/src/commands/deploy.ts b/packages/cli/src/commands/deploy.ts index f1aa16ff8..20a298978 100644 --- a/packages/cli/src/commands/deploy.ts +++ b/packages/cli/src/commands/deploy.ts @@ -8,7 +8,7 @@ import { loadChecklyConfig } from '../services/checkly-config-loader.js' import { Check, AlertChannelSubscription, AlertChannel, CheckGroup, Dashboard, MaintenanceWindow, PrivateLocation, PrivateLocationCheckAssignment, PrivateLocationGroupAssignment, - Project, ProjectData, Diagnostics, + Project, ProjectData, Session, StatusPage, StatusPageService, } from '../constructs/index.js' import chalk from 'chalk' @@ -129,6 +129,7 @@ export default class Deploy extends AuthCommand { const { config: checklyConfig, constructs: checklyConfigConstructs, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) const account = this.account @@ -185,28 +186,7 @@ export default class Deploy extends AuthCommand { this.style.actionSuccess() - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Unable to continue due to unresolved validation errors.`) - this.exit(1) - } - - this.style.actionSuccess() + await this.validateProject(project, { configDiagnostics }) const bundler = await Bundler.createForWorkspace(Session.workspace.unwrap(), { dependencyCacheVersion: checklyConfig.caching?.dependencyCache?.version, diff --git a/packages/cli/src/commands/destroy.ts b/packages/cli/src/commands/destroy.ts index 4ec85eaaf..48e36b755 100644 --- a/packages/cli/src/commands/destroy.ts +++ b/packages/cli/src/commands/destroy.ts @@ -38,7 +38,11 @@ export default class Destroy extends AuthCommand { 'cancel-in-progress-deployment': cancelInProgress, } = flags const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) - const { config: checklyConfig } = await loadChecklyConfig(configDirectory, configFilenames) + const { + config: checklyConfig, + diagnostics: configDiagnostics, + } = await loadChecklyConfig(configDirectory, configFilenames) + this.style.diagnostics(configDiagnostics) const account = this.account await this.confirmOrAbort({ diff --git a/packages/cli/src/commands/import/__tests__/apply.spec.ts b/packages/cli/src/commands/import/__tests__/apply.spec.ts index fa34dcd2c..8310186c1 100644 --- a/packages/cli/src/commands/import/__tests__/apply.spec.ts +++ b/packages/cli/src/commands/import/__tests__/apply.spec.ts @@ -19,6 +19,7 @@ vi.mock('../../../services/checkly-config-loader', () => ({ import { detectCliMode } from '../../../helpers/cli-mode.js' import * as api from '../../../rest/api.js' import { loadChecklyConfig } from '../../../services/checkly-config-loader.js' +import { Diagnostics } from '../../../constructs/diagnostics.js' import { ImportPlan } from '../../../rest/projects.js' import ImportApplyCommand from '../apply.js' @@ -39,6 +40,7 @@ function createCommandContext (parsed: unknown) { throw new Error(`EXIT_${code}`) }), style: { + diagnostics: vi.fn(), actionStart: vi.fn(), actionSuccess: vi.fn(), actionFailure: vi.fn(), @@ -57,6 +59,7 @@ describe('import apply command (non-interactive)', () => { vi.clearAllMocks() vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.applyImportPlan).mockResolvedValue({} as any) vi.mocked(api.projects.commitImportPlan).mockResolvedValue({} as any) diff --git a/packages/cli/src/commands/import/__tests__/cancel.spec.ts b/packages/cli/src/commands/import/__tests__/cancel.spec.ts index f5db9479b..2b0846c0d 100644 --- a/packages/cli/src/commands/import/__tests__/cancel.spec.ts +++ b/packages/cli/src/commands/import/__tests__/cancel.spec.ts @@ -18,6 +18,7 @@ vi.mock('../../../services/checkly-config-loader', () => ({ import { detectCliMode } from '../../../helpers/cli-mode.js' import * as api from '../../../rest/api.js' import { loadChecklyConfig } from '../../../services/checkly-config-loader.js' +import { Diagnostics } from '../../../constructs/diagnostics.js' import { ImportPlan } from '../../../rest/projects.js' import ImportCancelCommand from '../cancel.js' @@ -54,6 +55,7 @@ function createCommandContext (parsed: unknown) { }), confirmOrAbort: vi.fn(), style: { + diagnostics: vi.fn(), actionStart: vi.fn(), actionSuccess: vi.fn(), actionFailure: vi.fn(), @@ -73,6 +75,7 @@ describe('import cancel command (non-interactive)', () => { vi.clearAllMocks() vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.cancelImportPlan).mockResolvedValue({} as any) }) @@ -147,6 +150,7 @@ describe('import cancel command (flag validation)', () => { vi.clearAllMocks() vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.cancelImportPlan).mockResolvedValue({} as any) }) @@ -180,6 +184,7 @@ describe('import cancel command (empty candidate list)', () => { vi.mocked(detectCliMode).mockReturnValue('agent') vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.findImportPlans).mockResolvedValue({ data: [] } as any) vi.mocked(api.projects.cancelImportPlan).mockResolvedValue({} as any) @@ -221,6 +226,7 @@ describe('import cancel command (confirmation gate)', () => { vi.mocked(detectCliMode).mockReturnValue('agent') vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.cancelImportPlan).mockResolvedValue({} as any) }) diff --git a/packages/cli/src/commands/import/__tests__/commit.spec.ts b/packages/cli/src/commands/import/__tests__/commit.spec.ts index 18d27a793..fe13aebfc 100644 --- a/packages/cli/src/commands/import/__tests__/commit.spec.ts +++ b/packages/cli/src/commands/import/__tests__/commit.spec.ts @@ -18,6 +18,7 @@ vi.mock('../../../services/checkly-config-loader', () => ({ import { detectCliMode } from '../../../helpers/cli-mode.js' import * as api from '../../../rest/api.js' import { loadChecklyConfig } from '../../../services/checkly-config-loader.js' +import { Diagnostics } from '../../../constructs/diagnostics.js' import { ImportPlan } from '../../../rest/projects.js' import ImportCommitCommand from '../commit.js' @@ -52,6 +53,7 @@ function createCommandContext (parsed: unknown) { }), confirmOrAbort: vi.fn(), style: { + diagnostics: vi.fn(), actionStart: vi.fn(), actionSuccess: vi.fn(), actionFailure: vi.fn(), @@ -68,6 +70,7 @@ describe('import commit command (non-interactive)', () => { vi.mocked(detectCliMode).mockReturnValue('agent') vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.commitImportPlan).mockResolvedValue({} as any) }) @@ -132,6 +135,7 @@ describe('import commit command (confirmation gate)', () => { vi.mocked(detectCliMode).mockReturnValue('agent') vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.commitImportPlan).mockResolvedValue({} as any) }) diff --git a/packages/cli/src/commands/import/apply.ts b/packages/cli/src/commands/import/apply.ts index 9919633e7..586349b0c 100644 --- a/packages/cli/src/commands/import/apply.ts +++ b/packages/cli/src/commands/import/apply.ts @@ -42,7 +42,9 @@ export default class ImportApplyCommand extends AuthCommand { const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) const { config: checklyConfig, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) + this.style.diagnostics(configDiagnostics) const { logicalId, diff --git a/packages/cli/src/commands/import/cancel.ts b/packages/cli/src/commands/import/cancel.ts index 0c3a206bc..8832c0aab 100644 --- a/packages/cli/src/commands/import/cancel.ts +++ b/packages/cli/src/commands/import/cancel.ts @@ -49,7 +49,9 @@ export default class ImportCancelCommand extends AuthCommand { const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) const { config: checklyConfig, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) + this.style.diagnostics(configDiagnostics) const { logicalId, diff --git a/packages/cli/src/commands/import/commit.ts b/packages/cli/src/commands/import/commit.ts index 1e2aa4962..7c59c1bc6 100644 --- a/packages/cli/src/commands/import/commit.ts +++ b/packages/cli/src/commands/import/commit.ts @@ -41,7 +41,9 @@ export default class ImportCommitCommand extends AuthCommand { const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) const { config: checklyConfig, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) + this.style.diagnostics(configDiagnostics) const { logicalId, diff --git a/packages/cli/src/commands/import/plan.ts b/packages/cli/src/commands/import/plan.ts index 0a338567e..d9330ab8c 100644 --- a/packages/cli/src/commands/import/plan.ts +++ b/packages/cli/src/commands/import/plan.ts @@ -141,6 +141,13 @@ future deployments include the imported resources.` 'import', ] + /** + * Non-fatal diagnostics from loading the Checkly config file, rendered + * ahead of project-level diagnostics. Empty when no config file exists + * (e.g. the config was created interactively). + */ + readonly #configDiagnostics = new Diagnostics() + async run (): Promise { const { flags, argv } = await this.parse(ImportPlanCommand) const { @@ -897,8 +904,11 @@ ${chalk.cyan('For safety, resources are not deletable until the plan has been co try { const { config: checklyConfig, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) + this.#configDiagnostics.extend(configDiagnostics) + return checklyConfig } catch (err) { if (err instanceof ConfigNotFoundError) { @@ -909,31 +919,6 @@ ${chalk.cyan('For safety, resources are not deletable until the plan has been co } } - async #validateProject (project: Project): Promise { - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Unable to continue due to unresolved validation errors.`) - this.exit(1) - } - - this.style.actionSuccess() - } - async #findExportedResources ( configDirectory: string, checklyConfig: ChecklyConfig, @@ -972,7 +957,9 @@ ${chalk.cyan('For safety, resources are not deletable until the plan has been co throw err } - await this.#validateProject(project) + await this.validateProject(project, { + configDiagnostics: this.#configDiagnostics, + }) this.style.actionStart('Searching for exported resources') diff --git a/packages/cli/src/commands/pw-test.ts b/packages/cli/src/commands/pw-test.ts index 897064da9..3fa559e69 100644 --- a/packages/cli/src/commands/pw-test.ts +++ b/packages/cli/src/commands/pw-test.ts @@ -12,7 +12,7 @@ import { prepareReportersTypes, prepareRunLocation, splitChecklyAndPlaywrightFla import * as api from '../rest/api.js' import config from '../services/config.js' import { parseProject } from '../services/project-parser.js' -import { Diagnostics, PlaywrightCheck, RuntimeCheck, Session } from '../constructs/index.js' +import { PlaywrightCheck, RuntimeCheck, Session } from '../constructs/index.js' import { Flags } from '@oclif/core' import { createReporters, ReporterType } from '../reporters/reporter.js' import TestRunner from '../services/test-runner.js' @@ -156,6 +156,7 @@ export default class PwTestCommand extends AuthCommand { const { config: checklyConfig, constructs: checklyConfigConstructs, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames, false, pwPathFlag) let playwrightConfigPath = pwPathFlag ?? checklyConfig.checks?.playwrightConfigPath @@ -242,28 +243,7 @@ export default class PwTestCommand extends AuthCommand { this.style.actionSuccess() - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Unable to continue due to unresolved validation errors.`) - this.exit(1) - } - - this.style.actionSuccess() + await this.validateProject(project, { configDiagnostics }) const bundler = await Bundler.createForWorkspace(Session.workspace.unwrap(), { dependencyCacheVersion: checklyConfig.caching?.dependencyCache?.version, diff --git a/packages/cli/src/commands/test.ts b/packages/cli/src/commands/test.ts index c1efdec98..52f010785 100644 --- a/packages/cli/src/commands/test.ts +++ b/packages/cli/src/commands/test.ts @@ -12,7 +12,7 @@ import TestRunner from '../services/test-runner.js' import { loadChecklyConfig } from '../services/checkly-config-loader.js' import { filterByFileNamePattern, filterByCheckNamePattern, filterByTags } from '../services/test-filters.js' import { AuthCommand } from './authCommand.js' -import { BrowserCheck, Check, Diagnostics, HeartbeatMonitor, MultiStepCheck, Project, RetryStrategyBuilder, RuntimeCheck, Session } from '../constructs/index.js' +import { BrowserCheck, Check, HeartbeatMonitor, MultiStepCheck, Project, RetryStrategyBuilder, RuntimeCheck, Session } from '../constructs/index.js' import type { Region } from '../index.js' import { splitConfigFilePath, getGitInformation, getCiInformation, getEnvs } from '../services/util.js' import { createReporters, ReporterType } from '../reporters/reporter.js' @@ -169,6 +169,7 @@ export default class Test extends AuthCommand { const { config: checklyConfig, constructs: checklyConfigConstructs, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) const location = await prepareRunLocation(checklyConfig.cli, { @@ -267,28 +268,7 @@ export default class Test extends AuthCommand { this.style.actionSuccess() - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Unable to continue due to unresolved validation errors.`) - this.exit(1) - } - - this.style.actionSuccess() + await this.validateProject(project, { configDiagnostics }) const bundler = await Bundler.createForWorkspace(Session.workspace.unwrap(), { dependencyCacheVersion: checklyConfig.caching?.dependencyCache?.version, diff --git a/packages/cli/src/commands/trigger.ts b/packages/cli/src/commands/trigger.ts index 9632107d3..c3c00dac5 100644 --- a/packages/cli/src/commands/trigger.ts +++ b/packages/cli/src/commands/trigger.ts @@ -4,6 +4,7 @@ import { isCI } from 'ci-info' import * as api from '../rest/api.js' import { AuthCommand } from './authCommand.js' import { loadChecklyConfig } from '../services/checkly-config-loader.js' +import { InvalidConfigError } from '../services/config-diagnostics.js' import { splitConfigFilePath, getEnvs, getGitInformation, getCiInformation } from '../services/util.js' import type { Region } from '../index.js' import TriggerRunner from '../services/trigger-runner.js' @@ -140,10 +141,16 @@ export default class Trigger extends AuthCommand { let checklyConfig try { - const { config } = await loadChecklyConfig(configDirectory, configFilenames) + const { config, diagnostics: configDiagnostics } = await loadChecklyConfig(configDirectory, configFilenames) checklyConfig = config - } catch { - // Don't throw an error if the config file is missing + this.style.diagnostics(configDiagnostics) + } catch (err) { + // Trigger works without a project, so config-load failures are + // tolerated - except an invalid config, which should be fixed rather + // than silently ignored. + if (err instanceof InvalidConfigError) { + throw err + } } const location = await this.prepareRunLocation(checklyConfig?.cli, { runLocation: runLocation as keyof Region, diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index b06c1ef4d..1a561fcbc 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -3,7 +3,6 @@ import { Flags } from '@oclif/core' import { AuthCommand } from './authCommand.js' import { parseProject } from '../services/project-parser.js' import { loadChecklyConfig } from '../services/checkly-config-loader.js' -import { Diagnostics } from '../constructs/index.js' import { splitConfigFilePath } from '../services/util.js' import commonMessages from '../messages/common-messages.js' import { Runtime } from '../runtimes/index.js' @@ -39,6 +38,7 @@ export default class Validate extends AuthCommand { const { config: checklyConfig, constructs: checklyConfigConstructs, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) const account = this.account const availableRuntimes = await api.runtimes.getAll() @@ -68,28 +68,10 @@ export default class Validate extends AuthCommand { this.style.actionSuccess() - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Your project is not valid.`) - this.exit(1) - } - - this.style.actionSuccess() + await this.validateProject(project, { + configDiagnostics, + failureMessage: `Your project is not valid.`, + }) this.style.shortSuccess(`Your project is valid.`) } diff --git a/packages/cli/src/helpers/command-style.ts b/packages/cli/src/helpers/command-style.ts index 3e25c6b1c..d1462aaa7 100644 --- a/packages/cli/src/helpers/command-style.ts +++ b/packages/cli/src/helpers/command-style.ts @@ -2,6 +2,7 @@ import chalk from 'chalk' import { ux } from '@oclif/core' import { BaseCommand } from '../commands/baseCommand.js' +import { Diagnostics } from '../constructs/diagnostics.js' import { wrap } from './wrap.js' import logSymbols from 'log-symbols' @@ -110,6 +111,18 @@ export class CommandStyle { this.c.log() } + diagnostics (diagnostics: Diagnostics) { + for (const diag of diagnostics.observations) { + if (diag.isFatal()) { + this.longError(diag.title, diag.message) + } else if (!diag.isBenign()) { + this.longWarning(diag.title, diag.message) + } else { + this.longInfo(diag.title, diag.message) + } + } + } + fatal (message: string) { this.c.log(chalk.red(message)) this.c.log() diff --git a/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts b/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts index ac2794275..9e1e4352f 100644 --- a/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts +++ b/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts @@ -3,27 +3,69 @@ import path from 'node:path' import { describe, it, expect } from 'vitest' import { loadChecklyConfig, defaultFilenames } from '../checkly-config-loader.js' +import { InvalidConfigError } from '../config-diagnostics.js' import { splitConfigFilePath } from '../util.js' +const configDir = path.join(__dirname, 'fixtures', 'configs') + +async function loadInvalidConfig (filename: string): Promise { + const error = await loadChecklyConfig(configDir, [filename]).then( + () => undefined, + err => err, + ) + expect(error).toBeInstanceOf(InvalidConfigError) + return error +} + describe('loadChecklyConfig()', () => { it('config file should export an object', async () => { - try { - await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), ['no-export-config.js']) - } catch (e: any) { - expect(e.message).toContain('Config object missing a logicalId as type string') - } + await expect(loadChecklyConfig(configDir, ['no-export-config.js'])) + .rejects.toThrow(`Property "logicalId" is required and must be set`) }) it('config file should export an object with projectName and logicalId', async () => { - try { - await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), ['no-logical-id-config.js']) - } catch (e: any) { - expect(e.message).toContain('Config object missing a logicalId as type string') - } + await expect(loadChecklyConfig(configDir, ['no-logical-id-config.js'])) + .rejects.toThrow(`Property "logicalId" is required and must be set`) + }) + it('reports all config errors in a single run', async () => { + const error = await loadInvalidConfig('multiple-errors.js') + expect(error.diagnostics.isFatal()).toBe(true) + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining(`Property "logicalId" is required and must be set`), + }), + expect.objectContaining({ + message: expect.stringContaining(`The value provided for property "bundle" is not valid`), + }), + expect.objectContaining({ + message: expect.stringContaining(`Property "runner.registires" is not supported`), + }), + ])) + }) + it('attributes diagnostics to the config file', async () => { + const error = await loadInvalidConfig('multiple-errors.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringMatching(/^\[\S*multiple-errors\.js\] /), + }), + ])) + }) + it('reports present but non-string required fields as invalid values', async () => { + const error = await loadInvalidConfig('non-string-fields.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringContaining(`Invalid property value`), + message: expect.stringContaining(`The value provided for property "logicalId" is not valid`), + }), + expect.objectContaining({ + title: expect.stringContaining(`Invalid property value`), + message: expect.stringContaining(`The value provided for property "projectName" is not valid`), + }), + ])) }) it('error should indicate the tried file name combinations', async () => { - const configDir = path.join(__dirname, 'fixtures', 'not-existing-config-path') + const missingConfigDir = path.join(__dirname, 'fixtures', 'not-existing-config-path') try { - await loadChecklyConfig(configDir) + await loadChecklyConfig(missingConfigDir) } catch (e: any) { expect(e.message).toContain(`Unable to detect a Checkly configuration file`) for (const filename of defaultFilenames) { @@ -41,7 +83,8 @@ describe('loadChecklyConfig()', () => { const { config, - } = await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), [filename]) + diagnostics, + } = await loadChecklyConfig(configDir, [filename]) expect(config).toMatchObject({ checks: { @@ -51,6 +94,8 @@ describe('loadChecklyConfig()', () => { }, }, }) + expect(diagnostics.isFatal()).toBe(false) + expect(diagnostics.observations).toEqual([]) }) it('config JS file should export an object', async () => { const filename = 'good-config.js' @@ -62,7 +107,7 @@ describe('loadChecklyConfig()', () => { const { config, - } = await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), [filename]) + } = await loadChecklyConfig(configDir, [filename]) expect(config).toMatchObject({ checks: { @@ -75,39 +120,39 @@ describe('loadChecklyConfig()', () => { }) it('accepts a string caching.dependencyCache.version', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-string.ts'], ) expect(config.caching?.dependencyCache?.version).toBe('v2') }) it('accepts 0 as a caching.dependencyCache.version', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-zero.ts'], ) expect(config.caching?.dependencyCache?.version).toBe(0) }) it('rejects a non-integer caching.dependencyCache.version', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-float.js'], - )).rejects.toThrow(`Config field 'caching.dependencyCache.version' must be a string or a safe integer if set`) + )).rejects.toThrow(`The value provided for property "caching.dependencyCache.version" is not valid`) }) it('rejects an unsafe integer caching.dependencyCache.version', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-unsafe-integer.js'], - )).rejects.toThrow(`Config field 'caching.dependencyCache.version' must be a string or a safe integer if set`) + )).rejects.toThrow(`must be a safe integer if given as a number`) }) it('rejects a caching.dependencyCache.version that is neither string nor number', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-bad-type.js'], - )).rejects.toThrow(`Config field 'caching.dependencyCache.version' must be a string or a safe integer if set`) + )).rejects.toThrow(`must be a string or a safe integer`) }) it('accepts valid bundle.packages.embed entries', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-valid.ts'], ) expect(config.bundle?.packages?.embed) @@ -115,44 +160,55 @@ describe('loadChecklyConfig()', () => { }) it('rejects a bundle.packages.embed that is not an array', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-not-array.js'], - )).rejects.toThrow(`Config field 'bundle.packages.embed' must be an array of strings if set`) + )).rejects.toThrow(`The value provided for property "bundle.packages.embed" is not valid`) }) it('rejects a bundle that is not an object', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-bundle-not-object.js'], - )).rejects.toThrow(`Config field 'bundle' must be an object if set`) + )).rejects.toThrow(`The value provided for property "bundle" is not valid`) }) it('rejects a bundle.packages that is not an object', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-packages-not-object.js'], - )).rejects.toThrow(`Config field 'bundle.packages' must be an object if set`) + )).rejects.toThrow(`The value provided for property "bundle.packages" is not valid`) }) it('rejects a bundle.packages.embed entry that is not a valid package name', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-bad-name.js'], )).rejects.toThrow(`is not a valid npm package name`) }) it('rejects a bundle.packages.embed entry with a version range', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-range-version.js'], )).rejects.toThrow(`is not an exact semver version`) }) + it('reports every invalid bundle.packages.embed entry', async () => { + const error = await loadInvalidConfig('embedded-packages-multiple-bad.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining(`is not a valid npm package name`), + }), + expect.objectContaining({ + message: expect.stringContaining(`is not an exact semver version`), + }), + ])) + }) it('accepts a bundle.packages.prune pattern array', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-valid.ts'], ) expect(config.bundle?.packages?.prune).toEqual(['@acme/*', '!@acme/keep', 'left-pad']) }) it('accepts a bundle.packages.prune per-class map', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-valid-classes.ts'], ) expect(config.bundle?.packages?.prune).toEqual({ @@ -162,37 +218,35 @@ describe('loadChecklyConfig()', () => { }) it('rejects a bundle.packages.prune that is neither array nor object', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-bad-shape.js'], )).rejects.toThrow( - `Config field 'bundle.packages.prune' is invalid: must be an array of package name patterns` - + ` or an object keyed by dependency class`, + `must be an array of package name patterns or an object keyed by dependency class`, ) }) it('rejects a bundle.packages.prune with an unknown dependency class', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-bad-class.js'], - )).rejects.toThrow(`Config field 'bundle.packages.prune' is invalid: 'peerDependences' is not a dependency class`) + )).rejects.toThrow(`'peerDependences' is not a dependency class`) }) it('rejects a bundle.packages.prune class value that is neither true nor an array', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-bad-class-value.js'], )).rejects.toThrow( - `Config field 'bundle.packages.prune' is invalid: 'peerDependencies' must be true` - + ` or an array of package name patterns`, + `'peerDependencies' must be true or an array of package name patterns`, ) }) it('rejects a bundle.packages.prune entry with an embed-style version pin', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-bad-pattern.js'], )).rejects.toThrow(`'name@version' pins are not supported here`) }) it('accepts a valid runner.registries configuration', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-valid.ts'], ) expect(config.runner?.registries).toEqual({ @@ -211,28 +265,61 @@ describe('loadChecklyConfig()', () => { }) it('rejects a runner that is not an object', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-runner-not-object.js'], - )).rejects.toThrow(`Config field 'runner' must be an object if set`) + )).rejects.toThrow(`The value provided for property "runner" is not valid`) }) it('rejects a misspelled key inside the runner block', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-misspelled-key.js'], - )).rejects.toThrow(`Config field 'runner' contains unknown field 'registires' (expected only: 'registries')`) + )).rejects.toThrow(`Property "runner.registires" is not supported`) + }) + it('reports every runner.registries issue in a single run', async () => { + const error = await loadInvalidConfig('runner-registries-multiple-errors.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining(`property "runner.registries"`), + }), + expect.objectContaining({ + message: expect.stringContaining(`upstream 'yarnpkg': 'auth.type' must be 'bearer'`), + }), + expect.objectContaining({ + message: expect.stringContaining(`upstream 'yarnpkg': 'auth.token' must be exactly one environment variable reference`), + }), + expect.objectContaining({ + message: expect.stringContaining(`packages[0]: upstream 'red' is not defined under 'upstreams' (defined: 'yarnpkg')`), + }), + ])) + }) + it('reports every bundle.packages.prune issue in a single run', async () => { + const error = await loadInvalidConfig('bundle-packages-prune-multiple-errors.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining(`property "bundle.packages.prune"`), + }), + expect.objectContaining({ + message: expect.stringContaining(`'peerDependences' is not a dependency class`), + }), + expect.objectContaining({ + message: expect.stringContaining(`'name@version' pins are not supported here`), + }), + expect.objectContaining({ + message: expect.stringContaining(`'dependencies' must be true or an array of package name patterns`), + }), + ])) }) it('rejects a runner.registries rule using an unknown upstream name', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-unknown-upstream.js'], )).rejects.toThrow( - `Config field 'runner.registries' is invalid: packages[0]: upstream 'mirror' is not defined` - + ` under 'upstreams' (defined: 'npmjs')`, + `packages[0]: upstream 'mirror' is not defined under 'upstreams' (defined: 'npmjs')`, ) }) it('rejects a runner.registries auth token without a ${VAR} reference', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-literal-token.js'], )).rejects.toThrow(/must be exactly one environment variable reference in \$\{VAR\} syntax/) }) @@ -246,7 +333,7 @@ describe('loadChecklyConfig()', () => { const { config, - } = await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), [filename]) + } = await loadChecklyConfig(configDir, [filename]) expect(config).toMatchObject({ checks: { diff --git a/packages/cli/src/services/__tests__/config-diagnostics.spec.ts b/packages/cli/src/services/__tests__/config-diagnostics.spec.ts new file mode 100644 index 000000000..a715f9ff5 --- /dev/null +++ b/packages/cli/src/services/__tests__/config-diagnostics.spec.ts @@ -0,0 +1,78 @@ +import { describe, it, expect } from 'vitest' + +import { ConfigFileDiagnostic, ConfigFileDiagnostics, InvalidConfigError } from '../config-diagnostics.js' +import { Diagnostics, ErrorDiagnostic, WarningDiagnostic } from '../../constructs/diagnostics.js' + +describe('ConfigFileDiagnostic', () => { + it('prefixes the title with the config file name and delegates severity', () => { + const underlying = new ErrorDiagnostic({ + title: 'Invalid property value', + message: 'The value is not valid.', + error: new Error('The value is not valid.'), + }) + const diagnostic = new ConfigFileDiagnostic('checkly.config.ts', underlying) + expect(diagnostic.title).toBe('[checkly.config.ts] Invalid property value') + expect(diagnostic.message).toBe('The value is not valid.') + expect(diagnostic.isFatal()).toBe(true) + expect(diagnostic.isBenign()).toBe(false) + }) +}) + +describe('ConfigFileDiagnostics', () => { + it('wraps added diagnostics in ConfigFileDiagnostic', () => { + const diagnostics = new ConfigFileDiagnostics('checkly.config.ts') + diagnostics.add(new WarningDiagnostic({ + title: 'Some warning', + message: 'Something looks off.', + })) + expect(diagnostics.isFatal()).toBe(false) + expect(diagnostics.observations).toEqual([ + expect.objectContaining({ + title: '[checkly.config.ts] Some warning', + }), + ]) + }) + + it('renders before later diagnostics when extended into a collector first', () => { + const configDiagnostics = new ConfigFileDiagnostics('checkly.config.ts') + configDiagnostics.add(new WarningDiagnostic({ + title: 'Config warning', + message: 'A config-level warning.', + })) + + const diagnostics = new Diagnostics() + diagnostics.extend(configDiagnostics) + diagnostics.add(new WarningDiagnostic({ + title: 'Construct warning', + message: 'A construct-level warning.', + })) + + expect(diagnostics.observations.map(diagnostic => diagnostic.title)).toEqual([ + '[checkly.config.ts] Config warning', + 'Construct warning', + ]) + }) +}) + +describe('InvalidConfigError', () => { + it('composes its message from the fatal observations', () => { + const diagnostics = new ConfigFileDiagnostics('checkly.config.ts') + diagnostics.add(new ErrorDiagnostic({ + title: 'Invalid property value', + message: 'The value provided for property "bundle" is not valid.', + error: new Error('The value provided for property "bundle" is not valid.'), + })) + diagnostics.add(new WarningDiagnostic({ + title: 'Some warning', + message: 'Not included in the message.', + })) + + const error = new InvalidConfigError(diagnostics) + expect(error.name).toBe('InvalidConfigError') + expect(error.diagnostics).toBe(diagnostics) + expect(error.message).toContain('Checkly configuration is not valid:') + expect(error.message).toContain('[checkly.config.ts] Invalid property value') + expect(error.message).toContain('The value provided for property "bundle" is not valid.') + expect(error.message).not.toContain('Not included in the message.') + }) +}) diff --git a/packages/cli/src/services/__tests__/fixtures/configs/bundle-packages-prune-multiple-errors.js b/packages/cli/src/services/__tests__/fixtures/configs/bundle-packages-prune-multiple-errors.js new file mode 100644 index 000000000..c223f296c --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/bundle-packages-prune-multiple-errors.js @@ -0,0 +1,19 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. Contains several independent errors +// inside bundle.packages.prune to verify that all of them are reported in a +// single run. +const config = { + projectName: 'test-config-project', + logicalId: 'test-config-project', + bundle: { + packages: { + prune: { + peerDependences: true, + devDependencies: ['ok-name', 'bad@1.0.0'], + dependencies: 5, + }, + }, + }, +} + +export default config diff --git a/packages/cli/src/services/__tests__/fixtures/configs/embedded-packages-multiple-bad.js b/packages/cli/src/services/__tests__/fixtures/configs/embedded-packages-multiple-bad.js new file mode 100644 index 000000000..6fe1dee8b --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/embedded-packages-multiple-bad.js @@ -0,0 +1,14 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation of bundle.packages.embed is what rejects it. Contains +// two invalid specs to verify that both are reported in a single run. +const config = { + projectName: 'test-config-project', + logicalId: 'test-config-project', + bundle: { + packages: { + embed: ['Not A Valid Name', 'left-pad@^1.0.0'], + }, + }, +} + +export default config diff --git a/packages/cli/src/services/__tests__/fixtures/configs/multiple-errors.js b/packages/cli/src/services/__tests__/fixtures/configs/multiple-errors.js new file mode 100644 index 000000000..d1ceb874c --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/multiple-errors.js @@ -0,0 +1,13 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. Contains several independent errors +// to verify that all of them are reported in a single run. +const config = { + projectName: 'multiple-errors', + // logicalId is missing + bundle: 5, + runner: { + registires: {}, + }, +} + +export default config diff --git a/packages/cli/src/services/__tests__/fixtures/configs/non-string-fields.js b/packages/cli/src/services/__tests__/fixtures/configs/non-string-fields.js new file mode 100644 index 000000000..5c53019d7 --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/non-string-fields.js @@ -0,0 +1,9 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. The fields are present but not +// strings, which is a different diagnostic than a missing field. +const config = { + projectName: 42, + logicalId: false, +} + +export default config diff --git a/packages/cli/src/services/__tests__/fixtures/configs/runner-registries-multiple-errors.js b/packages/cli/src/services/__tests__/fixtures/configs/runner-registries-multiple-errors.js new file mode 100644 index 000000000..d07ac1ecf --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/runner-registries-multiple-errors.js @@ -0,0 +1,23 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. Contains several independent errors +// inside runner.registries to verify that all of them are reported in a +// single run. +const config = { + projectName: 'test-config-project', + logicalId: 'test-config-project', + runner: { + registries: { + upstreams: { + yarnpkg: { + url: 'https://registry.yarnpkg.com/', + auth: {}, + }, + }, + packages: [ + { pattern: '**', upstreams: ['yarnpkg', 'red'] }, + ], + }, + }, +} + +export default config diff --git a/packages/cli/src/services/check-parser/__tests__/package-prune.spec.ts b/packages/cli/src/services/check-parser/__tests__/package-prune.spec.ts index 1fee83f42..4bc33ba50 100644 --- a/packages/cli/src/services/check-parser/__tests__/package-prune.spec.ts +++ b/packages/cli/src/services/check-parser/__tests__/package-prune.spec.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest' import { + collectPackagePruneIssues, DEPENDENCY_CLASSES, normalizePackagePrune, prunePackageJson, @@ -127,7 +128,55 @@ describe('normalizePackagePrune()', () => { it('rejects invalid patterns in either shape', () => { expect(() => normalizePackagePrune(['pkg@1.0.0'])).toThrow(InvalidPackageNamePatternError) expect(() => normalizePackagePrune({ dependencies: ['pkg@1.0.0'] })) - .toThrow(InvalidPackageNamePatternError) + .toThrow(`'dependencies': Invalid package name pattern 'pkg@1.0.0'`) + }) +}) + +function packagePruneIssueErrors (raw: unknown): Error[] { + const issues: Error[] = [] + collectPackagePruneIssues(raw as any, issue => issues.push(issue)) + return issues +} + +function packagePruneIssues (raw: unknown): string[] { + return packagePruneIssueErrors(raw).map(issue => issue.message) +} + +describe('collectPackagePruneIssues()', () => { + it('returns no issues for valid values', () => { + expect(packagePruneIssues(undefined)).toEqual([]) + expect(packagePruneIssues(['@acme/*'])).toEqual([]) + expect(packagePruneIssues({ peerDependencies: true })).toEqual([]) + }) + + it('collects every issue instead of stopping at the first', () => { + const issues = packagePruneIssues({ + peerDependences: true, + devDependencies: ['ok-name', 'bad@1.0.0'], + dependencies: 5, + }) + expect(issues).toEqual([ + expect.stringContaining(`'peerDependences' is not a dependency class`), + expect.stringContaining(`'name@version' pins are not supported here`), + expect.stringContaining(`'dependencies' must be true or an array of package name patterns`), + ]) + }) + + it('reports a bad array-shape entry once, not once per dependency class', () => { + const issues = packagePruneIssueErrors(['pkg@1.0.0']) + expect(issues).toHaveLength(1) + expect(issues[0]).toBeInstanceOf(InvalidPackageNamePatternError) + }) + + it('distinguishes the same bad pattern across dependency classes', () => { + const issues = packagePruneIssues({ + dependencies: ['bad@1.0.0'], + devDependencies: ['bad@1.0.0'], + }) + expect(issues).toEqual([ + expect.stringContaining(`'dependencies': Invalid package name pattern 'bad@1.0.0'`), + expect.stringContaining(`'devDependencies': Invalid package name pattern 'bad@1.0.0'`), + ]) }) }) diff --git a/packages/cli/src/services/check-parser/cache-hash.ts b/packages/cli/src/services/check-parser/cache-hash.ts index a01f4c872..6aa1b9f45 100644 --- a/packages/cli/src/services/check-parser/cache-hash.ts +++ b/packages/cli/src/services/check-parser/cache-hash.ts @@ -478,12 +478,12 @@ export function normalizeDependencyCacheVersion (version: string | number | unde } if (typeof version === 'number') { if (!Number.isSafeInteger(version)) { - throw new Error(`Dependency cache version must be a safe integer if given as a number, got ${version}`) + throw new Error(`Dependency cache version must be a safe integer if given as a number, got ${version}.`) } return String(version) } if (typeof version !== 'string') { - throw new Error(`Dependency cache version must be a string or a safe integer, got ${typeof version}`) + throw new Error(`Dependency cache version must be a string or a safe integer, got ${typeof version}.`) } return version } diff --git a/packages/cli/src/services/check-parser/package-prune.ts b/packages/cli/src/services/check-parser/package-prune.ts index 9ebc11793..75ebb6ad0 100644 --- a/packages/cli/src/services/check-parser/package-prune.ts +++ b/packages/cli/src/services/check-parser/package-prune.ts @@ -36,43 +36,81 @@ export type NormalizedPackagePrune = { [K in DependencyClass]?: true | PackageNamePattern[] } -const SHAPE_ERROR = `must be an array of package name patterns or an object keyed by dependency class` +const SHAPE_ERROR = `must be an array of package name patterns or an object keyed by dependency class.` + +/** + * Walks a `bundle.packages.prune` value, reporting every issue through + * `onIssue` so that all of them surface in a single run — the config + * loader turns each one into its own diagnostic. Pattern issues from the + * per-class map shape are prefixed with the dependency class; array-shape + * pattern issues keep their `InvalidPackageNamePatternError` class and + * message as-is. + */ +export function collectPackagePruneIssues ( + raw: BundlePackagesPrune | undefined, + onIssue: (issue: Error) => void, +): void { + normalizePackagePruneInternal(raw, onIssue) +} /** * Validates and normalizes a `bundle.packages.prune` value. Returns * `undefined` when there is nothing to do — the value is absent, or every - * shape it carries is empty. Throws `InvalidPackageNamePatternError` on an - * invalid pattern and a plain `Error` on an invalid shape; the config - * loader relies on that to reject plain-JS configs that bypass the - * TypeScript type. + * shape it carries is empty. Throws the first issue found; the bundler + * relies on that to reject a value that bypassed config validation. */ export function normalizePackagePrune (raw: BundlePackagesPrune | undefined): NormalizedPackagePrune | undefined { + let firstIssue: Error | undefined + const normalized = normalizePackagePruneInternal(raw, issue => { + firstIssue ??= issue + }) + if (firstIssue !== undefined) { + throw firstIssue + } + + return normalized +} + +function normalizePackagePruneInternal ( + raw: BundlePackagesPrune | undefined, + onIssue: (issue: Error) => void, +): NormalizedPackagePrune | undefined { if (raw === undefined) { return undefined } - let perClass: Record if (Array.isArray(raw)) { - perClass = Object.fromEntries(DEPENDENCY_CLASSES.map(dependencyClass => [dependencyClass, raw])) - } else if (raw !== null && typeof raw === 'object') { - // A non-plain object (a Set, a Map, a Date) has no own enumerable - // string keys, so without this check it would silently normalize to - // "nothing to do" instead of being rejected. - const proto = Object.getPrototypeOf(raw) - if (proto !== Object.prototype && proto !== null) { - throw new Error(SHAPE_ERROR) + // The pattern array applies to every dependency class, but is parsed + // only once so a bad entry does not repeat as an issue for each class. + const patterns = parsePatternArray(raw, onIssue) + if (patterns === undefined) { + return undefined } - perClass = raw - } else { - throw new Error(SHAPE_ERROR) + // A separate array per class, so a consumer mutating one class's list + // cannot silently change the others. (The pattern objects themselves + // are shared; they are read-only everywhere.) + return Object.fromEntries( + DEPENDENCY_CLASSES.map(dependencyClass => [dependencyClass, [...patterns]]), + ) as NormalizedPackagePrune + } + + // A non-plain object (a Set, a Map, a Date) has no own enumerable + // string keys, so without the prototype check it would silently + // normalize to "nothing to do" instead of being rejected. + const proto = raw !== null && typeof raw === 'object' ? Object.getPrototypeOf(raw) : undefined + if (proto !== Object.prototype && proto !== null) { + onIssue(new Error(SHAPE_ERROR)) + return undefined } + const perClass: Record = raw const normalized: NormalizedPackagePrune = {} for (const [key, value] of Object.entries(perClass)) { if (!DEPENDENCY_CLASSES.includes(key as DependencyClass)) { - throw new Error( - `'${key}' is not a dependency class (expected one of ${DEPENDENCY_CLASSES.join(', ')})`, - ) + onIssue(new Error( + `'${key}' is not a dependency class (expected one of ${DEPENDENCY_CLASSES.join(', ')}).`, + )) + continue } if (value === undefined) { continue @@ -82,16 +120,16 @@ export function normalizePackagePrune (raw: BundlePackagesPrune | undefined): No continue } if (Array.isArray(value)) { - if (value.length === 0) { - continue + // The dependency class prefixes any pattern issue: the same bad + // pattern may appear under several classes, and without the prefix + // their issues would be indistinguishable. + const patterns = parsePatternArray(value, onIssue, `'${key}'`) + if (patterns !== undefined) { + normalized[key as DependencyClass] = patterns } - // Parsed per class even for the array shape, so no two classes share - // one pattern array instance and a consumer mutating one cannot - // silently change the others. - normalized[key as DependencyClass] = value.map(entry => parsePackageNamePattern(entry as string)) continue } - throw new Error(`'${key}' must be true or an array of package name patterns`) + onIssue(new Error(`'${key}' must be true or an array of package name patterns.`)) } if (Object.keys(normalized).length === 0) { @@ -101,6 +139,36 @@ export function normalizePackagePrune (raw: BundlePackagesPrune | undefined): No return normalized } +/** + * Parses a pattern array, collecting an issue per invalid entry + * (prefixed with `context` when given). Returns `undefined` when the + * array is empty (nothing to do) or any entry is invalid. + */ +function parsePatternArray ( + value: unknown[], + onIssue: (issue: Error) => void, + context?: string, +): PackageNamePattern[] | undefined { + const patterns: PackageNamePattern[] = [] + for (const entry of value) { + try { + patterns.push(parsePackageNamePattern(entry as string)) + } catch (cause) { + onIssue(context === undefined + ? cause as Error + : new Error(`${context}: ${(cause as Error).message}`, { cause })) + } + } + + // Each entry contributed either a pattern or an issue, so a shorter + // patterns list means some entry was invalid. + if (patterns.length !== value.length || patterns.length === 0) { + return undefined + } + + return patterns +} + export interface PrunePackageJsonResult { content: string /** diff --git a/packages/cli/src/services/checkly-config-loader.ts b/packages/cli/src/services/checkly-config-loader.ts index e189bac73..dc0295fd7 100644 --- a/packages/cli/src/services/checkly-config-loader.ts +++ b/packages/cli/src/services/checkly-config-loader.ts @@ -5,14 +5,21 @@ import { CheckProps, RuntimeCheckProps } from '../constructs/check.js' import { PlaywrightCheckProps } from '../constructs/playwright-check.js' import { Session } from '../constructs/index.js' import { Construct } from '../constructs/construct.js' +import { Diagnostics } from '../constructs/diagnostics.js' +import { + InvalidPropertyValueDiagnostic, + RequiredPropertyDiagnostic, + UnsupportedPropertyDiagnostic, +} from '../constructs/construct-diagnostics.js' +import { ConfigFileDiagnostics, InvalidConfigError } from './config-diagnostics.js' import type { Region } from '../index.js' import { ReporterType } from '../reporters/reporter.js' import { PlaywrightConfig } from '../constructs/playwright-config.js' import { FileLoader } from '../loader/index.js' import { normalizeDependencyCacheVersion } from './check-parser/cache-hash.js' -import { BundlePackagesPrune, normalizePackagePrune } from './check-parser/package-prune.js' +import { BundlePackagesPrune, collectPackagePruneIssues } from './check-parser/package-prune.js' import { parseEmbeddedPackageSpec } from './embedded-packages/spec.js' -import { Registries, validateRegistries } from './runner/registries.js' +import { Registries, collectRegistriesIssues } from './runner/registries.js' export type CheckConfigDefaults = Pick { +): Promise<{ config: ChecklyConfig, constructs: Construct[], diagnostics: Diagnostics }> { Session.loadingChecklyConfigFile = true try { let config: ChecklyConfig | undefined + // When no config file exists, a default config may be generated in + // memory without ever being written to disk, in which case there is no + // file to attribute diagnostics to. + let configFileName = '' Session.checklyConfigFileConstructs = [] for (const filename of filenames) { const filePath = path.join(dir, filename) @@ -407,15 +428,26 @@ export async function loadChecklyConfig ( continue } config = await Session.loadFile(filePath) + configFileName = path.relative(process.cwd(), filePath) break } if (!config) { config = await handleMissingConfig(dir, filenames, writeChecklyConfig, playwrightConfigPath) + if (writeChecklyConfig) { + // handleMissingConfig() wrote the generated default config to disk. + configFileName = path.relative(process.cwd(), path.join(dir, 'checkly.config.ts')) + } + } + + const diagnostics = new ConfigFileDiagnostics(configFileName) + validateConfigFields(config, ['logicalId', 'projectName'] as const, diagnostics) + validateDependencyCacheVersion(config, diagnostics) + validateBundle(config, diagnostics) + validateRunner(config, diagnostics) + + if (diagnostics.isFatal()) { + throw new InvalidConfigError(diagnostics) } - validateConfigFields(config, ['logicalId', 'projectName'] as const) - validateDependencyCacheVersion(config) - validateBundle(config) - validateRunner(config) const constructs = Session.checklyConfigFileConstructs @@ -423,7 +455,7 @@ export async function loadChecklyConfig ( if (config.cli?.loader) { Session.loader = config.cli.loader } - return { config, constructs } + return { config, constructs, diagnostics } } finally { Session.loadingChecklyConfigFile = false } @@ -447,26 +479,39 @@ async function handleMissingConfig ( throw new ConfigNotFoundError([dir], filenames) } -function validateConfigFields (config: ChecklyConfig, fields: (keyof ChecklyConfig)[]): void { +function validateConfigFields ( + config: ChecklyConfig, + fields: (keyof ChecklyConfig)[], + diagnostics: Diagnostics, +): void { for (const field of fields) { - if (!config?.[field] || !isString(config[field])) { - throw new Error(`Config object missing a ${field} as type string`) + const value = config?.[field] + if (value === undefined || value === null || value === '') { + diagnostics.add(new RequiredPropertyDiagnostic( + field, + new Error(`Value must be a non-empty string.`), + )) + } else if (!isString(value)) { + diagnostics.add(new InvalidPropertyValueDiagnostic( + field, + new Error(`Value must be a non-empty string.`), + )) } } } -function validateDependencyCacheVersion (config: ChecklyConfig): void { +function validateDependencyCacheVersion (config: ChecklyConfig, diagnostics: Diagnostics): void { try { normalizeDependencyCacheVersion(config.caching?.dependencyCache?.version) } catch (cause) { - throw new Error( - `Config field 'caching.dependencyCache.version' must be a string or a safe integer if set`, - { cause }, - ) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'caching.dependencyCache.version', + cause as Error, + )) } } -function validateBundle (config: ChecklyConfig): void { +function validateBundle (config: ChecklyConfig, diagnostics: Diagnostics): void { const { bundle } = config if (bundle === undefined) { return @@ -477,7 +522,11 @@ function validateBundle (config: ChecklyConfig): void { // the runner. Plain-JS configs bypass the TypeScript type, so the shape // must be enforced at runtime. if (bundle === null || typeof bundle !== 'object' || Array.isArray(bundle)) { - throw new Error(`Config field 'bundle' must be an object if set`) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'bundle', + new Error(`Value must be an object if set.`), + )) + return } const { packages } = bundle @@ -486,14 +535,16 @@ function validateBundle (config: ChecklyConfig): void { } if (packages === null || typeof packages !== 'object' || Array.isArray(packages)) { - throw new Error(`Config field 'bundle.packages' must be an object if set`) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'bundle.packages', + new Error(`Value must be an object if set.`), + )) + return } - try { - normalizePackagePrune(packages.prune) - } catch (cause) { - throw new Error(`Config field 'bundle.packages.prune' is invalid: ${(cause as Error).message}`, { cause }) - } + collectPackagePruneIssues(packages.prune, issue => { + diagnostics.add(new InvalidPropertyValueDiagnostic('bundle.packages.prune', issue)) + }) const embeddedPackages = packages.embed if (embeddedPackages === undefined) { @@ -501,19 +552,26 @@ function validateBundle (config: ChecklyConfig): void { } if (!Array.isArray(embeddedPackages)) { - throw new Error(`Config field 'bundle.packages.embed' must be an array of strings if set`) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'bundle.packages.embed', + new Error(`Value must be an array of strings if set.`), + )) + return } for (const spec of embeddedPackages) { try { parseEmbeddedPackageSpec(spec) } catch (cause) { - throw new Error(`Config field 'bundle.packages.embed' is invalid: ${(cause as Error).message}`, { cause }) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'bundle.packages.embed', + cause as Error, + )) } } } -function validateRunner (config: ChecklyConfig): void { +function validateRunner (config: ChecklyConfig, diagnostics: Diagnostics): void { const { runner } = config if (runner === undefined) { return @@ -524,14 +582,21 @@ function validateRunner (config: ChecklyConfig): void { // `registries: undefined` and silently disable routing, surfacing only as // an install failure on the runner. if (runner === null || typeof runner !== 'object' || Array.isArray(runner)) { - throw new Error(`Config field 'runner' must be an object if set`) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'runner', + new Error(`Value must be an object if set.`), + )) + return } // A misspelled key (`registires`) would silently disable routing the same // way a misshapen block would. for (const key of Object.keys(runner)) { if (key !== 'registries') { - throw new Error(`Config field 'runner' contains unknown field '${key}' (expected only: 'registries')`) + diagnostics.add(new UnsupportedPropertyDiagnostic( + `runner.${key}`, + new Error(`Only 'registries' is supported in the 'runner' block.`), + )) } } @@ -540,9 +605,7 @@ function validateRunner (config: ChecklyConfig): void { return } - try { - validateRegistries(registries) - } catch (cause) { - throw new Error(`Config field 'runner.registries' is invalid: ${(cause as Error).message}`, { cause }) - } + collectRegistriesIssues(registries, issue => { + diagnostics.add(new InvalidPropertyValueDiagnostic('runner.registries', issue)) + }) } diff --git a/packages/cli/src/services/config-diagnostics.ts b/packages/cli/src/services/config-diagnostics.ts new file mode 100644 index 000000000..6db7d6666 --- /dev/null +++ b/packages/cli/src/services/config-diagnostics.ts @@ -0,0 +1,66 @@ +import { Diagnostic, Diagnostics } from '../constructs/diagnostics.js' + +/** + * Attributes an underlying diagnostic to the Checkly configuration file it + * originates from by prefixing the title with the file name. + */ +export class ConfigFileDiagnostic extends Diagnostic { + underlying: Diagnostic + + constructor (fileName: string, underlying: Diagnostic) { + super({ + title: `[${fileName}] ${underlying.title}`, + message: underlying.message, + }) + + this.underlying = underlying + } + + isFatal (): boolean { + return this.underlying.isFatal() + } + + isBenign (): boolean { + return this.underlying.isBenign() + } +} + +/** + * A Diagnostics collector that attributes every added diagnostic to a + * Checkly configuration file. + */ +export class ConfigFileDiagnostics extends Diagnostics { + fileName: string + + constructor (fileName: string) { + super() + this.fileName = fileName + } + + add (diagnostic: Diagnostic): void { + super.add(new ConfigFileDiagnostic(this.fileName, diagnostic)) + } +} + +/** + * Thrown when the Checkly configuration file contains fatal diagnostics. + * + * The message is composed from the fatal observations so that the error + * remains readable even when rendered by a generic error handler. Callers + * that can render diagnostics should prefer the `diagnostics` field. + */ +export class InvalidConfigError extends Error { + diagnostics: Diagnostics + + constructor (diagnostics: Diagnostics) { + const fatalObservations = diagnostics.observations.filter(diagnostic => diagnostic.isFatal()) + const message = `Checkly configuration is not valid:` + + `\n\n` + + fatalObservations + .map(diagnostic => `${diagnostic.title}\n\n${diagnostic.message}`) + .join('\n\n') + super(message) + this.name = 'InvalidConfigError' + this.diagnostics = diagnostics + } +} diff --git a/packages/cli/src/services/embedded-packages/spec.ts b/packages/cli/src/services/embedded-packages/spec.ts index 4347fffb4..f0701c5ea 100644 --- a/packages/cli/src/services/embedded-packages/spec.ts +++ b/packages/cli/src/services/embedded-packages/spec.ts @@ -203,7 +203,7 @@ export class InvalidEmbeddedPackageSpecError extends Error { readonly reason: string constructor (spec: string, reason: string) { - super(`Invalid embedded package '${spec}': ${reason}`) + super(`Invalid embedded package '${spec}': ${reason}.`) this.name = 'InvalidEmbeddedPackageSpecError' this.reason = reason } @@ -293,7 +293,7 @@ export type PackageNamePattern = Pick ({ upstreams: { @@ -16,6 +16,118 @@ const validRegistries = (): Registries => ({ ], }) +function registriesIssues (value: unknown): string[] { + const issues: string[] = [] + collectRegistriesIssues(value, issue => issues.push(issue.message)) + return issues +} + +describe('collectRegistriesIssues()', () => { + it('returns no issues for a valid configuration', () => { + expect(registriesIssues(validRegistries())).toEqual([]) + }) + + it('collects every issue instead of stopping at the first', () => { + const value: any = validRegistries() + value.upstreams.internal.auth = {} + value.packages[0].upstreams = ['internal', 'mirror'] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`upstream 'internal': 'auth.type' must be 'bearer'`), + expect.stringContaining(`upstream 'internal': 'auth.token' must be exactly one environment variable reference`), + expect.stringContaining(`packages[0]: upstream 'mirror' is not defined under 'upstreams'`), + ]) + }) + + it('skips upstream-existence checks when the upstreams block is invalid', () => { + const value: any = validRegistries() + value.upstreams = 'nope' + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`'upstreams' must be an object mapping upstream names to { url, auth? }`), + ]) + }) + + it('skips upstream-existence checks when the upstreams block is empty', () => { + const value: any = validRegistries() + value.upstreams = {} + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`'upstreams' must define at least one upstream`), + ]) + }) + + it('skips the match-all-position check when the final pattern is invalid', () => { + const value: any = validRegistries() + value.packages[1].pattern = 42 + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[1]: 'pattern' must be a string`), + ]) + }) + + it('does not ask for a match-all rule when a misplaced one exists', () => { + const value: any = validRegistries() + value.packages = [ + { pattern: '**', upstreams: ['npmjs'] }, + { pattern: '@acme/**', upstreams: ['internal'] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[0]: rules after a '**' match-all rule can never apply`), + ]) + }) + + it('does not ask for a match-all rule when the only rule is an exclusion', () => { + const value: any = validRegistries() + value.packages = [ + { pattern: '!**', upstreams: ['npmjs'] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[0]: exclusion patterns ('!') are not supported in routing rules`), + ]) + }) + + it('reports non-string upstream entries even when the upstreams block is invalid', () => { + const value: any = validRegistries() + value.upstreams = {} + value.packages = [ + { pattern: '**', upstreams: ['npmjs', 42] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`'upstreams' must define at least one upstream`), + expect.stringContaining(`packages[0]: 'upstreams'[1] must be an upstream name`), + ]) + }) + + it('does not flag a non-final excluded match-all as a misplaced match-all rule', () => { + const value: any = validRegistries() + value.packages = [ + { pattern: '!**', upstreams: ['npmjs'] }, + { pattern: '**', upstreams: ['npmjs'] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[0]: exclusion patterns ('!') are not supported in routing rules`), + ]) + }) + + it('still asks for a match-all rule when the trailing exclusion is not one', () => { + const value: any = validRegistries() + value.packages = [ + { pattern: '@acme/**', upstreams: ['npmjs'] }, + { pattern: '!@acme/foo', upstreams: ['npmjs'] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[1]: exclusion patterns ('!') are not supported in routing rules`), + expect.stringContaining(`'packages' must end with a match-all rule`), + ]) + }) +}) + describe('validateRegistries()', () => { it('accepts a valid configuration', () => { const value = validRegistries() @@ -95,7 +207,7 @@ describe('validateRegistries()', () => { it('rejects unknown fields at every level', () => { const top = { ...validRegistries(), extra: true } expect(() => validateRegistries(top)) - .toThrow(`'runner.registries': unknown field 'extra' (expected only: 'upstreams', 'packages')`) + .toThrow(`unknown field 'extra' (expected only: 'upstreams', 'packages')`) const upstream = validRegistries() ;(upstream.upstreams.internal as any).authh = { type: 'bearer', token: '${T}' } diff --git a/packages/cli/src/services/runner/registries.ts b/packages/cli/src/services/runner/registries.ts index b3e3449de..b8d8d1030 100644 --- a/packages/cli/src/services/runner/registries.ts +++ b/packages/cli/src/services/runner/registries.ts @@ -1,4 +1,4 @@ -import { parsePackageNamePattern } from '../embedded-packages/spec.js' +import { PackageNamePattern, parsePackageNamePattern } from '../embedded-packages/spec.js' import { COMPOSABLE_URL_REQUIREMENT, parseComposableUrl } from '../embedded-packages/url.js' /** @@ -154,108 +154,156 @@ function isPlainObject (value: unknown): value is Record { * an intention, and ignoring it silently disables whatever the user meant * to configure, surfacing only as an install failure on the runner. */ -function rejectUnknownKeys (context: string, value: Record, known: string[]): void { +function collectUnknownKeyIssues ( + onIssue: (issue: Error) => void, + context: string | undefined, + value: Record, + known: string[], +): void { + const prefix = context === undefined ? '' : `${context}: ` for (const key of Object.keys(value)) { if (!known.includes(key)) { - throw new Error(`${context}: unknown field '${key}' (expected only: ${known.map(k => `'${k}'`).join(', ')})`) + onIssue(new Error(`${prefix}unknown field '${key}' (expected only: ${known.map(k => `'${k}'`).join(', ')}).`)) } } } /** - * Validates the value of `runner.registries` and returns it typed. All - * failures throw a plain `Error` whose message names the offending part - * relative to `runner.registries`; the config loader wraps it with the - * full config path. Invalid URLs and tokens are deliberately never echoed - * back — a malformed registry URL may carry an inline credential. + * Walks a registries value, reporting every issue through `onIssue` so + * that all of them surface in a single run — the config loader turns each + * one into its own diagnostic. Messages name the offending part relative + * to the registries block itself. Invalid URLs and tokens are + * deliberately never echoed back — a malformed registry URL may carry an + * inline credential. + * + * Structural problems gate their dependents: an `upstreams` block that is + * not an object (or is empty) skips the per-rule upstream-existence + * checks (they would all fail spuriously), and a rule whose pattern is + * invalid skips the match-all checks for that rule. * * Plain-JS configs bypass the TypeScript type, so the shape must be * enforced at runtime, exactly as `validateBundle` does for * `bundle.packages`. */ -export function validateRegistries (value: unknown): Registries { +export function collectRegistriesIssues (value: unknown, onIssue: (issue: Error) => void): void { if (!isPlainObject(value)) { - throw new Error(`must be an object`) + onIssue(new Error(`must be an object.`)) + return } - rejectUnknownKeys(`'runner.registries'`, value, ['upstreams', 'packages']) + // No context: the diagnostic wrapping the issue already names the + // registries block's own config path. + collectUnknownKeyIssues(onIssue, undefined, value, ['upstreams', 'packages']) const { upstreams, packages } = value + let upstreamNames: string[] | undefined if (!isPlainObject(upstreams)) { - throw new Error(`'upstreams' must be an object mapping upstream names to { url, auth? }`) - } - - const upstreamNames = Object.keys(upstreams) - if (upstreamNames.length === 0) { - throw new Error(`'upstreams' must define at least one upstream`) - } - - for (const name of upstreamNames) { - validateUpstream(name, upstreams[name]) + onIssue(new Error(`'upstreams' must be an object mapping upstream names to { url, auth? }.`)) + } else if (Object.keys(upstreams).length === 0) { + // Gates the per-rule upstream-existence checks the same way a + // non-object block does: with no upstreams defined, every rule would + // otherwise cascade a spurious issue of its own. + onIssue(new Error(`'upstreams' must define at least one upstream.`)) + } else { + upstreamNames = Object.keys(upstreams) + for (const name of upstreamNames) { + collectUpstreamIssues(onIssue, name, upstreams[name]) + } } if (!Array.isArray(packages)) { - throw new Error(`'packages' must be an array of { pattern, upstreams } routing rules`) + onIssue(new Error(`'packages' must be an array of { pattern, upstreams } routing rules.`)) + return } - let lastPattern: string | undefined + let lastPattern: PackageNamePattern | undefined + let sawMatchAll = false for (const [index, rule] of packages.entries()) { - const { pattern } = validatePackageRoutingRule(index, rule, upstreamNames) + const pattern = collectPackageRoutingRuleIssues(onIssue, index, rule, upstreamNames) // First match wins, so nothing past a match-all rule could ever // apply. Requiring the match-all to be the final rule both guarantees // every package a route and keeps silently dead rules out of the // config. The final-rule check below also rejects an empty rule list. - if (pattern === MATCH_ALL_PATTERN && index !== packages.length - 1) { - throw new Error( - `packages[${index}]: rules after a '${MATCH_ALL_PATTERN}' match-all rule can never apply ` - + `(the first matching rule wins); move the match-all rule last`, - ) + // An excluded '!**' — invalid in itself — is not a match-all rule, so + // it never triggers the position issue, and it only satisfies the + // final-rule check when it is itself the final rule (where removing + // the '!' is all it takes; anywhere else the rule list still needs a + // trailing match-all). + if (pattern !== undefined && pattern.name === MATCH_ALL_PATTERN) { + if (!pattern.exclude && index !== packages.length - 1) { + onIssue(new Error( + `packages[${index}]: rules after a '${MATCH_ALL_PATTERN}' match-all rule can never apply ` + + `(the first matching rule wins); move the match-all rule last.`, + )) + } + if (!pattern.exclude || index === packages.length - 1) { + sawMatchAll = true + } } lastPattern = pattern } - if (lastPattern !== MATCH_ALL_PATTERN) { - throw new Error( + // Skipped when a match-all rule exists (a misplaced one already carries + // its own issue telling the user to move it, and asking them to add one + // they have would contradict it), and when the final rule's own pattern + // is invalid (its issue is already reported and this check would be + // speculative). + if (!sawMatchAll && (packages.length === 0 || lastPattern !== undefined)) { + onIssue(new Error( `'packages' must end with a match-all rule ({ pattern: '${MATCH_ALL_PATTERN}', ... }) ` - + `so that every package has a route`, - ) + + `so that every package has a route.`, + )) + } +} + +/** + * Validates the value of `runner.registries` and returns it typed, + * throwing the first issue found. Invalid URLs and tokens are never + * echoed back — a malformed registry URL may carry an inline credential. + */ +export function validateRegistries (value: unknown): Registries { + let firstIssue: Error | undefined + collectRegistriesIssues(value, issue => { + firstIssue ??= issue + }) + if (firstIssue !== undefined) { + throw firstIssue } return value as unknown as Registries } -function validateUpstream (name: string, value: unknown): void { +function collectUpstreamIssues (onIssue: (issue: Error) => void, name: string, value: unknown): void { if (!UPSTREAM_NAME_RE.test(name)) { - throw new Error( + onIssue(new Error( `upstream name '${name}' is invalid: names must start with a letter or digit ` - + `and contain only letters, digits, '-' and '_'`, - ) + + `and contain only letters, digits, '-' and '_'.`, + )) } if (!isPlainObject(value)) { - throw new Error(`upstream '${name}' must be an object with a 'url'`) + onIssue(new Error(`upstream '${name}' must be an object with a 'url'.`)) + return } - rejectUnknownKeys(`upstream '${name}'`, value, ['url', 'auth']) + collectUnknownKeyIssues(onIssue, `upstream '${name}'`, value, ['url', 'auth']) const { url, auth } = value const parsedUrl = typeof url === 'string' ? parseComposableUrl(url) : undefined if (parsedUrl === undefined) { - throw new Error(`upstream '${name}': 'url' must be ${COMPOSABLE_URL_REQUIREMENT}`) - } - - // An inline credential would ship in plaintext inside the uploaded code - // bundle — the exact outcome the ${VAR}-only rule on auth tokens exists - // to prevent. The URL is not echoed for the same reason. - if (parsedUrl.username !== '' || parsedUrl.password !== '') { - throw new Error( + onIssue(new Error(`upstream '${name}': 'url' must be ${COMPOSABLE_URL_REQUIREMENT}.`)) + } else if (parsedUrl.username !== '' || parsedUrl.password !== '') { + // An inline credential would ship in plaintext inside the uploaded code + // bundle — the exact outcome the ${VAR}-only rule on auth tokens exists + // to prevent. The URL is not echoed for the same reason. + onIssue(new Error( `upstream '${name}': 'url' must not contain credentials; ` - + `use auth: { type: 'bearer', token: '\${VAR}' } instead`, - ) + + `use auth: { type: 'bearer', token: '\${VAR}' } instead.`, + )) } if (auth === undefined) { @@ -263,65 +311,84 @@ function validateUpstream (name: string, value: unknown): void { } if (!isPlainObject(auth)) { - throw new Error(`upstream '${name}': 'auth' must be an object if set`) + onIssue(new Error(`upstream '${name}': 'auth' must be an object if set.`)) + return } - rejectUnknownKeys(`upstream '${name}': 'auth'`, auth, ['type', 'token']) + collectUnknownKeyIssues(onIssue, `upstream '${name}': 'auth'`, auth, ['type', 'token']) if (auth.type !== 'bearer') { - throw new Error(`upstream '${name}': 'auth.type' must be 'bearer'`) + onIssue(new Error(`upstream '${name}': 'auth.type' must be 'bearer'.`)) } if (typeof auth.token !== 'string' || !ENV_VAR_REFERENCE_RE.test(auth.token)) { - throw new Error( + onIssue(new Error( `upstream '${name}': 'auth.token' must be exactly one environment variable reference in \${VAR} syntax ` + `(e.g. '\${NPM_TOKEN}'). The variable is resolved from the check's environment variables on the ` - + `Checkly runner; any literal content would bake a secret into the code bundle`, - ) + + `Checkly runner; any literal content would bake a secret into the code bundle.`, + )) } } -function validatePackageRoutingRule (index: number, value: unknown, upstreamNames: string[]): PackageRoutingRule { +/** + * Collects the issues of one routing rule. Returns the rule's parsed + * pattern (its name has any leading '!' stripped) when it is a parseable + * string, so the caller can run the match-all checks; `undefined` means + * the pattern is unknown and those checks do not apply. When + * `upstreamNames` is undefined (the `upstreams` block itself is invalid), + * the upstream-existence checks are skipped. + */ +function collectPackageRoutingRuleIssues ( + onIssue: (issue: Error) => void, + index: number, + value: unknown, + upstreamNames: string[] | undefined, +): PackageNamePattern | undefined { if (!isPlainObject(value)) { - throw new Error(`packages[${index}] must be an object with 'pattern' and 'upstreams'`) + onIssue(new Error(`packages[${index}] must be an object with 'pattern' and 'upstreams'.`)) + return undefined } - rejectUnknownKeys(`packages[${index}]`, value, ['pattern', 'upstreams']) + collectUnknownKeyIssues(onIssue, `packages[${index}]`, value, ['pattern', 'upstreams']) const { pattern, upstreams } = value + let knownPattern: PackageNamePattern | undefined if (typeof pattern !== 'string') { - throw new Error(`packages[${index}]: 'pattern' must be a string`) - } - - let parsed - try { - parsed = parsePackageNamePattern(pattern) - } catch (cause) { - throw new Error(`packages[${index}]: ${(cause as Error).message}`, { cause }) - } - - if (parsed.exclude) { - throw new Error( - `packages[${index}]: exclusion patterns ('!') are not supported in routing rules; ` - + `each rule stands alone, so there is nothing for an exclusion to subtract from`, - ) + onIssue(new Error(`packages[${index}]: 'pattern' must be a string.`)) + } else { + try { + const parsed = parsePackageNamePattern(pattern) + if (parsed.exclude) { + onIssue(new Error( + `packages[${index}]: exclusion patterns ('!') are not supported in routing rules; ` + + `each rule stands alone, so there is nothing for an exclusion to subtract from.`, + )) + } + knownPattern = parsed + } catch (cause) { + onIssue(new Error(`packages[${index}]: ${(cause as Error).message}`, { cause })) + } } if (!Array.isArray(upstreams) || upstreams.length === 0) { - throw new Error(`packages[${index}]: 'upstreams' must be a non-empty array of upstream names`) - } - - for (const name of upstreams) { - if (typeof name !== 'string' || !upstreamNames.includes(name)) { - throw new Error( - `packages[${index}]: upstream '${String(name)}' is not defined under 'upstreams' ` - + `(defined: ${upstreamNames.map(known => `'${known}'`).join(', ')})`, - ) + onIssue(new Error(`packages[${index}]: 'upstreams' must be a non-empty array of upstream names.`)) + } else { + for (const [entryIndex, name] of upstreams.entries()) { + if (typeof name !== 'string') { + // Unlike the existence check below, this does not depend on the + // 'upstreams' block, so it is never gated. + onIssue(new Error(`packages[${index}]: 'upstreams'[${entryIndex}] must be an upstream name.`)) + } else if (upstreamNames !== undefined && !upstreamNames.includes(name)) { + onIssue(new Error( + `packages[${index}]: upstream '${name}' is not defined under 'upstreams' ` + + `(defined: ${upstreamNames.map(known => `'${known}'`).join(', ')}).`, + )) + } } } - return value as unknown as PackageRoutingRule + return knownPattern } /**