Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions packages/@aws-cdk/toolkit/lib/actions/deploy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,6 @@ export interface BaseDeployOptions {
*/
readonly stacks?: StackSelector;

/**
* @deprecated set on toolkit
* Name of the toolkit stack to use/deploy
*
* @default CDKToolkit
*/
readonly toolkitStackName?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in favor of the same option on the Toolkit.


/**
* Role to pass to CloudFormation for deployment
*/
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/toolkit/lib/api/aws-cdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export { Deployments, type SuccessfulDeployStackResult } from '../../../../aws-c
export { Settings } from '../../../../aws-cdk/lib/api/settings';
export { tagsForStack } from '../../../../aws-cdk/lib/api/tags';
export { DEFAULT_TOOLKIT_STACK_NAME } from '../../../../aws-cdk/lib/api/toolkit-info';
export { ResourceMigrator } from '../../../../aws-cdk/lib/api/resource-import';

// Context Providers
export * as contextproviders from '../../../../aws-cdk/lib/context-providers';
Expand All @@ -19,7 +20,6 @@ export { formatTime } from '../../../../aws-cdk/lib/api/util/string-manipulation

// @todo Not yet API probably should be
export { formatErrorMessage } from '../../../../aws-cdk/lib/util/error';
export { ResourceMigrator } from '../../../../aws-cdk/lib/migrator';
export { obscureTemplate, serializeStructure } from '../../../../aws-cdk/lib/serialize';
export { loadTree, some } from '../../../../aws-cdk/lib/tree';
export { splitBySize } from '../../../../aws-cdk/lib/util';
Expand Down
6 changes: 5 additions & 1 deletion packages/@aws-cdk/toolkit/lib/api/io/private/codes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,15 @@ export const CODES = {
// 2: List
CDK_TOOLKIT_I2901: 'Provides details on the selected stacks and their dependencies',

// 3: Import & Migrate
CDK_TOOLKIT_E3900: 'Resource import failed',

// 4: Diff

// 5: Deploy
// 5: Deploy & Watch
CDK_TOOLKIT_I5000: 'Provides deployment times',
CDK_TOOLKIT_I5001: 'Provides total time in deploy action, including synth and rollback',
CDK_TOOLKIT_I5002: 'Provides time for resource migration',
CDK_TOOLKIT_I5031: 'Informs about any log groups that are traced as part of the deployment',
CDK_TOOLKIT_I5050: 'Confirm rollback during deployment',
CDK_TOOLKIT_I5060: 'Confirm deploy security sensitive changes',
Expand Down
37 changes: 20 additions & 17 deletions packages/@aws-cdk/toolkit/lib/toolkit/toolkit.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor refactoring to make the code more human readable

Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { CachedCloudAssemblySource, IdentityCloudAssemblySource, StackAssembly,
import { ALL_STACKS, CloudAssemblySourceBuilder } from '../api/cloud-assembly/private';
import { ToolkitError } from '../api/errors';
import { IIoHost, IoMessageCode, IoMessageLevel } from '../api/io';
import { asSdkLogger, withAction, Timer, confirm, error, highlight, info, success, warn, ActionAwareIoHost, debug, result, withoutEmojis, withoutColor, withTrimmedWhitespace } from '../api/io/private';
import { asSdkLogger, withAction, Timer, confirm, error, info, success, warn, ActionAwareIoHost, debug, result, withoutEmojis, withoutColor, withTrimmedWhitespace } from '../api/io/private';

/**
* The current action being performed by the CLI. 'none' represents the absence of an action.
Expand Down Expand Up @@ -256,11 +256,12 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
}

const deployments = await this.deploymentsForAction('deploy');
const migrator = new ResourceMigrator({ deployments, ioHost, action });

const migrator = new ResourceMigrator({
deployments,
await migrator.tryMigrateResources(stackCollection, {
toolkitStackName: this.toolkitStackName,
...options,
});
await migrator.tryMigrateResources(stackCollection, options);

const requireApproval = options.requireApproval ?? RequireApproval.NEVER;

Expand All @@ -275,7 +276,6 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
}

const stacks = stackCollection.stackArtifacts;

const stackOutputs: { [key: string]: any } = {};
const outputsFile = options.outputsFile;

Expand Down Expand Up @@ -303,28 +303,31 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
const deployStack = async (stackNode: StackNode) => {
const stack = stackNode.stack;
if (stackCollection.stackCount !== 1) {
await ioHost.notify(highlight(stack.displayName));
await ioHost.notify(info(chalk.bold(stack.displayName)));
}

if (!stack.environment) {
// eslint-disable-next-line max-len
throw new ToolkitError(
`Stack ${stack.displayName} does not define an environment, and AWS credentials could not be obtained from standard locations or no region was configured.`,
);
}

// The generated stack has no resources
if (Object.keys(stack.template.Resources || {}).length === 0) {
// The generated stack has no resources
if (!(await deployments.stackExists({ stack }))) {
await ioHost.notify(warn(`${chalk.bold(stack.displayName)}: stack has no resources, skipping deployment.`));
} else {
await ioHost.notify(warn(`${chalk.bold(stack.displayName)}: stack has no resources, deleting existing stack.`));
await this._destroy(assembly, 'deploy', {
stacks: { patterns: [stack.hierarchicalId], strategy: StackSelectionStrategy.PATTERN_MUST_MATCH_SINGLE },
roleArn: options.roleArn,
ci: options.ci,
});
// stack is empty and doesn't exist => do nothing
const stackExists = await deployments.stackExists({ stack });
if (!stackExists) {
return ioHost.notify(warn(`${chalk.bold(stack.displayName)}: stack has no resources, skipping deployment.`));
}

// stack is empty, but exists => delete
await ioHost.notify(warn(`${chalk.bold(stack.displayName)}: stack has no resources, deleting existing stack.`));
await this._destroy(assembly, 'deploy', {
stacks: { patterns: [stack.hierarchicalId], strategy: StackSelectionStrategy.PATTERN_MUST_MATCH_SINGLE },
roleArn: options.roleArn,
ci: options.ci,
});

return;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,71 @@
import { DeployOptions } from '@aws-cdk/cloud-assembly-schema';
import { format } from 'util';
import * as cfnDiff from '@aws-cdk/cloudformation-diff';
import { ResourceDifference } from '@aws-cdk/cloudformation-diff';
import * as cxapi from '@aws-cdk/cx-api';
import * as chalk from 'chalk';
import * as fs from 'fs-extra';
import * as promptly from 'promptly';
import { assertIsSuccessfulDeployStackResult, Deployments, DeploymentMethod, ResourceIdentifierProperties, ResourcesToImport } from './api/deployments';
import { Tag } from './api/tags';
import { StackActivityProgress } from './api/util/cloudformation/stack-activity-monitor';
import { error, info, success, warning } from './logging';
import { ToolkitError } from './toolkit/error';

export interface ImportDeploymentOptions extends DeployOptions {
deploymentMethod?: DeploymentMethod;
progress?: StackActivityProgress;
tags?: Tag[];
import { error, info, warn } from '../../cli/messages';
import { IIoHost, ToolkitAction } from '../../toolkit/cli-io-host';
import { ToolkitError } from '../../toolkit/error';
import { assertIsSuccessfulDeployStackResult, Deployments, DeploymentMethod, ResourceIdentifierProperties, ResourcesToImport } from '../deployments';
import { Tag } from '../tags';
import { StackActivityProgress } from '../util/cloudformation/stack-activity-monitor';

export interface ResourceImporterProps {
deployments: Deployments;
ioHost: IIoHost;
action: ToolkitAction;
}

export interface ImportDeploymentOptions {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this an explicit copy to limit the interface to a small subset of expected options.

/**
* Name of the toolkit stack to use/deploy
*
* @default CDKToolkit
*/
readonly toolkitStackName: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this where you're pointing to when you say "makes toolkitStackName a mandatory option"? outside of 'mandatory option' feeling like an oxymoron, i'm curious why that decision was made and what it means to have a mandatory option with a default.

when i first read 'mandatory option' i thought you meant that you were mandating toolkitStackName as an option for cli option bags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mandatory on the Toolkit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outside of 'mandatory option' feeling like an oxymoron

I guess it's more of a forced choice. You do have options, but you are mandated to pick one. I'm open to learn about better language choices though!


/**
* Role to pass to CloudFormation for deployment
*
* @default - Default stack role
*/
readonly roleArn?: string;

/**
* Deployment method
*/
readonly deploymentMethod?: DeploymentMethod;

/**
* Stack tags (pass through to CloudFormation)
*/
readonly tags?: Tag[];

/**
* Use previous values for unspecified parameters
*
* If not set, all parameters must be specified for every deployment.
*
* @default true
*/
readonly usePreviousParameters?: boolean;

/**
* Display mode for stack deployment progress.
*
* @default - StackActivityProgress.Bar - stack events will be displayed for
* the resource currently being deployed.
*/
readonly progress?: StackActivityProgress;

/**
* Rollback failed deployments
*
* @default true
*/
readonly rollback?: boolean;
}

/**
Expand Down Expand Up @@ -61,9 +112,20 @@
export class ResourceImporter {
private _currentTemplate: any;

private readonly stack: cxapi.CloudFormationStackArtifact;
private readonly cfn: Deployments;
private readonly ioHost: IIoHost;
private readonly action: ToolkitAction;

constructor(
private readonly stack: cxapi.CloudFormationStackArtifact,
private readonly cfn: Deployments) { }
stack: cxapi.CloudFormationStackArtifact,
props: ResourceImporterProps,
) {
this.stack = stack;
this.cfn = props.deployments;
this.ioHost = props.ioHost;
this.action = props.action;
}

/**
* Ask the user for resources to import
Expand Down Expand Up @@ -96,19 +158,19 @@
const descr = this.describeResource(resource.logicalId);
const idProps = contents[resource.logicalId];
if (idProps) {
info('%s: importing using %s', chalk.blue(descr), chalk.blue(fmtdict(idProps)));
await this.ioHost.notify(info(this.action, format('%s: importing using %s', chalk.blue(descr), chalk.blue(fmtdict(idProps)))));

Check warning on line 161 in packages/aws-cdk/lib/api/resource-import/importer.ts

View check run for this annotation

Codecov / codecov/patch

packages/aws-cdk/lib/api/resource-import/importer.ts#L161

Added line #L161 was not covered by tests

ret.importResources.push(resource);
ret.resourceMap[resource.logicalId] = idProps;
delete contents[resource.logicalId];
} else {
info('%s: skipping', chalk.blue(descr));
await this.ioHost.notify(info(this.action, format('%s: skipping', chalk.blue(descr))));

Check warning on line 167 in packages/aws-cdk/lib/api/resource-import/importer.ts

View check run for this annotation

Codecov / codecov/patch

packages/aws-cdk/lib/api/resource-import/importer.ts#L167

Added line #L167 was not covered by tests
}
}

const unknown = Object.keys(contents);
if (unknown.length > 0) {
warning(`Unrecognized resource identifiers in mapping file: ${unknown.join(', ')}`);
await this.ioHost.notify(warn(this.action, `Unrecognized resource identifiers in mapping file: ${unknown.join(', ')}`));

Check warning on line 173 in packages/aws-cdk/lib/api/resource-import/importer.ts

View check run for this annotation

Codecov / codecov/patch

packages/aws-cdk/lib/api/resource-import/importer.ts#L173

Added line #L173 was not covered by tests
}

return ret;
Expand Down Expand Up @@ -158,9 +220,9 @@
? ' ✅ %s (no changes)'
: ' ✅ %s';

success('\n' + message, this.stack.displayName);
await this.ioHost.notify(info(this.action, '\n' + chalk.green(format(message, this.stack.displayName))));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did we do away with the success helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting rid of all helpers with implicit formatting.

} catch (e) {
error('\n ❌ %s failed: %s', chalk.bold(this.stack.displayName), e);
await this.ioHost.notify(error(this.action, format('\n ❌ %s failed: %s', chalk.bold(this.stack.displayName), e), 'CDK_TOOLKIT_E3900'));

Check warning on line 225 in packages/aws-cdk/lib/api/resource-import/importer.ts

View check run for this annotation

Codecov / codecov/patch

packages/aws-cdk/lib/api/resource-import/importer.ts#L225

Added line #L225 was not covered by tests
throw e;
}
}
Expand Down Expand Up @@ -189,7 +251,7 @@
const offendingResources = nonAdditions.map(([logId, _]) => this.describeResource(logId));

if (allowNonAdditions) {
warning(`Ignoring updated/deleted resources (--force): ${offendingResources.join(', ')}`);
await this.ioHost.notify(warn(this.action, `Ignoring updated/deleted resources (--force): ${offendingResources.join(', ')}`));
} else {
throw new ToolkitError('No resource updates or deletes are allowed on import operation. Make sure to resolve pending changes ' +
`to existing resources, before attempting an import. Updated/deleted resources: ${offendingResources.join(', ')} (--force to override)`);
Expand Down Expand Up @@ -277,7 +339,7 @@
// Skip resources that do not support importing
const resourceType = chg.resourceDiff.newResourceType;
if (resourceType === undefined || !(resourceType in resourceIdentifiers)) {
warning(`${resourceName}: unsupported resource type ${resourceType}, skipping import.`);
await this.ioHost.notify(warn(this.action, `${resourceName}: unsupported resource type ${resourceType}, skipping import.`));

Check warning on line 342 in packages/aws-cdk/lib/api/resource-import/importer.ts

View check run for this annotation

Codecov / codecov/patch

packages/aws-cdk/lib/api/resource-import/importer.ts#L342

Added line #L342 was not covered by tests
return undefined;
}

Expand All @@ -303,7 +365,7 @@

// If we got here and the user rejected any available identifiers, then apparently they don't want the resource at all
if (satisfiedPropSets.length > 0) {
info(chalk.grey(`Skipping import of ${resourceName}`));
await this.ioHost.notify(info(this.action, chalk.grey(`Skipping import of ${resourceName}`)));

Check warning on line 368 in packages/aws-cdk/lib/api/resource-import/importer.ts

View check run for this annotation

Codecov / codecov/patch

packages/aws-cdk/lib/api/resource-import/importer.ts#L368

Added line #L368 was not covered by tests
return undefined;
}

Expand All @@ -321,7 +383,7 @@

// Do the input loop here
if (preamble) {
info(preamble);
await this.ioHost.notify(info(this.action, preamble));
}
for (const idProps of idPropSets) {
const input: Record<string, string> = {};
Expand Down Expand Up @@ -356,7 +418,7 @@
}
}

info(chalk.grey(`Skipping import of ${resourceName}`));
await this.ioHost.notify(info(this.action, chalk.grey(`Skipping import of ${resourceName}`)));
return undefined;
}

Expand Down
2 changes: 2 additions & 0 deletions packages/aws-cdk/lib/api/resource-import/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './importer';
export * from './migrator';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort of a meta question -- is there a way to block us from importing directly from the file itself when we have an index.ts?

i want to disawllow import * as blah from 'aws-cdk/lib/api/resource-inport/importer'; in favor of import * as blah from 'aws-cdk/lib/api/resource-import'; in this case, so that we don't have multiple ways of doing the same thing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could write an eslint rule for it, might need a plugin. There can be good reasons to import directly though. My current concern is that anything outside of api should only import form the barrel files. Within api I don't mind so much.

Loading