Skip to content

Updating node verson #422

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

Merged
merged 7 commits into from
May 5, 2020
Merged

Conversation

ColtonHerrodWork
Copy link

Updating the node version for the example TODO application since 6.x is no longer supported in Lambda.

@christophgysin
Copy link
Contributor

There are a few more examples that still use nodejs6.10, could you update those too?

https://github.com/serverless/examples/search?q=%22runtime%3A+nodejs6.10%22

@ColtonHerrodWork
Copy link
Author

Updated the rest of them, should be good to go now.

Copy link

@preshetin preshetin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest updating runtime to recommended nodejs12.x.

When I was trying to sls deploy one of the examples using nodejs8.10, I got this error

  Serverless Error ---------------------------------------
 
  An error occurred: TimeLambdaFunction - The runtime parameter of nodejs8.10 is no longer supported for creating or updating AWS Lambda functions. We recommend you use the new runtime (nodejs12.x) while creating or updating functions.

Also, according to AWS deprecation schedule, even updating of Node.js 8.10 will be very soon deprecated.

P.S. This is my first code review. My motivation is that I also encountered deprecation runtime errors while using these examples. I hope this review will speed up things with merging.

Copy link

@preshetin preshetin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update needed from nodejs8.10 to nodejs12.x

@ColtonHerrodWork
Copy link
Author

Updated existing examples to nodejs12.x to leverage latest supported runtime.

@eahefnawy
Copy link
Contributor

@ColtonHerrodWork I know this review is long overdue. I'm sorry about that. I'd like to merge this right away! Could you just fix the conflicts? 😊

Copy link

@preshetin preshetin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fvant
Copy link
Contributor

fvant commented Apr 22, 2020

@ColtonHerrodWork you have a possibility to resolve that merge conflict ? or would you like a hand ?

@preshetin
Copy link

preshetin commented Apr 22, 2020

@fvant I think it was already resolved in #486

So I think this PR should be closed

@fvant
Copy link
Contributor

fvant commented Apr 22, 2020

@fvant I think it was already resolved in #486

So I think this PR should be closed

i checked out aws-node-env-variables today and it still had node8.10, triggering me to checkout the PR/issues
#486 only has 1 file...

@preshetin
Copy link

@fvant I see.

It seems there is no activity. So if you decide to create a PR, you may ping me to make a code review

@fvant
Copy link
Contributor

fvant commented Apr 22, 2020

@preshetin created #501 with resolved merge conflict

@preshetin preshetin mentioned this pull request Apr 23, 2020
@eahefnawy eahefnawy merged commit 110c326 into serverless:master May 5, 2020
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.

5 participants