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

change behaviour on initial stack create failed #5631

Merged
merged 7 commits into from Dec 31, 2018

Conversation

Projects
None yet
3 participants
@Imran99
Copy link
Contributor

Imran99 commented Dec 28, 2018

What did you implement:

Closes #5630

How did you implement it:

Change stack creation failed behaviour to DELETE.

How can we verify it:

Deploy a failing stack to confirm. e.g trigger a lambda with a nonexistent sns resource.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@shortjared

This comment has been minimized.

Copy link
Member

shortjared commented Dec 28, 2018

My biggest issue with this is often I want to go in and debug why this happened. Of course you could go look at deleted stacks, but by default this is hidden in the console and may lead to some confusion.

Generally if something failed you need to know why, fix it, delete the bad stack, then redeploy. This skips the delete. The biggest consideration is the hidden by default nature of deleted stacks, and ability to redeploy a known "bad stack" over and over which doesn't seem like a huge deal.

@dschep
Copy link
Member

dschep left a comment

@shortjared makes good points re: debugging.

to facilitate this, could you add a log output containing a link to the deleted stack in the cloudformation console?

shanehandley pushed a commit to shanehandley/serverless that referenced this pull request Dec 30, 2018

Fix issues with PR serverless#5631 causing base64 errors
- Some of values were not converted back from base64, which resulted in
  spurious errors with base64 strings.
  (This was caused by nested arrays, like commands, not being converted
   back, combined with the wrong assumption that commands always occur
   before options. It is not always true, as the added test case
   demonstrates.)
- Some other values were converted *from* base64, but they weren't
  base64 in the first place, which resulted in junk binary data.
  (This was due to the invalid assumption that only odd-numbered values
   need to be converted.)

shanehandley pushed a commit to shanehandley/serverless that referenced this pull request Dec 30, 2018

Merge pull request serverless#5492 from operasoftware/jkozera/fix-pr-…
…5361-issues

Fix issues with PR serverless#5631 causing base64 errors

Imran99 added some commits Dec 30, 2018

@Imran99

This comment has been minimized.

Copy link
Contributor

Imran99 commented Dec 30, 2018

The biggest consideration is the hidden by default nature of deleted stacks

Yeh thats true, an unfortunate quirk of the aws cf console. However I'm also looking at this from the perspective of a production environment. Manually deleting stacks during development is no biggy but I've seen a stack create failure sometimes make it through to production because of some intermittent third party plugin bug for example. In that case we'd want to avoid having to log in to a production environment with a privileged role and then manually deleting the stack before we can redeploy it. The manual step always carries some risk of human error.

to facilitate this, could you add a log output containing a link to the deleted stack in the cloudformation console?

Good idea, added that.

Imran99 added some commits Dec 30, 2018

@dschep

dschep approved these changes Dec 31, 2018

Copy link
Member

dschep left a comment

:shipit:

@dschep dschep merged commit f3273be into serverless:master Dec 31, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.006%) to 90.787%
Details

dschep added a commit that referenced this pull request Jan 9, 2019

v1.36.0 release!
 - [Log AWS SDK calls in debug mode](#5604)
 - [Added currently supported regions for GCP functions](#5601)
 - [Update Cloudflare Templates](#5620)
 - [AWS: Validate rate/cron syntax before Deploy](#5635)
 - [Fix error log output](#5378)
 - [Support for native async/await in AWS Lambda for aws-nodejs-typescript template ](#5607)
 - [aws-csharp create template uses handler-specific artifact](#5411)
 - [change behaviour on initial stack create failed](#5631)
 - [Add warning for multiple functions having same handler](#5638)
 - [AWS: Add API Gateway stage name validation.](#5639)

@dschep dschep referenced this pull request Jan 9, 2019

Merged

v1.36.0 release! #5670

@shortjared shortjared added this to the 1.36.0 milestone Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment