-
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Node 10 support #325
Node 10 support #325
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.
Thanks for getting this started @hcourt!
We should definitely make a prerelease and test this with ecs-telephone. I'd be happy to help with or pair on that!
Tested with a staging stack. Logs indicated that messages could be processed successfully. |
This reverts commit f2e71e6.
One (custom) resource was failing to finish creation, and I believe this is due to aws/aws-sdk-js#2955 Trying out a fix for this. |
This worked. |
This reverts commit 37af6f5.
Think this is finished. Before releasing, I will also merge #326. |
@@ -823,14 +823,14 @@ module.exports = (options = {}) => { | |||
Role: cf.getAtt(prefixed('LambdaScalingRole'), 'Arn'), | |||
Code: { | |||
ZipFile: cf.sub(` | |||
const response = require('cfn-response'); | |||
const 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.
Wow - nice find, though that is super weird. I did see that in the support ticket you linked (aws/aws-sdk-js#2955) they also recommend trying to update the AWS-SDK node dependency to the latest version. Would you consider trying that to see if it makes this change not needed?
We could also ship this as-is and open a support case to track - I'll leave that up to you.
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.
Hm, going to try it out, shouldn't take that long.
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.
I think this is a matter of where the Lambda or CloudFormation code packages cfn-response
, not anything to do with the aws sdk. The support ticket I linked also mentions this.
I did a lot of testing but I'm ready to let this go for now. I think even using a different package version would be more onerous than this pretty simple workaround. I will open a support case to track it.
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.
Thanks @hcourt 🎉
Fixes #324