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

robertma96 · 2019-11-05T10:13:18Z

I created the option to NOT execute the ChangeSet via cdk deploy.

The flag is called execute and by default is set to true. By not providing this flag, the workflow of cdk deploy will be the same.

If anyone wants to NOT execute the ChangeSet, providing the flag --no-execute will pass the execution of the ChangeSet.

You will be able to see the ChangeSet in AWS CloudFormation Console, validate the resources and discard or execute the ChangeSet.

closes #4739

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

I created the option to *NOT* execute the ChangeSet via `cdk deploy`. The flag is called **execute** and by default is set to true. By not providing this flag, the workflow of `cdk deploy` will be the same. If anyone wants to *NOT* execute the ChangeSet, providing the flag `--no-execute` will pass the execution of the ChangeSet. You will be able to see the ChangeSet in AWS CloudFormation Console, validate the resources and discard or execute the ChangeSet. closes aws#4739

mergify · 2019-11-05T10:13:59Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-05T10:15:45Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

shivlaks

Please also add some tests - integration tests would also be ideal, but ensure you run the CLI integration tests at a minimum.

packages/aws-cdk/lib/cdk-toolkit.ts

shivlaks · 2019-11-08T00:18:53Z

packages/aws-cdk/lib/api/deploy-stack.ts

+      await monitor.stop();
+    }
+    debug('Stack %s has completed updating', deployName);
+  }
  return { noOp: false, outputs: await getStackOutputs(cfn, deployName), stackArn: changeSet.StackId! };


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.

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.

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

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.

…geSet should be executed I created the option to *NOT* execute the ChangeSet via `cdk deploy`. The flag is called **execute** and by default is set to true. By not providing this flag, the workflow of `cdk deploy` will be the same. If anyone wants to *NOT* execute the ChangeSet, providing the flag `--no-execute` will pass the execution of the ChangeSet. You will be able to see the ChangeSet in AWS CloudFormation Console, validate the resources and discard or execute the ChangeSet. closes aws#4739

# Conflicts: # packages/aws-cdk/bin/cdk.ts # packages/aws-cdk/lib/api/deploy-stack.ts # packages/aws-cdk/lib/cdk-toolkit.ts

Pull request has been modified.

aws-cdk-automation · 2019-11-08T15:27:45Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…geSet should be executed I created the option to *NOT* execute the ChangeSet via `cdk deploy`. The flag is called **execute** and by default is set to true. By not providing this flag, the workflow of `cdk deploy` will be the same. If anyone wants to *NOT* execute the ChangeSet, providing the flag `--no-execute` will pass the execution of the ChangeSet. You will be able to see the ChangeSet in AWS CloudFormation Console, validate the resources and discard or execute the ChangeSet. closes aws#4739

robertma96 · 2019-11-08T15:59:51Z

New CLI integration test for testing the flag added.

aws-cdk-automation · 2019-11-08T16:01:04Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/aws-cdk/test/integ/cli/test-cdk-deploy-no-execute.sh

shivlaks · 2019-11-11T21:15:13Z

packages/aws-cdk/lib/api/deploy-stack.ts

+      await monitor.stop();
+    }
+    debug('Stack %s has completed updating', deployName);
+  }
  return { noOp: false, outputs: await getStackOutputs(cfn, deployName), stackArn: changeSet.StackId! };


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

packages/aws-cdk/lib/api/deploy-stack.ts

…geSet should be executed I created the option to *NOT* execute the ChangeSet via `cdk deploy`. The flag is called **execute** and by default is set to true. By not providing this flag, the workflow of `cdk deploy` will be the same. If anyone wants to *NOT* execute the ChangeSet, providing the flag `--no-execute` will pass the execution of the ChangeSet. You will be able to see the ChangeSet in AWS CloudFormation Console, validate the resources and discard or execute the ChangeSet. closes aws#4739

aws-cdk-automation · 2019-11-12T08:14:55Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

SYNC

