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

Update to nodejs 16 and npm lockfile version 2 #2465

Merged
merged 4 commits into from
Feb 7, 2022

Conversation

steven-supersolid
Copy link
Collaborator

What type of PR is this?
/kind cleanup

What this PR does / Why we need it:

  • Update to Node.js 16.x as this is the current LTS version
  • Update also includes npm update to 8.x (8.1.2 bundled with node 16.13.2 used to perform the package-lock upgrade)
  • Update dependencies
  • Resync package-lock file with examples - should be easier to keep in sync now
  • Update promisified version of setTimeout in example to use that included in Node.js 16

Which issue(s) this PR fixes:

Closes #2450

Special notes for your reviewer:

const util = require('util');

const sleep = util.promisify(setTimeout);
const {setTimeout} = require('timers/promises');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will require using Node.js 16, we could revert if we want the examples to remain backwards compatible

Copy link
Member

Choose a reason for hiding this comment

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

I'd be more concerned if the SDK code required the new version but I think it's ok for the example code to use the new version.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 51e8bf1f-e678-4175-98e4-8017b941f1fe

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@google-oss-prow
Copy link

@steven-supersolid: GitHub didn't allow me to request PR reviews from the following users: robertbailey.

Note that only googleforgames members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc robertbailey

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@steven-supersolid
Copy link
Collaborator Author

/cc roberthbailey

@steven-supersolid
Copy link
Collaborator Author

The build failed on this step

Step #10 - "lint": Step 34/42 : RUN apt-get update && apt-get install -y nodejs
Step #10 - "lint":  ---> Running in 469ca59598d7
Step #10 - "lint": Hit:1 http://deb.debian.org/debian buster InRelease
Step #10 - "lint": Hit:2 http://deb.debian.org/debian buster-updates InRelease
Step #10 - "lint": Hit:3 http://security.debian.org/debian-security buster/updates InRelease
Step #10 - "lint": Ign:4 https://deb.nodesource.com/node_16.x jessie InRelease
Step #10 - "lint": Err:5 https://deb.nodesource.com/node_16.x jessie Release
Step #10 - "lint":   404  Not Found [IP: 23.62.230.72 443]
Step #10 - "lint": Reading package lists...
Step #10 - "lint": �[91mE: The repository 'https://deb.nodesource.com/node_16.x jessie Release' does not have a Release file.
Step #10 - "lint": The command '/bin/sh -c apt-get update && apt-get install -y nodejs' returned a non-zero code: 100
Step #10 - "lint": make[2]: *** [includes/build-image.mk:26: build-build-image] Error 100
Step #10 - "lint": �[0mmake[2]: Leaving directory '/workspace/build'
Step #10 - "lint": make[1]: *** [includes/build-image.mk:54: ensure-image] Error 2
Step #10 - "lint": make[1]: Leaving directory '/workspace/build'
Step #10 - "lint": make: *** [includes/build-image.mk:41: ensure-build-image] Error 2

Looking at https://deb.nodesource.com/node_12.x/dists/, there is a jessie folder but https://deb.nodesource.com/node_16.x/dists/ doesn't have one although the next version stretch is present

@roberthbailey
Copy link
Member

Debian 8 "jessie" is super old (initially released in 2015) so I'm not sure why we would still be using it. Our build image is using Debian 10 "buster" (see here).

Any idea why it switches from buster

Step #10 - "lint": Hit:3 http://security.debian.org/debian-security buster/updates InRelease

to jessie

Step #10 - "lint": Ign:4 https://deb.nodesource.com/node_16.x jessie InRelease

in the following step?

@steven-supersolid
Copy link
Collaborator Author

It looks like the version arg was introduced in #2438 and set differently

@steven-supersolid
Copy link
Collaborator Author

From https://github.com/nodesource/distributions/blob/master/README.md#manual-installation it looks like we could alternatively we could replace the manual arg setting with the following if that will execute correctly in the Dockerfile?

ARG DISTRO="$(lsb_release -s -c)"

@roberthbailey
Copy link
Member

@cindy52 might have some context on the change in #2438

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 63dd11ac-1803-415f-bf77-0c92f4717755

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@steven-supersolid
Copy link
Collaborator Author

Tests failing due to an expired certificate on a 3rd party website:

for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; \
	do echo "Html Test: Attempt $i" && \
	  docker run --rm -t -e "TERM=xterm-256color" -v /workspace/build//.config/gcloud:/root/.config/gcloud -v ~/.kube/:/root/.kube -v ~/.config/helm:/root/.config/helm -v ~/.cache/helm:/root/.cache/helm -v /workspace:/go/src/agones.dev/agones -v /workspace/build//.gomod:/go/pkg/mod -v /workspace/build//.gocache:/root/.cache/go-build  agones-build:02a242a0ad bash -c \
		"mkdir -p /tmp/website && cp -r /go/src/agones.dev/agones/site/public /tmp/website/site && htmltest -c /go/src/agones.dev/agones/site/htmltest.yaml /tmp/website" && \
break || sleep 60 && false; done
Html Test: Attempt 1
htmltest started at 08:02:16 on /tmp/website
========================================================================
site/index.html
  Get https://www.embark.games/: x509: certificate has expired or is not yet valid --- site/index.html --> https://www.embark.games/
========================================================================
✘✘✘ failed in 12m54.620830518s

@roberthbailey
Copy link
Member

You can add a data-proofer-ignore to it like we have for some of the other external links on that page.

@steven-supersolid
Copy link
Collaborator Author

The certificate looks to be renewed so will try another test

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: e4b8f9cf-350f-451b-b745-e214acae2a80

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: eec12505-bcb8-4abc-8827-a041ade26438

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2465/head:pr_2465 && git checkout pr_2465
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.21.0-41b91d4

const util = require('util');

const sleep = util.promisify(setTimeout);
const {setTimeout} = require('timers/promises');
Copy link
Member

Choose a reason for hiding this comment

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

I'd be more concerned if the SDK code required the new version but I think it's ok for the example code to use the new version.

@google-oss-prow google-oss-prow bot added the lgtm label Feb 7, 2022
@roberthbailey roberthbailey merged commit 0080ae7 into googleforgames:main Feb 7, 2022
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: roberthbailey, steven-supersolid

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@steven-supersolid steven-supersolid deleted the nodejs-ugrade branch February 7, 2022 19:25
@SaitejaTamma SaitejaTamma added this to the 1.21.0 milestone Feb 8, 2022
@roberthbailey roberthbailey mentioned this pull request Feb 9, 2022
@cindy52
Copy link
Contributor

cindy52 commented Feb 9, 2022

From https://github.com/nodesource/distributions/blob/master/README.md#manual-installation it looks like we could alternatively we could replace the manual arg setting with the following if that will execute correctly in the Dockerfile?

ARG DISTRO="$(lsb_release -s -c)"

The arg doesn't work as I tried before and I need to provide a specific DISTRO.

@cindy52
Copy link
Contributor

cindy52 commented Feb 9, 2022

Debian 8 "jessie" is super old (initially released in 2015) so I'm not sure why we would still be using it. Our build image is using Debian 10 "buster" (see here).

Any idea why it switches from buster

Step #10 - "lint": Hit:3 http://security.debian.org/debian-security buster/updates InRelease

to jessie

Step #10 - "lint": Ign:4 https://deb.nodesource.com/node_16.x jessie InRelease

in the following step?

We can change the DISTRO ARG from jessie to buster here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to node 16 / npm 7
5 participants