Skip to content

Commit

Permalink
feat(cli): throw typed errors (#33005)
Browse files Browse the repository at this point in the history
### Reason for this change

In #32548 we enabled typed errors for some places in the CLI.
However many places were missed and the eslint rule wasn't enabled to
enforce it in future.

### Description of changes

Enforce by enabling the respective eslint rule.
Also adds and implements the eslint rule in the toolkit.

This has little functional effect since all new errors are still
`Error`s. The printed output of an error will slightly change.

### Describe any new or updated permissions being added

n/a

### Description of how you validated changes

existing tests

### Checklist
- [x] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license*
  • Loading branch information
mrgrain authored Jan 20, 2025
1 parent 53dc0d8 commit bf81b3c
Show file tree
Hide file tree
Showing 43 changed files with 218 additions and 135 deletions.
10 changes: 10 additions & 0 deletions packages/@aws-cdk/toolkit/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
const baseConfig = require('@aws-cdk/cdk-build-tools/config/eslintrc');
baseConfig.parserOptions.project = __dirname + '/tsconfig.json';

// custom rules
baseConfig.rules['@cdklabs/no-throw-default-error'] = ['error'];
baseConfig.overrides.push({
files: ["./test/**"],
rules: {
"@cdklabs/no-throw-default-error": "off",
},
});

module.exports = baseConfig;
4 changes: 2 additions & 2 deletions packages/@aws-cdk/toolkit/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ module.exports = {
coverageThreshold: {
global: {
// this is very sad but we will get better
branches: 27,
statements: 53,
branches: 42,
statements: 69,
},
},
};
6 changes: 6 additions & 0 deletions packages/@aws-cdk/toolkit/lib/toolkit/toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
const assembly = await this.assemblyFromSource(cx);
ioHost;
assembly;
// temporary
// eslint-disable-next-line @cdklabs/no-throw-default-error
throw new Error('Not implemented yet');
}

Expand All @@ -189,6 +191,8 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
const assembly = await this.assemblyFromSource(cx);
const stacks = await assembly.selectStacksV2(options.stacks);
await this.validateStacksMetadata(stacks, ioHost);
// temporary
// eslint-disable-next-line @cdklabs/no-throw-default-error
throw new Error('Not implemented yet');
}

Expand Down Expand Up @@ -619,6 +623,8 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
time: synthTime.asMs,
}));

// temporary
// eslint-disable-next-line @cdklabs/no-throw-default-error
throw new Error('Not implemented yet');
}

Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/toolkit/test/_helpers/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as fs from 'node:fs';
import * as path from 'node:path';
import { Toolkit } from '../../lib';
import { Toolkit, ToolkitError } from '../../lib';
import { determineOutputDirectory } from '../../lib/api/cloud-assembly/private';

export * from './test-io-host';
Expand All @@ -12,7 +12,7 @@ function fixturePath(...parts: string[]): string {
export async function appFixture(toolkit: Toolkit, name: string, context?: { [key: string]: any }) {
const appPath = fixturePath(name, 'app.js');
if (!fs.existsSync(appPath)) {
throw new Error(`App Fixture ${name} does not exist in ${appPath}`);
throw new ToolkitError(`App Fixture ${name} does not exist in ${appPath}`);
}
const app = `cat ${appPath} | node --input-type=module`;
return toolkit.fromCdkApp(app, {
Expand All @@ -33,7 +33,7 @@ export function builderFixture(toolkit: Toolkit, name: string, context?: { [key:
export function cdkOutFixture(toolkit: Toolkit, name: string) {
const outdir = path.join(__dirname, '..', '_fixtures', name, 'cdk.out');
if (!fs.existsSync(outdir)) {
throw new Error(`Assembly Dir Fixture ${name} does not exist in ${outdir}`);
throw new ToolkitError(`Assembly Dir Fixture ${name} does not exist in ${outdir}`);
}
return toolkit.fromAssemblyDirectory(outdir);
}
18 changes: 14 additions & 4 deletions packages/aws-cdk/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
const baseConfig = require('@aws-cdk/cdk-build-tools/config/eslintrc');
baseConfig.ignorePatterns.push('lib/init-templates/**/typescript/**/*.ts');
baseConfig.ignorePatterns.push('test/integ/cli/sam_cdk_integ_app/**/*.ts');
baseConfig.parserOptions.project = __dirname + '/tsconfig.json';
const baseConfig = require("@aws-cdk/cdk-build-tools/config/eslintrc");
baseConfig.ignorePatterns.push("lib/init-templates/**/typescript/**/*.ts");
baseConfig.ignorePatterns.push("test/integ/cli/sam_cdk_integ_app/**/*.ts");
baseConfig.parserOptions.project = __dirname + "/tsconfig.json";

// custom rules
baseConfig.rules["@cdklabs/no-throw-default-error"] = ["error"];
baseConfig.overrides.push({
files: ["./test/**"],
rules: {
"@cdklabs/no-throw-default-error": "off",
},
});

module.exports = baseConfig;
15 changes: 8 additions & 7 deletions packages/aws-cdk/lib/api/deploy-stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { AssetManifestBuilder } from '../util/asset-manifest-builder';
import { determineAllowCrossAccountAssetPublishing } from './util/checks';
import { publishAssets } from '../util/asset-publishing';
import { StringWithoutPlaceholders } from './util/placeholders';
import { ToolkitError } from '../toolkit/error';
import { formatErrorMessage } from '../util/error';

export type DeployStackResult =
Expand Down Expand Up @@ -63,7 +64,7 @@ export interface ReplacementRequiresRollbackStackResult {

export function assertIsSuccessfulDeployStackResult(x: DeployStackResult): asserts x is SuccessfulDeployStackResult {
if (x.type !== 'did-deploy-stack') {
throw new Error(`Unexpected deployStack result. This should not happen: ${JSON.stringify(x)}. If you are seeing this error, please report it at https://github.com/aws/aws-cdk/issues/new/choose.`);
throw new ToolkitError(`Unexpected deployStack result. This should not happen: ${JSON.stringify(x)}. If you are seeing this error, please report it at https://github.com/aws/aws-cdk/issues/new/choose.`);
}
}

Expand Down Expand Up @@ -297,7 +298,7 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
await cfn.deleteStack({ StackName: deployName });
const deletedStack = await waitForStackDelete(cfn, deployName);
if (deletedStack && deletedStack.stackStatus.name !== 'DELETE_COMPLETE') {
throw new Error(
throw new ToolkitError(
`Failed deleting stack ${deployName} that had previously failed creation (current state: ${deletedStack.stackStatus})`,
);
}
Expand Down Expand Up @@ -455,7 +456,7 @@ class FullCloudFormationDeployment {
};

if (deploymentMethod.method === 'direct' && this.options.resourcesToImport) {
throw new Error('Importing resources requires a changeset deployment');
throw new ToolkitError('Importing resources requires a changeset deployment');
}

switch (deploymentMethod.method) {
Expand Down Expand Up @@ -669,11 +670,11 @@ class FullCloudFormationDeployment {

// This shouldn't really happen, but catch it anyway. You never know.
if (!successStack) {
throw new Error('Stack deploy failed (the stack disappeared while we were deploying it)');
throw new ToolkitError('Stack deploy failed (the stack disappeared while we were deploying it)');
}
finalState = successStack;
} catch (e: any) {
throw new Error(suffixWithErrors(formatErrorMessage(e), monitor?.errors));
throw new ToolkitError(suffixWithErrors(formatErrorMessage(e), monitor?.errors));
} finally {
await monitor?.stop();
}
Expand Down Expand Up @@ -748,10 +749,10 @@ export async function destroyStack(options: DestroyStackOptions) {
await cfn.deleteStack({ StackName: deployName, RoleARN: options.roleArn });
const destroyedStack = await waitForStackDelete(cfn, deployName);
if (destroyedStack && destroyedStack.stackStatus.name !== 'DELETE_COMPLETE') {
throw new Error(`Failed to destroy ${deployName}: ${destroyedStack.stackStatus}`);
throw new ToolkitError(`Failed to destroy ${deployName}: ${destroyedStack.stackStatus}`);
}
} catch (e: any) {
throw new Error(suffixWithErrors(formatErrorMessage(e), monitor?.errors));
throw new ToolkitError(suffixWithErrors(formatErrorMessage(e), monitor?.errors));
} finally {
if (monitor) {
await monitor.stop();
Expand Down
19 changes: 10 additions & 9 deletions packages/aws-cdk/lib/api/deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
Template,
uploadStackTemplateAssets,
} from './util/cloudformation';
import { ToolkitError } from '../toolkit/error';
import { AssetManifestBuilder } from '../util/asset-manifest-builder';
import {
buildAssets,
Expand Down Expand Up @@ -439,7 +440,7 @@ export class Deployments {
let deploymentMethod = options.deploymentMethod;
if (options.changeSetName || options.execute !== undefined) {
if (deploymentMethod) {
throw new Error(
throw new ToolkitError(
"You cannot supply both 'deploymentMethod' and 'changeSetName/execute'. Supply one or the other.",
);
}
Expand Down Expand Up @@ -492,7 +493,7 @@ export class Deployments {
public async rollbackStack(options: RollbackStackOptions): Promise<RollbackStackResult> {
let resourcesToSkip: string[] = options.orphanLogicalIds ?? [];
if (options.force && resourcesToSkip.length > 0) {
throw new Error('Cannot combine --force with --orphan');
throw new ToolkitError('Cannot combine --force with --orphan');
}

const env = await this.envs.accessStackForMutableStackOperations(options.stack);
Expand Down Expand Up @@ -564,7 +565,7 @@ export class Deployments {
return { notInRollbackableState: true };

default:
throw new Error(`Unexpected rollback choice: ${cloudFormationStack.stackStatus.rollbackChoice}`);
throw new ToolkitError(`Unexpected rollback choice: ${cloudFormationStack.stackStatus.rollbackChoice}`);
}

const monitor = options.quiet
Expand All @@ -580,7 +581,7 @@ export class Deployments {

// This shouldn't really happen, but catch it anyway. You never know.
if (!successStack) {
throw new Error('Stack deploy failed (the stack disappeared while we were rolling it back)');
throw new ToolkitError('Stack deploy failed (the stack disappeared while we were rolling it back)');
}
finalStackState = successStack;

Expand All @@ -604,11 +605,11 @@ export class Deployments {
continue;
}

throw new Error(
throw new ToolkitError(
`${stackErrorMessage} (fix problem and retry, or orphan these resources using --orphan or --force)`,
);
}
throw new Error(
throw new ToolkitError(
"Rollback did not finish after a large number of iterations; stopping because it looks like we're not making progress anymore. You can retry if rollback was progressing as expected.",
);
}
Expand Down Expand Up @@ -698,7 +699,7 @@ export class Deployments {
const publisher = this.cachedPublisher(assetManifest, resolvedEnvironment, options.stackName);
await publisher.buildEntry(asset);
if (publisher.hasFailures) {
throw new Error(`Failed to build asset ${asset.id}`);
throw new ToolkitError(`Failed to build asset ${asset.id}`);
}
}

Expand All @@ -717,7 +718,7 @@ export class Deployments {
const publisher = this.cachedPublisher(assetManifest, stackEnv, options.stackName);
await publisher.publishEntry(asset, { allowCrossAccount: await this.allowCrossAccountAssetPublishingForEnv(options.stack) });
if (publisher.hasFailures) {
throw new Error(`Failed to publish asset ${asset.id}`);
throw new ToolkitError(`Failed to publish asset ${asset.id}`);
}
}

Expand Down Expand Up @@ -756,7 +757,7 @@ export class Deployments {
try {
await envResources.validateVersion(requiresBootstrapStackVersion, bootstrapStackVersionSsmParameter);
} catch (e: any) {
throw new Error(`${stackName}: ${formatErrorMessage(e)}`);
throw new ToolkitError(`${stackName}: ${formatErrorMessage(e)}`);
}
}

Expand Down
9 changes: 5 additions & 4 deletions packages/aws-cdk/lib/api/environment-access.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { CredentialsOptions, SdkForEnvironment, SdkProvider } from './aws-auth/s
import { EnvironmentResources, EnvironmentResourcesRegistry } from './environment-resources';
import { Mode } from './plugin/mode';
import { replaceEnvPlaceholders, StringWithoutPlaceholders } from './util/placeholders';
import { ToolkitError } from '../toolkit/error';
import { formatErrorMessage } from '../util/error';

/**
Expand Down Expand Up @@ -87,7 +88,7 @@ export class EnvironmentAccess {
*/
public async accessStackForLookup(stack: cxapi.CloudFormationStackArtifact): Promise<TargetEnvironment> {
if (!stack.environment) {
throw new Error(`The stack ${stack.displayName} does not have an environment`);
throw new ToolkitError(`The stack ${stack.displayName} does not have an environment`);
}

const lookupEnv = await this.prepareSdk({
Expand All @@ -102,7 +103,7 @@ export class EnvironmentAccess {
if (lookupEnv.didAssumeRole && stack.lookupRole?.bootstrapStackVersionSsmParameter && stack.lookupRole.requiresBootstrapStackVersion) {
const version = await lookupEnv.resources.versionFromSsmParameter(stack.lookupRole.bootstrapStackVersionSsmParameter);
if (version < stack.lookupRole.requiresBootstrapStackVersion) {
throw new Error(`Bootstrap stack version '${stack.lookupRole.requiresBootstrapStackVersion}' is required, found version '${version}'. To get rid of this error, please upgrade to bootstrap version >= ${stack.lookupRole.requiresBootstrapStackVersion}`);
throw new ToolkitError(`Bootstrap stack version '${stack.lookupRole.requiresBootstrapStackVersion}' is required, found version '${version}'. To get rid of this error, please upgrade to bootstrap version >= ${stack.lookupRole.requiresBootstrapStackVersion}`);
}
}
if (lookupEnv.isFallbackCredentials) {
Expand All @@ -125,7 +126,7 @@ export class EnvironmentAccess {
*/
public async accessStackForLookupBestEffort(stack: cxapi.CloudFormationStackArtifact): Promise<TargetEnvironment> {
if (!stack.environment) {
throw new Error(`The stack ${stack.displayName} does not have an environment`);
throw new ToolkitError(`The stack ${stack.displayName} does not have an environment`);
}

try {
Expand All @@ -147,7 +148,7 @@ export class EnvironmentAccess {
*/
private async accessStackForStackOperations(stack: cxapi.CloudFormationStackArtifact, mode: Mode): Promise<TargetEnvironment> {
if (!stack.environment) {
throw new Error(`The stack ${stack.displayName} does not have an environment`);
throw new ToolkitError(`The stack ${stack.displayName} does not have an environment`);
}

return this.prepareSdk({
Expand Down
15 changes: 8 additions & 7 deletions packages/aws-cdk/lib/api/environment-resources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { SDK } from './aws-auth';
import { type EcrRepositoryInfo, ToolkitInfo } from './toolkit-info';
import { debug, warning } from '../logging';
import { Notices } from '../notices';
import { ToolkitError } from '../toolkit/error';
import { formatErrorMessage } from '../util/error';

/**
Expand Down Expand Up @@ -101,7 +102,7 @@ export class EnvironmentResources {
return;
}

throw new Error(
throw new ToolkitError(
`This CDK deployment requires bootstrap stack version '${expectedVersion}', but during the confirmation via SSM parameter ${ssmParameterName} the following error occurred: ${e}`,
);
}
Expand All @@ -119,7 +120,7 @@ export class EnvironmentResources {
notices.addBootstrappedEnvironment({ bootstrapStackVersion: version, environment });
}
if (defExpectedVersion > version) {
throw new Error(
throw new ToolkitError(
`This CDK deployment requires bootstrap stack version '${expectedVersion}', found '${version}'. Please run 'cdk bootstrap'.`,
);
}
Expand All @@ -142,14 +143,14 @@ export class EnvironmentResources {

const asNumber = parseInt(`${result.Parameter?.Value}`, 10);
if (isNaN(asNumber)) {
throw new Error(`SSM parameter ${parameterName} not a number: ${result.Parameter?.Value}`);
throw new ToolkitError(`SSM parameter ${parameterName} not a number: ${result.Parameter?.Value}`);
}

this.cache.ssmParameters.set(parameterName, asNumber);
return asNumber;
} catch (e: any) {
if (e.name === 'ParameterNotFound') {
throw new Error(
throw new ToolkitError(
`SSM parameter ${parameterName} not found. Has the environment been bootstrapped? Please run \'cdk bootstrap\' (see https://docs.aws.amazon.com/cdk/latest/guide/bootstrapping.html)`,
);
}
Expand All @@ -159,7 +160,7 @@ export class EnvironmentResources {

public async prepareEcrRepository(repositoryName: string): Promise<EcrRepositoryInfo> {
if (!this.sdk) {
throw new Error('ToolkitInfo needs to have been initialized with an sdk to call prepareEcrRepository');
throw new ToolkitError('ToolkitInfo needs to have been initialized with an sdk to call prepareEcrRepository');
}
const ecr = this.sdk.ecr();

Expand Down Expand Up @@ -188,7 +189,7 @@ export class EnvironmentResources {
});
const repositoryUri = response.repository?.repositoryUri;
if (!repositoryUri) {
throw new Error(`CreateRepository did not return a repository URI for ${repositoryUri}`);
throw new ToolkitError(`CreateRepository did not return a repository URI for ${repositoryUri}`);
}

// configure image scanning on push (helps in identifying software vulnerabilities, no additional charge)
Expand All @@ -211,7 +212,7 @@ export class NoBootstrapStackEnvironmentResources extends EnvironmentResources {
* Look up the toolkit for a given environment, using a given SDK
*/
public async lookupToolkit(): Promise<ToolkitInfo> {
throw new Error(
throw new ToolkitError(
'Trying to perform an operation that requires a bootstrap stack; you should not see this error, this is a bug in the CDK CLI.',
);
}
Expand Down
3 changes: 2 additions & 1 deletion packages/aws-cdk/lib/api/evaluate-cloudformation-template.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { Export, ListExportsCommandOutput, StackResourceSummary } from '@aws-sdk/client-cloudformation';
import type { SDK } from './aws-auth';
import type { NestedStackTemplates } from './nested-stack-helpers';
import { ToolkitError } from '../toolkit/error';

export interface ListStackResources {
listStackResources(): Promise<StackResourceSummary[]>;
Expand Down Expand Up @@ -556,7 +557,7 @@ interface Intrinsic {

async function asyncGlobalReplace(str: string, regex: RegExp, cb: (x: string) => Promise<string>): Promise<string> {
if (!regex.global) {
throw new Error('Regex must be created with /g flag');
throw new ToolkitError('Regex must be created with /g flag');
}

const ret = new Array<string>();
Expand Down
3 changes: 2 additions & 1 deletion packages/aws-cdk/lib/api/hotswap-deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { isHotswappableStateMachineChange } from './hotswap/stepfunctions-state-
import { NestedStackTemplates, loadCurrentTemplateWithNestedStacks } from './nested-stack-helpers';
import { Mode } from './plugin/mode';
import { CloudFormationStack } from './util/cloudformation';
import { ToolkitError } from '../toolkit/error';
import { formatErrorMessage } from '../util/error';

// Must use a require() otherwise esbuild complains about calling a namespace
Expand Down Expand Up @@ -425,7 +426,7 @@ async function applyHotswappableChange(sdk: SDK, hotswapOperation: HotswappableC
} catch (e: any) {
if (e.name === 'TimeoutError' || e.name === 'AbortError') {
const result: WaiterResult = JSON.parse(formatErrorMessage(e));
const error = new Error([
const error = new ToolkitError([
`Resource is not in the expected state due to waiter status: ${result.state}`,
result.reason ? `${result.reason}.` : '',
].join('. '));
Expand Down
Loading

0 comments on commit bf81b3c

Please sign in to comment.