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.

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
2 changes: 2 additions & 0 deletions packages/aws-cdk/bin/cdk.ts
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 (--no-execute will NOT execute the ChangeSet)', default: true})
Copy link
Contributor

Choose a reason for hiding this comment

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

This switch is not defined only for deploy (despite the indentation). See that the closing ) do not match?

.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 @@ -207,6 +208,7 @@ async function initCommandLine() {
reuseAssets: args['build-exclude'],
tags: configuration.settings.get(['tags']),
sdk: aws,
execute: args.execute
});

case 'destroy':
Expand Down
24 changes: 16 additions & 8 deletions packages/aws-cdk/lib/api/deploy-stack.ts
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 @@ -92,14 +93,21 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
return { noOp: true, outputs: await getStackOutputs(cfn, deployName), stackArn: changeSet.StackId! };
}

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.

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);
} else {
debug('Entering no-execute workflow for ChangeSet %s on stack %s', changeSetName, deployName);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be printed to the user so they don't think the operation is complete.

cfn.executeChangeSet();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we calling executeChangeSet here?

}
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
4 changes: 3 additions & 1 deletion packages/aws-cdk/lib/api/deployment-target.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface DeployStackOptions {
toolkitStackName?: string;
reuseAssets?: string[];
tags?: Tag[];
execute?: boolean;
}

export interface ProvisionerProps {
Expand Down Expand Up @@ -75,7 +76,8 @@ export class CloudFormationDeploymentTarget implements IDeploymentTarget {
ci: options.ci,
reuseAssets: options.reuseAssets,
toolkitInfo,
tags: options.tags
tags: options.tags,
execute: options.execute
});
}
}
10 changes: 9 additions & 1 deletion packages/aws-cdk/lib/cdk-toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ export class CdkToolkit {
toolkitStackName: options.toolkitStackName,
reuseAssets: options.reuseAssets,
notificationArns: options.notificationArns,
tags
tags,
execute: options.execute
});

const message = result.noOp
Expand Down Expand Up @@ -303,6 +304,13 @@ export interface DeployOptions {
* AWS SDK
*/
sdk: ISDK;

/**
* Whether to execute the ChangeSet
* Not providing `execute` parameter will result in execution of ChangeSet
* @default true
*/
execute?: boolean;
}

export interface DestroyOptions {
Expand Down
27 changes: 27 additions & 0 deletions packages/aws-cdk/test/integ/cli/test-cdk-deploy-no-execute.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#!/bin/bash
set -euo pipefail
scriptdir=$(cd $(dirname $0) && pwd)
source ${scriptdir}/common.bash
# ----------------------------------------------------------

setup

stack_arn=$(cdk deploy -v ${STACK_NAME_PREFIX}-test-2 --no-execute)
echo "Stack deployed successfully"

# verify that we only deployed a single stack (there's a single ARN in the output)
assert_lines "${stack_arn}" 1

# verify the number of resources in the stack
response_json=$(mktemp).json
aws cloudformation describe-stacks --stack-name ${stack_arn} > ${response_json}

stack_status=$(node -e "console.log(require('${response_json}').Stacks[0].StackStatus)")
if [ "${stack_status}" -ne "REVIEW_IN_PROGRESS" ]; then
fail "Expected stack to be in status REVIEW_IN_PROGRESS but got ${stack_status}"
fi

# destroy
cdk destroy -f ${STACK_NAME_PREFIX}-test-2

echo "✅ success"