-
Notifications
You must be signed in to change notification settings - Fork 30
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
custom cloudformation resource for scale up adustment #249
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried passing garbage strings into the maxSize option? If we allow a ref, the user could pass in anything
Yep, when passing in garbage the response is:
Or:
|
Nice! |
Awesome! 馃檶 |
Ah - I've realized that you could send a string parameter in if you changed the type of your cloudormation parameter from
I'm against error handling in the custom resource; because if that lambda function errors the stack ends up in an hour long wait before ending in an UpdateRollbackFailed state. I don't see that we've done any validation on other fields like CPU, minSize, etc. So it feels okay to me to leave this as the way a user encounters failure if they put in a non-numeric maxSize. |
lib/template.js
Outdated
Role: cf.getAtt(prefixed('ScalingRole'), 'Arn'), | ||
Code: { | ||
ZipFile: cf.sub(` | ||
var response = require('cfn-response'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooooh good find.
lib/template.js
Outdated
Role: cf.getAtt(prefixed('LambdaScalingRole'), 'Arn'), | ||
Code: { | ||
ZipFile: cf.sub(` | ||
var response = require('cfn-response'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick var
=> const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one nitpick, but other than that I think this looks great
Goals here:
maxSize
to be parameterized in templatesTested this out with maxSize as a cf.ref and updating maxSize to 3333, 33, 250, etc. Looked 馃啑 to me.
cc: @jakepruitt @arunasank @k-mahoney @rclark