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

Jump to bottom

fix(toolkit): do not deploy empty stacks #3144

Merged
merged 16 commits into from
Aug 28, 2019
Merged

fix(toolkit): do not deploy empty stacks #3144

merged 16 commits into from
Aug 28, 2019

Conversation

jogold
Copy link
Contributor

@jogold jogold commented Jul 1, 2019 •
edited
Loading

Do not deploy stacks without resources and issue a warning.

See also discussion #3142 (comment)

Please read the contribution guidelines and follow the pull-request checklist.

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

@jogold jogold requested a review from a team as a code owner July 1, 2019 12:56
@ghost ghost requested a review from shivlaks July 1, 2019 12:56
eladb
eladb previously requested changes Jul 1, 2019
View reviewed changes
packages/aws-cdk/lib/cdk-toolkit.ts Outdated Show resolved Hide resolved
@RomainMuller
Copy link
Contributor

I wonder if we shouldn't destroy empty stacks if they already exist? Or inject some dummy resource in the empty stacks (like a WaitConditionHandle)? This might end up being a somewhat more natural way to handle cases of stacks that have been delayed non-empty, and now are becoming empty...

dalawwa
dalawwa previously approved these changes Jul 2, 2019
View reviewed changes
@jogold
Copy link
Contributor Author

jogold commented Jul 3, 2019

@RomainMuller have you decided what should happen with destroy and empty stacks? Something for this PR?

@RomainMuller
Copy link
Contributor

@jogold:

  • destroy should be idempotent - if you try to destroy a stack that does not exist, it's instant success!
  • if we don't deploy empty stacks, we need to check if the stack exists & destroy it if it does.

@RomainMuller RomainMuller self-requested a review as a code owner August 7, 2019 17:14
@mergify
Copy link
Contributor

mergify bot commented Aug 22, 2019

Pull Request Checklist

  • Testing
  • Unit test added (prefer to add a new test rather than modify existing tests)
  • CLI change? Re-run/add CLI integration tests
  • Documentation
  • Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
  • README: update module README
  • Design: for significant features, follow the design process
  • Title uses the format type(scope): text
  • Type: fix, feat, refactor go into CHANGELOG, chore is hidden
  • Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
  • Style: use all lower-case, do not end with a period
  • 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>
  • Sensitive Modules (requires 2 PR approvers)
  • IAM document library (in @aws-cdk/aws-iam)
  • EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
  • Grant APIs (if not based on official documentation with a reference)

@mergify mergify bot dismissed eladb’s stale review August 22, 2019 08:55

Pull request has been modified.

@jogold
Copy link
Contributor Author

jogold commented Aug 22, 2019

  • if we don't deploy empty stacks, we need to check if the stack exists & destroy it if it does.

@RomainMuller updated PR to address this.

RomainMuller
RomainMuller reviewed Aug 26, 2019
View reviewed changes
packages/aws-cdk/lib/cdk-toolkit.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/cdk-toolkit.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/cdk-toolkit.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/cdk-toolkit.ts Outdated
success('%s: destroying...', colors.blue(stack.name));
try {
await destroyStack({ stack, sdk: options.sdk, deployName: stack.name, roleArn: options.roleArn });
success('\n ✅ %s: destroyed', colors.blue(stack.name));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm part afraid of the kind of messaging this will be on the whole experience... Should make sure all the messaging when you cdk deploy is "deployesque".

packages/aws-cdk/lib/cdk-toolkit.ts Outdated Show resolved Hide resolved
eladb
eladb previously requested changes Aug 26, 2019
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

We need integration tests for this

packages/aws-cdk/lib/cdk-toolkit.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed eladb’s stale review August 26, 2019 13:00

Pull request has been modified.

@jogold
Copy link
Contributor Author

jogold commented Aug 26, 2019

We need integration tests for this

You mean with cdk-integ-tools or something custom for this? Any guidance?

@shivlaks
Copy link
Contributor

We need integration tests for this

You mean with cdk-integ-tools or something custom for this? Any guidance?

the PR checklist above ^^ includes a link for CLI integration tests.
Reference: Integration tests

@jogold
Copy link
Contributor Author

jogold commented Aug 27, 2019

We need integration tests for this

done

shivlaks
shivlaks previously approved these changes Aug 28, 2019
View reviewed changes
RomainMuller
RomainMuller previously approved these changes Aug 28, 2019
View reviewed changes
@mergify mergify bot dismissed stale reviews from shivlaks and RomainMuller August 28, 2019 16:43

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Aug 28, 2019

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

@mergify
Copy link
Contributor

mergify bot commented Aug 28, 2019

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

@mergify mergify bot merged commit 64ace90 into aws:master Aug 28, 2019
@jogold jogold deleted the empty-stacks branch August 28, 2019 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shivlaks shivlaks shivlaks approved these changes

@dalawwa dalawwa dalawwa left review comments

@eladb eladb eladb left review comments

@RomainMuller RomainMuller RomainMuller left review comments

Assignees

@RomainMuller RomainMuller

@shivlaks shivlaks

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jogold @RomainMuller @shivlaks @eladb @dalawwa