aws-cdk-automation · 2019-11-12T09:14:08Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…geSet should be executed I created the option to *NOT* execute the ChangeSet via `cdk deploy`. The flag is called **execute** and by default is set to true. By not providing this flag, the workflow of `cdk deploy` will be the same. If anyone wants to *NOT* execute the ChangeSet, providing the flag `--no-execute` will pass the execution of the ChangeSet. You will be able to see the ChangeSet in AWS CloudFormation Console, validate the resources and discard or execute the ChangeSet. closes aws#4739

aws-cdk-automation · 2019-11-12T14:16:08Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…geSet should be executed I created the option to *NOT* execute the ChangeSet via `cdk deploy`. The flag is called **execute** and by default is set to true. By not providing this flag, the workflow of `cdk deploy` will be the same. If anyone wants to *NOT* execute the ChangeSet, providing the flag `--no-execute` will pass the execution of the ChangeSet. You will be able to see the ChangeSet in AWS CloudFormation Console, validate the resources and discard or execute the ChangeSet. closes aws#4739

aws-cdk-automation · 2019-11-12T16:31:42Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2019-11-13T11:24:33Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-11-13T11:52:40Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-13T11:57:03Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…nto 4295-windows-ecs-support * '4295-windows-ecs-support' of github.com:arhea/aws-cdk: chore(deps-dev): bump @types/lodash from 4.14.146 to 4.14.147 (aws#5021) Revert "fix(assets): support exceptions to exclude patterns (aws#4473)" (aws#5022) chore(deps): bump jsii-pacmak from 0.20.3 to 0.20.5 (aws#5003) chore(deps): bump codemaker from 0.20.3 to 0.20.5 (aws#5007) chore(deps-dev): bump @types/jest from 24.0.22 to 24.0.23 (aws#4993) chore(deps): bump jsii from 0.20.3 to 0.20.5 (aws#5006) chore(deps-dev): bump jsii-diff from 0.20.3 to 0.20.5 (aws#5005) chore(deps): bump jsii-spec from 0.20.3 to 0.20.5 (aws#5008) chore(core): resolve tokens before publishing tree.json (aws#4984) feat(cli): adding new option to `cdk deploy` to indicate whether ChangeSet should be executed (aws#4852) chore: move semantic.yaml to .github/ fix(core): unable to find stack by name using the cli in legacy mode (aws#4998) fix(ecs-patterns): Fix issue related to protocol being passed to target group (aws#4988)

eladb · 2019-11-19T07:31:50Z

packages/aws-cdk/lib/api/deploy-stack.ts

+    debug('Stack %s has completed updating', deployName);
+  } else {
+    debug('Entering no-execute workflow for ChangeSet %s on stack %s', changeSetName, deployName);
+    cfn.executeChangeSet();


Why are we calling executeChangeSet here?

eladb · 2019-11-19T07:33:35Z

packages/aws-cdk/lib/api/deploy-stack.ts

-  await waitForStack(cfn, deployName);
-  if (monitor) { await monitor.stop(); }
-  debug('Stack %s has completed updating', deployName);
+  if (options.execute) {


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

eladb · 2019-11-19T07:34:37Z

packages/aws-cdk/lib/api/deploy-stack.ts

+    }
+    debug('Stack %s has completed updating', deployName);
+  } else {
+    debug('Entering no-execute workflow for ChangeSet %s on stack %s', changeSetName, deployName);


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

eladb · 2019-11-19T07:39:31Z

packages/aws-cdk/bin/cdk.ts

@@ -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})


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

eladb · 2019-11-19T07:49:36Z

Another issue is that the integ test is not actually working. Fails successfully with:

