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.

Sign up for GitHub

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

Jump to bottom

Fix nodejs-simple cannot find agones-sdk module and update server_tag to 0.8 #2633

Merged
merged 2 commits into from
Jun 27, 2022
Merged

Fix nodejs-simple cannot find agones-sdk module and update server_tag to 0.8 #2633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9401cc7
Fix nodejs-simple cannot find agones-sdk module and update server_tag…
govargo Jun 17, 2022
e918121
Merge branch 'main' into fix-nodejs-simple
markmandel Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion examples/nodejs-simple/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,12 @@ RUN apt-get update && apt-get install -y nodejs

WORKDIR /home/server

COPY ./sdks/nodejs sdks/nodejs
RUN cd sdks/nodejs && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why we have npm rebuild below so this may be a good time to remove it and on this line too, but it won't do anything bad.

npm rebuild calls npm build which is an internal command to npm. The same command is called from npm install already as far as I could gather, so is redundant. Information is a bit scarce on this.

Another potential error in this file is that we are using npm install instead of npm ci. The former can override package-lock.json and also takes slightly longer to run. Generally npm install should only be run by users and npm ci by automation. In this case I don't think we are running tests as part of the Docker build so do not need the dev dependencies and so can use npm ci --production

So these changes are fine but we are duplicating lines that could be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your great review!!

I updated and tested locally. This works fine.

npm ci --production
COPY ./examples/nodejs-simple examples/nodejs-simple
RUN cd examples/nodejs-simple && \
npm install && npm rebuild
npm ci --production
RUN chown -R server /home/server
USER 1000

Expand Down
2 changes: 1 addition & 1 deletion examples/nodejs-simple/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ REPOSITORY = gcr.io/agones-images
# Directory that this Makefile is in.
mkfile_path := $(abspath $(lastword $(MAKEFILE_LIST)))
project_path := $(dir $(mkfile_path))
server_tag = $(REPOSITORY)/nodejs-simple-server:0.7
server_tag = $(REPOSITORY)/nodejs-simple-server:0.8
root_path = $(realpath $(project_path)/../..)

# _____ _
Expand Down
12 changes: 6 additions & 6 deletions examples/nodejs-simple/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ $ make run

The example can also be run via docker:
```
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.6
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.8
```
Or directly via npm:
```
Expand All @@ -51,7 +51,7 @@ $ npm start

You will see the output like the following:
```
docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.6
docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.8

> @ start /home/server/examples/nodejs-simple
> node src/index.js
Expand All @@ -63,22 +63,22 @@ Connecting to the SDK server...
To see help, pass `--help` as the argument (use the preferred command below, all are equivalent):
```
$ make args="--help" run
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.6 --help
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.8 --help
$ npm start -- --help
```

You can optionally specify how long the server will stay up once the basic tests are complete with the `--timeout` option.
To do this pass arguments through, e.g. to increase the shutdown duration to 120 seconds:
```
$ make args="--timeout=120" run
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.6 --timeout=120
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.8 --timeout=120
$ npm start -- --timeout=120
```

To make run indefinitely use the special timeout value of 0:
```
$ make args="--timeout=0" run
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.6 --timeout=0
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.8 --timeout=0
$ npm start -- --timeout=0
```

Expand All @@ -90,6 +90,6 @@ $ cd ../../build; make run-sdk-conformance-local TIMEOUT=120 FEATURE_GATES="Play
Then enable the alpha suite:
```
$ make args="--alpha" run
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.6 --alpha
$ docker run --network=host gcr.io/agones-images/nodejs-simple-server:0.8 --alpha
$ npm start -- --alpha
```
2 changes: 1 addition & 1 deletion examples/nodejs-simple/gameserver.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,5 @@ spec:
spec:
containers:
- name: nodejs-simple
image: gcr.io/agones-images/nodejs-simple-server:0.6
image: gcr.io/agones-images/nodejs-simple-server:0.8
# args: ["--timeout=0"] # Change the timeout here, if you like the nodejs server to run longer.