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

Node packaging #1264

Merged
merged 18 commits into from
Jan 13, 2020
Merged

Node packaging #1264

merged 18 commits into from
Jan 13, 2020

Conversation

rorygarand
Copy link
Contributor

Adds a publish script to send the node sdk to the github package registry.

Related to: #679.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: e89aff76-2ecf-46b2-8937-1034bc16c187

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


cd $AGONES_PATH/sdks/nodejs

npm version $RELEASE_VERSION
Copy link
Collaborator

Choose a reason for hiding this comment

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

How/when does this script get run?

I think updating the version number should be a careful change and follow semver, so potentially even a manual step. Or are these variables automatically populated and the aim is to keep the package version the same as the release version? If that is the case then what will happen when there are no SDK changes and how do we still comply with semver?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK I've read #679 . Perhaps best to first focus on the publishing part and ensure it works and can look at again if we want to do version sync?

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to remove the version update. i was going to handle the release part - sounds like a check in the release checklist that we update the version (we can do this by hand?) would work.

So for this PR, we could drop the npm version $RELEASE_VERSION command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I only added that as a potential way to address an earlier comment from you @markmandel.

@steven-supersolid I believe the idea is to run this from release.mk but I was going to leave that part to Mark.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good and it seems we keep the SDK version the same as the Agones version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the intent... I took out the npm version command for now though, we can always add it later if we want.

@markmandel
Copy link
Member

markmandel commented Jan 8, 2020

Ah, this did actually break a thing!

We have a SDK test here:
https://github.com/markmandel/agones/blob/master/test/sdk/nodejs/testSDKClient.js#L15

So we'll need to change the name there in this PR too for it to pass.

We'll also need to update the nodejs example:
https://github.com/googleforgames/agones/blob/master/examples/nodejs-simple/src/index.js#L15

So it's worth noting this is a breaking change, just because the package name has changed.

Finally, we'll also need to update the documentation:
https://github.com/googleforgames/agones/blob/master/site/content/en/docs/Guides/Client%20SDKs/nodejs.md

(See https://agones.dev/site/docs/contribute/#within-a-page on how to have version specific documentation)

@markmandel markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/breaking Breaking change kind/feature New features for Agones labels Jan 8, 2020
@rorygarand
Copy link
Contributor Author

I was planning on updating the docs, is this PR the best place to do it?

Also, I'll add a fix to the test and remove the npm version command.

@markmandel
Copy link
Member

Yep this PR will be the perfect place - we love having it all in one spot.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: aa70138b-37b4-4129-b69f-2b33c3c800f9

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

@roberthbailey roberthbailey removed the request for review from EricFortin January 9, 2020 05:57
@markmandel
Copy link
Member

> @ testSDK /go/src/agones.dev/agones/test/sdk/nodejs
> node ./testSDKClient.js

internal/modules/cjs/loader.js:670
    throw err;
    ^

Error: Cannot find module '@googleforgames/agones'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:668:15)
    at Function.Module._load (internal/modules/cjs/loader.js:591:27)
    at Module.require (internal/modules/cjs/loader.js:723:19)
    at require (internal/modules/cjs/helpers.js:14:16)
    at Object.<anonymous> (/go/src/agones.dev/agones/test/sdk/nodejs/testSDKClient.js:15:19)
    at Module._compile (internal/modules/cjs/loader.js:816:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:827:10)
    at Module.load (internal/modules/cjs/loader.js:685:32)
    at Function.Module._load (internal/modules/cjs/loader.js:620:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:877:12)

Not sure how to solve these problems, but figured I'd find the relevant error message for you.

Let me know if you need help running this test locally.

@steven-supersolid
Copy link
Collaborator

Oh it won't work until published but we shouldn't publish until the tests all pass :) There is however another way for local testing using npm link.
From the test sdk folder run npm link <relative path to sdk> - this could be added to the script. Please note it will change package-lock.json but any changes can be discarded
https://docs.npmjs.com/cli/link.html

Copy link
Contributor Author

@rorygarand rorygarand left a comment

Choose a reason for hiding this comment

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

Yes, a bit of a catch 22. I was puzzling over the best approach to fix this.

I'll give your suggestion a try and see if that fixes the problem.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 994394b2-9682-48e9-ad16-5855113ceee8

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/GoogleCloudPlatform/agones.git pull/1264/head:pr_1264 && git checkout pr_1264
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-35f65b7

@markmandel
Copy link
Member

Passing now! This LGTM - @steven-supersolid any issues on your end, or can I approve and merge?

@steven-supersolid
Copy link
Collaborator

Oh I thought I already approved. LGTM

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Oh I thought I already approved. LGTM

Yep, just wanted to double check with the changes. LGTM!

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, rorygarand, 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

@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 01b7fe2d-33d7-4145-98f7-12ec12a82ee2

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/GoogleCloudPlatform/agones.git pull/1264/head:pr_1264 && git checkout pr_1264
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-0eee135

@markmandel markmandel merged commit d30a4c0 into googleforgames:master Jan 13, 2020
@markmandel markmandel added this to the 1.3.0 milestone Jan 13, 2020
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
Adds a publish script to send the node sdk to the github package registry.

Related to: googleforgames#679.

Co-authored-by: Mark Mandel <mark.mandel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/breaking Breaking change kind/feature New features for Agones size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants