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

fix: remove support for Node.JS 10.x #459

Merged
merged 1 commit into from
Jun 18, 2021

Conversation

kozlove-aws
Copy link
Contributor

Problem

Node.js 10 is not supported anymore since April 10, 2021
We need to update it with newer library.

Solution

Replaced nodejs10 with nodejs14 in Docker file for lambda layer.
Also was changed to take amazonlinux from public.ecr for avoiding limit hit.

Testing

Was build image and deployed on local account.
Was verified that lambda layer is using nodejs12.x, nodejs14.x


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

@kozlove-aws kozlove-aws changed the title fix: update NodeJS version in lambda layer [WIP]fix: update NodeJS version in lambda layer Jun 2, 2021
@kozlove-aws kozlove-aws changed the title [WIP]fix: update NodeJS version in lambda layer fix: update NodeJS version in lambda layer Jun 2, 2021
@jusiskin jusiskin added the contribution/core This is a PR that came from AWS. label Jun 3, 2021
Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

Looks like Node 10 references here that should also be updated

@ddneilson ddneilson changed the title fix: update NodeJS version in lambda layer fix: remove support for Node.JS 10.x Jun 9, 2021
ddneilson
ddneilson previously approved these changes Jun 14, 2021
Copy link
Contributor

@horsmand horsmand left a comment

Choose a reason for hiding this comment

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

Looks good, I only found one spot where we're referencing node 10 instead of node 12 in a message that pkglint prints out. I'm also wondering if we should be running npx yarn install so it can update the yarn.lock file. I see this line in there:

"@types/node" "^10.17.60"

@@ -376,11 +376,11 @@ export class NodeCompatibility extends ValidationRule {

public validate(pkg: PackageJson): void {
const atTypesNode = pkg.getDevDependency('@types/node');
if (atTypesNode && !atTypesNode.startsWith('^10.')) {
if (atTypesNode && !atTypesNode.startsWith('^12.')) {
pkg.report({
ruleName: this.name,
message: `packages must support node version 10 and up, but ${atTypesNode} is declared`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
message: `packages must support node version 10 and up, but ${atTypesNode} is declared`,
message: `packages must support node version 12 and up, but ${atTypesNode} is declared`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. yarn.lock is updated now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you include your changes in that most recent push? I don't see this rule or the yarn.lock updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Forgot to add files to commit.

@kozlove-aws kozlove-aws force-pushed the update_node10 branch 2 times, most recently from 72141f5 to ac12361 Compare June 17, 2021 00:42
Copy link
Contributor

@horsmand horsmand left a comment

Choose a reason for hiding this comment

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

This looks good, I don't see anything that needs updating, but it seems you have a conflict in your yarn.lock so you'll need to rebase your changes and push them. I'll approve after that.

Copy link
Contributor

@jericht jericht left a comment

Choose a reason for hiding this comment

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

Looks good!

I only see the one reference to "@types/node@^10.17.60" that @horsmand mentioned in our yarn.lock file and it looks like it's a dependency of @aws-cdk/cloudformation-diff. We can force it to resolve to @types/node@12.x.x by adding an entry in the resolutions in the root package.json:

  "resolutions": {
    // ...
    "**/@aws-cdk/cloudformation-diff/**/@types/node": "^12.8.3"
  },

These are just the typings though, so not really necessary. I checked out @aws-cdk/cloudformation-diff, changed it to depend on "@types/node": "^12.18.3", then built and tested it and it succeeded, so I think it's safe to assume it's not relying on things that are incompatible between Node 10 and 12.

@kozlove-aws kozlove-aws merged commit 52d5050 into aws:mainline Jun 18, 2021
@jusiskin jusiskin linked an issue Jun 22, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop Node10, add Node16
5 participants