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.

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

Merged
merged 2 commits into from
May 20, 2019

Conversation

robertd
Copy link
Contributor

@robertd robertd commented May 15, 2019


Added NodeJS10x support and updated bunch of tests across the board.

Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • [] Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@robertd robertd requested review from RomainMuller, skinny85 and a team as code owners May 15, 2019 02:54
@robertd
Copy link
Contributor Author

robertd commented May 15, 2019

Updated tests in the following packages to use NodeJS10x runtime:

  • aws-lambda
  • aws-apigateway
  • aws-codepipeline-actions
  • aws-cognito
  • aws-sns
  • aws-sqs
  • aws-lambda-event-sources
  • aws-events-targets
  • aws-dynamodb-global
  • aws-stepfunctions-tasks
  • runtime-values

Few questions:

  • aws-sdk is still offering nodejs4.3 and nodejs6.10, but I'm not sure if those runtimes can be used when creating Lambda functions anymore?
  • Should we delete them from here and here? That would introduce breaking changes.
  • Should aws-sdk devs do the same?
  • Does .travis.yml needs to be updated to latest nodejs v8 or nodejs v10 LTS for building purposes?

@robertd
Copy link
Contributor Author

robertd commented May 15, 2019

Btw... I was not able to run npm run integ since I have limited access to aws account.

@aws-cdk/aws-lambda: Error: The following integ stacks have changed: integ.bucket-notifications.js, integ.lambda.js, integ.layer-version.lit.js, integ.log-retention.js. Run 'npm run integ' to verify that everything still deploys.

I'm hoping that AWS CodeBuild and Travis will be able to run it.

Update: I figured it out for aws-lambda but aws-sqs is still failing.

@aws-cdk/aws-sqs: Error: The following integ stacks have changed: integ.bucket-notifications.js. Run 'npm run integ' to verify that everything still deploys.

@robertd robertd force-pushed the add-nodejs10x-runtime branch from 383241e to bf829da Compare May 15, 2019 03:24
@jogold
Copy link
Contributor

jogold commented May 15, 2019

Please note that although the new runtime is available and functions can be deployed with CloudFormation, currently this is only possible when using Code.S3Bucket and Code.S3Key.

This means that the integration tests that currently use the Code.inline (resulting in ZipFile property being set for code) are failing with ZipFile can only be used when Runtime is set to either of nodejs, nodejs4.3, nodejs6.10, nodejs8.10, python2.7, python3.6, python3.7.

@eladb
Copy link
Contributor

eladb commented May 15, 2019

I don’t think it’s critical to update all our tests to use this new runtime.

@jogold
Copy link
Contributor

jogold commented May 15, 2019

I think that the runtimes Runtime.NodeJS, Runtime.NodeJS43 and Runtime.NodeJS610 should be removed. It's no longer possible to create functions with these runtimes https://docs.aws.amazon.com/lambda/latest/dg/runtime-support-policy.html

@eladb
Copy link
Contributor

eladb commented May 15, 2019

I think that the runtimes Runtime.NodeJS, Runtime.NodeJS43 and Runtime.NodeJS610 should be removed. It's no longer possible to create functions with these runtimes https://docs.aws.amazon.com/lambda/latest/dg/runtime-support-policy.html

Not yet...

@jogold
Copy link
Contributor

jogold commented May 15, 2019

I said create not update 😉

@robertd
Copy link
Contributor Author

robertd commented May 15, 2019

I’ll at least update all tests to use NodeJS8.10. That should also fix aws-sqs integration tests.

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)

@robertd
Copy link
Contributor Author

robertd commented May 15, 2019

@eladb I've updated all tests to use NodeJS810 just in case 4.3, 6.10 get dropped at some point.

@robertd robertd changed the title WIP: feat(lambda): add nodejs10x runtime and update tests feat(lambda): add nodejs10x runtime and update tests May 15, 2019
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.

Please change the "allows inline" to false for node 10

packages/@aws-cdk/aws-lambda/lib/runtime.ts Show resolved Hide resolved
@robertd robertd force-pushed the add-nodejs10x-runtime branch from c9c4286 to b25c7a2 Compare May 16, 2019 06:12
@leepa
Copy link
Contributor

leepa commented May 16, 2019

Test failing needs looking at as:

Error: Inline source not allowed for nodejs10.x

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?

@eladb
Copy link
Contributor

eladb commented May 16, 2019

@leepa thanks for looking into this. committed a fix and I will merge if CI passes

@coderbyheart
Copy link
Contributor

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 new Runtime('...') instead.

@eladb
Copy link
Contributor

eladb commented May 16, 2019

How about we add a @deprecated tag to them?

@robertd
Copy link
Contributor Author

robertd commented May 16, 2019

According to deprecation schedule nodejs and nodejs4.3 can be removed right now. Create and update deprecation period has been reached for both. However for nodejs6.10 and dotnetcore2.0 we can put @deprecated tag until their dates come up (in the next few weeks/month).

Question is... will aws-sdk do the same? https://github.com/aws/aws-sdk-js/blob/master/clients/lambda.d.ts#L1473

@rix0rrr
Copy link
Contributor

rix0rrr commented May 20, 2019

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 @deprecated tags.

@rix0rrr rix0rrr merged commit 553577a into aws:master May 20, 2019
@robertd robertd deleted the add-nodejs10x-runtime branch May 20, 2019 16:01
@nmussy nmussy mentioned this pull request Aug 9, 2019
5 tasks
This was referenced Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants