-
Notifications
You must be signed in to change notification settings - Fork 801
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 nodejs-simple cannot find agones-sdk module and update server_tag to 0.8 #2633
Conversation
Build Failed 😱 Build Id: a9ca267c-87bc-4ab7-9f25-ed666c5bb6bb To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Network error? I'd like to retest when the rebase
|
examples/nodejs-simple/Dockerfile
Outdated
@@ -35,6 +35,7 @@ RUN apt-get update && apt-get install -y nodejs | |||
|
|||
WORKDIR /home/server | |||
|
|||
COPY ../../sdks/nodejs sdks/nodejs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source path may not be correct here, e.g. see https://github.com/googleforgames/agones/blob/main/examples/cpp-simple/Dockerfile#L25 and also the line below which I think means the root project path is being used, which would mean changing to
COPY ./sdks/nodejs sdks/nodejs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review!
As you said, I missed the root path and docker build context.
I fixed the COPY path.
What are the problems with |
Phenomenon When try running the example container
Direct cause
Root cause Currently there is no test(E2E) which the Originally I sent this PR just to fix this bug, but if E2E test for node-simple is desirable, I would like to add E2E test case. |
Build Failed 😱 Build Id: 0b2dd21b-a551-4fc0-a452-639314fdd890 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
same error. I would test build-sdk localy and investigate later...
|
That Rust error was fixed in: #2635 - if you rebase against main, you should be good to go. |
Build Succeeded 👏 Build Id: 9e53cbda-4c78-41e3-817d-47a7cb01b81d 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:
|
Thanks! CI is green. |
@markmandel - LGTM but there is still the question of whether the docker image push is manual or automatic? |
Ah! Sorry - yep, example image push is a manual process. We have a check in the release process that makes sure that each example image exists before a release occurs - but we don't build images automatically. Those of us who have access to the project can push up the images though. |
We should merge this once the #2637 is merged and the release is complete though. |
So I just did a local build of the image and tested out running it, and got the following error:
|
Thanks. I confirmed this same error now in my local. This didn't happen before. I'll look to investigate this. |
Build Failed 😱 Build Id: 79c07427-33cb-40e9-8681-ab93655075f9 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 0a1094bf-fb17-4deb-8b00-1763f8075783 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:
|
@@ -35,6 +35,9 @@ RUN apt-get update && apt-get install -y nodejs | |||
|
|||
WORKDIR /home/server | |||
|
|||
COPY ./sdks/nodejs sdks/nodejs | |||
RUN cd sdks/nodejs && \ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Build Succeeded 👏 Build Id: 08b9fd5d-eb4e-45ad-bbb7-8dba956087f3 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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took this for a run, and it all worked perfectly! Thanks!
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: govargo, markmandel, 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 |
1 similar comment
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: govargo, markmandel, 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 |
What type of PR is this?
/kind bug
What this PR does / Why we need it:
Current nodejs-simple example cannot run due to the module not found.
The agones-sdk module is targeted in the file path but it doesn't exist in the target directory.
I found
gcr.io/agones-images/nodejs-simple-server:0.6
andgcr.io/agones-images/nodejs-simple-server:0.7
doesn't work.This PR fix nodejs-simple example bug.
Which issue(s) this PR fixes:
Closes #2625
Special notes for your reviewer:
Q1. I think we have to push docker image
gcr.io/agones-images/nodejs-simple-server:0.8
.Can I or you push this image automatically or manually?
Q2. Should I test the e2e test for nodejs-simple?