/Users/benisrae/code/cdk/aws-cdk/packages/aws-cdk/test/integ/cli/test-cdk-deploy-no-execute.sh: line 20: [: REVIEW_IN_PROGRESS: integer expression expected

The change that introduced the `--no-execute` feature (#4852) did not take into account that `cdk bootstrap` uses the same code path for deployment, and therefore `execute` was undefined, and resolved to false (despite the documented default). This change moves `execute` switch from the global scope into `deploy` and to `bootstrap` and passes it into the `cliBootstrap` command, as well as respects the default in case the option passed to the deployment utility is `undefined`. It also emits a message to STDOUT in case `--no-execute` is provided to let user know that deployment hasn't really finished. Otherwise, it appears as if deployment is successful. The `deploy-no-execute` integration test was also "failing successfully" due to invalid usage of bash conditions, so this is also fixed here. Added integration test to verify that --no-execute is respected by `cdk bootstrap`.

…5-eks-patterns * '4955-eks-patterns' of github.com:arhea/aws-cdk: feat(cli): adding new option to `cdk deploy` to indicate whether ChangeSet should be executed (aws#4852) chore: move semantic.yaml to .github/ fix(core): unable to find stack by name using the cli in legacy mode (aws#4998) fix(ecs-patterns): Fix issue related to protocol being passed to target group (aws#4988) chore: Backport CHANGELOG entries from v1.16.2 (aws#4980) feat(custom-resources): python handler skelaton in readme (aws#4977) fix(logs): cannot use same Lambda for multiple SubscriptionFilters (aws#4975) release: v1.16.1 (aws#4965) chore: resolve inaccurate label line in depbot config (aws#4729) chore(deps): bump aws-sdk from 2.568.0 to 2.569.0 (aws#4958)

robertma96 requested a review from RomainMuller as a code owner November 5, 2019 10:13

SomayaB assigned shivlaks Nov 5, 2019

shivlaks previously requested changes Nov 8, 2019

View reviewed changes

shivlaks changed the title ~~feat(aws-cdk): adding new option to cdk deploy~~ feat(cli): adding new option to cdk deploy to indicate whether ChangeSet should be executed Nov 8, 2019

Robert Marasescu added 2 commits November 8, 2019 16:49

Merge remote-tracking branch 'origin/master'

d8fc781

# Conflicts: # packages/aws-cdk/bin/cdk.ts # packages/aws-cdk/lib/api/deploy-stack.ts # packages/aws-cdk/lib/cdk-toolkit.ts

shivlaks reviewed Nov 11, 2019

View reviewed changes

Merge pull request #1 from aws/master

3e9ebdc

SYNC

shivlaks approved these changes Nov 13, 2019

View reviewed changes

mergify bot added 2 commits November 13, 2019 11:24

Merge branch 'master' into master

bd9caa9

Merge branch 'master' into master

6399716

mergify bot merged commit c02c9e5 into aws:master Nov 13, 2019

eladb reviewed Nov 19, 2019

View reviewed changes

eladb mentioned this pull request Nov 19, 2019

fix(cli): cdk bootstrap is broken due to --no-execute #5092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

robertma96 commented Nov 5, 2019

mergify bot commented Nov 5, 2019

aws-cdk-automation commented Nov 5, 2019

shivlaks left a comment

shivlaks Nov 8, 2019

robertma96 Nov 8, 2019

shivlaks Nov 11, 2019

eladb Nov 19, 2019

aws-cdk-automation commented Nov 8, 2019

robertma96 commented Nov 8, 2019

aws-cdk-automation commented Nov 8, 2019

shivlaks Nov 11, 2019

aws-cdk-automation commented Nov 12, 2019

aws-cdk-automation commented Nov 12, 2019

aws-cdk-automation commented Nov 12, 2019

aws-cdk-automation commented Nov 12, 2019

mergify bot commented Nov 13, 2019

aws-cdk-automation commented Nov 13, 2019

aws-cdk-automation commented Nov 13, 2019

eladb Nov 19, 2019

eladb Nov 19, 2019

eladb Nov 19, 2019

eladb Nov 19, 2019

eladb commented Nov 19, 2019

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

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

Conversation

robertma96 commented Nov 5, 2019

mergify bot commented Nov 5, 2019

aws-cdk-automation commented Nov 5, 2019

AWS CodeBuild CI Report

shivlaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

robertma96 commented Nov 8, 2019

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 12, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 12, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 12, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 12, 2019

AWS CodeBuild CI Report

mergify bot commented Nov 13, 2019

aws-cdk-automation commented Nov 13, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 13, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Nov 19, 2019

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

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