-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(lambda): add nodejs10x runtime and update tests #2544
Conversation
Updated tests in the following packages to use
Few questions:
|
Btw... I was not able to run
I'm hoping that AWS CodeBuild and Travis will be able to run it. Update: I figured it out for
|
383241e
to
bf829da
Compare
Please note that although the new runtime is available and functions can be deployed with CloudFormation, currently this is only possible when using This means that the integration tests that currently use the |
I don’t think it’s critical to update all our tests to use this new runtime. |
I think that the runtimes |
Not yet... |
I said create not update 😉 |
I’ll at least update all tests to use NodeJS8.10. That should also fix Also, Amazon Linux environment running Lambdas will receive updates next week. Not sure how will that affect functions (and lambda layers) out there that are using < NodeJS10x. (https://aws.amazon.com/blogs/compute/upcoming-updates-to-the-aws-lambda-execution-environment/) NodeJS10x runs on AL2 exec env anyway (good overview thread https://twitter.com/hichaelmart/status/1128055568764166144?s=21) |
bf829da
to
c9c4286
Compare
@eladb I've updated all tests to use NodeJS810 just in case 4.3, 6.10 get dropped at some point. |
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.
Please change the "allows inline" to false for node 10
c9c4286
to
b25c7a2
Compare
Test failing needs looking at as:
So it's asserting when doing the 'using an incompatible layer' test. It's correct, Node 10 does not support inline, but the test was flipped to use 10 and probably shouldn't have been? |
@leepa thanks for looking into this. committed a fix and I will merge if CI passes |
I am for removing 4.3 and 6.10 with this change, since you no longer can create lambdas with either of them. I understand that there might be people managing existing lambdas, but they can use |
How about we add a |
According to deprecation schedule Question is... will |
I'm not in favor of removing any constants, and here's my motivation: In the event where you have 2 stacks, stack A with a Node4 Lambda in it, and stack B that you want/need to update right now: that constant won't do any damage as long as you don't update the stack it's used in. However, removing the constant will prevent your entire application from compiling, and so will make it impossible to do anything to stack B until you fix the compilation error. Even though it's fairly trivial, there is some interruption and cognitive overhead to that that I feel we shouldn't impose on our users. Having said that, since we're pre-1.0 we're still free to remove the constant now, but once we go 1.0 we don't have that luxury anymore. Might as well take the opportunity as long as we have it :). I'm going to merge this PR now in order to get the new runtime in, I'm open to accepting a new PR with |
Added
NodeJS10x
support and updated bunch of tests across the board.Pull Request Checklist
design
folderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.