Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): adding new option to cdk deploy to indicate whether ChangeSet should be executed #4852

Merged
merged 10 commits into from
Nov 13, 2019
Merged

feat(cli): adding new option to cdk deploy to indicate whether ChangeSet should be executed #4852

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d275539
feat(aws-cdk): adding new option to `cdk deploy`
Nov 5, 2019
e2a7003
feat(cli): adding new option to `cdk deploy` to indicate whether Chan…
Nov 5, 2019
d8fc781
Merge remote-tracking branch 'origin/master'
Nov 8, 2019
acd543e
feat(cli): adding new option to `cdk deploy` to indicate whether Chan…
Nov 8, 2019
0d8ab27
feat(cli): adding new option to `cdk deploy` to indicate whether Chan…
Nov 12, 2019
3e9ebdc
Merge pull request #1 from aws/master
robertma96 Nov 12, 2019
cfc0486
feat(cli): adding new option to `cdk deploy` to indicate whether Chan…
Nov 12, 2019
f31d1cd
feat(cli): adding new option to `cdk deploy` to indicate whether Chan…
Nov 12, 2019
bd9caa9
Merge branch 'master' into master
mergify[bot] Nov 13, 2019
6399716
Merge branch 'master' into master
mergify[bot] Nov 13, 2019
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
2 changes: 2 additions & 0 deletions packages/aws-cdk/bin/cdk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ async function parseCommandLineArguments() {
.option('ci', { type: 'boolean', desc: 'Force CI detection. Use --no-ci to disable CI autodetection.', default: process.env.CI !== undefined })
.option('notification-arns', {type: 'array', desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events', nargs: 1, requiresArg: true})
.option('tags', { type: 'array', alias: 't', desc: 'Tags to add to the stack (KEY=VALUE)', nargs: 1, requiresArg: true }))
.option('execute', {type: 'boolean', desc: 'Whether to execute ChangeSet or not (--no-execute will NOT execute the ChangeSet)', default: true})
.command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs
.option('exclusively', { type: 'boolean', alias: 'e', desc: 'Only deploy requested stacks, don\'t include dependees' })
.option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' }))
Expand Down Expand Up @@ -205,6 +206,7 @@ async function initCommandLine() {
reuseAssets: args['build-exclude'],
tags: configuration.settings.get(['tags']),
sdk: aws,
execute: args.execute
});

case 'destroy':
Expand Down
19 changes: 12 additions & 7 deletions packages/aws-cdk/lib/api/deploy-stack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export interface DeployStackOptions {
ci?: boolean;
reuseAssets?: string[];
tags?: Tag[];
execute?: boolean;
}

const LARGE_TEMPLATE_SIZE_KB = 50;
Expand Down Expand Up @@ -93,13 +94,17 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
}

debug('Initiating execution of changeset %s on stack %s', changeSetName, deployName);
await cfn.executeChangeSet({ StackName: deployName, ChangeSetName: changeSetName }).promise();
// tslint:disable-next-line:max-line-length
const monitor = options.quiet ? undefined : new StackActivityMonitor(cfn, deployName, options.stack, (changeSetDescription.Changes || []).length).start();
debug('Execution of changeset %s on stack %s has started; waiting for the update to complete...', changeSetName, deployName);
await waitForStack(cfn, deployName);
if (monitor) { await monitor.stop(); }
debug('Stack %s has completed updating', deployName);
if (options.execute) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not respect the default of "true" (and caused a regression). If options.execute is undefined, it evaluates to false.

await cfn.executeChangeSet({StackName: deployName, ChangeSetName: changeSetName}).promise();
// tslint:disable-next-line:max-line-length
const monitor = options.quiet ? undefined : new StackActivityMonitor(cfn, deployName, options.stack, (changeSetDescription.Changes || []).length).start();
debug('Execution of changeset %s on stack %s has started; waiting for the update to complete...', changeSetName, deployName);
await waitForStack(cfn, deployName);
if (monitor) {
await monitor.stop();
}
debug('Stack %s has completed updating', deployName);
}
return { noOp: false, outputs: await getStackOutputs(cfn, deployName), stackArn: changeSet.StackId! };
Copy link
Contributor

Choose a reason for hiding this comment

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

clarification: just verifying that this would not count as a noOp as we are still generating a ChangeSet and mutating the state of the stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified this portion of code but it doesn't show as outdated.
I tried to not interfere with the testing scripts for as much as I could. The executeChangeSet method is needed to be called in deploy-stacks.ts in order for some tests to pass. I think calling cfn.executeChangeSet() without the promise class. This way the tests will pass successfully without modifying them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good call not to modify existing tests so we don't mask a potential regression.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree. The fact that tests fail is an indication of a regression, and indeed we've had a regression here where bootstrap stacks failed to execute because "execute" was not passed to it.

}

Expand Down
5 changes: 5 additions & 0 deletions packages/aws-cdk/lib/cdk-toolkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,11 @@ export interface DeployOptions {
* AWS SDK
*/
sdk: ISDK;

/**
* Whether to execute or not the ChangeSet
*/
execute?: boolean;
}

export interface DestroyOptions {
Expand Down