-
Notifications
You must be signed in to change notification settings - Fork 820
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
Support Unity Package Manager #2343
Support Unity Package Manager #2343
Conversation
- Agones assembly definition - Package information - Meta files required for the engine
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aaronchapin-tocaboca The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build Succeeded 👏 Build Id: f31fb896-b1a0-409e-8c61-0d04609a9f4b 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:
|
@@ -0,0 +1,11 @@ | |||
{ | |||
"name": "com.googleforgames.agones", | |||
"version": "1.18.1", |
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.
Is this the version of Unity it works with, or is this the version of Agones it corresponds to?
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.
This would be the version of Agones that the package corresponds to.
A complete list of all the properties and their descriptions can be found here: https://docs.unity3d.com/Manual/upm-manifestPkg.html
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.
That page says that the version must be MAJOR.MINOR.PATCH
but also says it can follow semantic versioning, which would allow us to have this version match what we use elsewhere, e.g. 1.19.0-dev
(then 1.19.0-rc
, then 1.19.0
, then 1.20.0-dev
, and so on).
Do you know if unity will work correctly with our semantic versioning scheme.
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 answered this question in another thread, but just to have the info here as well: I tested this semantic versioning scheme and it is supported.
Build Succeeded 👏 Build Id: 2daae425-5842-4b5b-85b9-9cb16d5f40a8 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.
There are a lot of guid
values in these files. Where do they come from? And when do they need to be updated?
If we change the version during the release process, does that require updating any other fields?
@@ -0,0 +1,11 @@ | |||
{ | |||
"name": "com.googleforgames.agones", | |||
"version": "1.18.1", |
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.
That page says that the version must be MAJOR.MINOR.PATCH
but also says it can follow semantic versioning, which would allow us to have this version match what we use elsewhere, e.g. 1.19.0-dev
(then 1.19.0-rc
, then 1.19.0
, then 1.20.0-dev
, and so on).
Do you know if unity will work correctly with our semantic versioning scheme.
sdks/unity/package.json
Outdated
"description": "The Unity Gameserver SDK for Agones", | ||
"unity": "2019.1", | ||
"author": { | ||
"name": "Google For Games", |
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 NodeJS and csharp SDKs has the author set to "Google LLC"
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.
Regarding semantic versioning - yes it does work. I tried out a -dev append on a separate branch (here) and it functioned as expected. So that shouldn't be a problem.
I wasn't sure what to set the version number to, but I assumed that if this PR is accepted it'll go out with the next release. Do you have a way of updating the version number across the repo that I could utilize here?
Regarding author name, that has been updated.
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.
Please add a new step to the section "Update SDK Package Versions" in https://github.com/googleforgames/agones/blob/main/docs/governance/templates/release_issue.md to bump the version number (we currently have steps for the nodejs and csharp SDKs). You should also add a step near the very end to bump the version after a full release (look for "If full release, change to the sdks/nodejs
directory").
Since this will pull from github, it sounds like we don't need to update the "Publish SDK packages" section.
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.
Updated 👍
…apin-tocaboca/agones into feature/issue-2338-unity-package
They are created by the Unity engine on file creation. They don't need to be updated aside from removing the .meta file if the file is removed. In fact, if they are updated it could cause issues.
Not as far as I'm aware. |
Build Succeeded 👏 Build Id: 310decd9-3a26-45d2-8c17-e026227400be 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:
|
Build Failed 😱 Build Id: 8bbc7671-f1da-446e-9538-6572e2f1131f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
…apin-tocaboca/agones into feature/issue-2338-unity-package
Build Failed 😱 Build Id: 180406dc-a5ef-4a7b-aa3b-9989b6f47858 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 7158a89a-04f8-4b50-80b2-1a548aa073c3 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:
|
@@ -80,6 +80,7 @@ and copy it into a release issue. Fill in relevant values, found inside {} | |||
- [ ] If full release move the [helm `Chart` version values][chart] is to {version}+1-dev | |||
- [ ] If full release, change to the `sdks/nodejs` directory and run the command `npm version {version}+1-dev` to update the package version | |||
- [ ] If full release move the [`sdks/csharp/sdk/AgonesSDK.nuspec` and `sdks/csharp/sdk/csharp-sdk.csproj`][csharp] to {version}+1-dev | |||
- [ ] If full release update the [`sdks/unity/package.json`][unity] package file's `Version` field to {version}+1-dev |
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.
We should also add a checklist item in the Update SDK Package Versions
section as well to increment the Unity 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.
Done 👍
…issue-2338-unity-package
…apin-tocaboca/agones into feature/issue-2338-unity-package
Build Failed 😱 Build Id: 93900d10-2f49-4af3-8b3a-54628bddd2f3 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Looks like a flake of |
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.
This LGTM!
The only thing I just realised was outstanding on my end -
We should update https://agones.dev/site/docs/guides/client-sdks/unity/#download to have the new instructions on the best way to install.
We'll need to use the feature
shortcode to hide and show the current vs new content:
https://agones.dev/site/docs/contribute/#within-a-page
Build Succeeded 👏 Build Id: 387e7f2e-a8ce-47df-9e46-c44f6e6bc632 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:
|
…issue-2338-unity-package
…apin-tocaboca/agones into feature/issue-2338-unity-package
Build Succeeded 👏 Build Id: bbd8a3fd-65b7-4c91-a731-4e09981d400a 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:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aaronchapin-tocaboca, markmandel 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 |
Nice work! |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: 0c36225e-53d1-4b9b-8bc7-ee2034fdc9f7 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:
|
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Unity has its own package manager, which can be used to load external dependencies. This change allows unity developers to add the Agones SDK to their project by simply adding a line to their
manifest.json
file that follows this format:"com.googleforgames.agones": "https://github.com/googleforgames/agones.git?path=/sdks/unity#<TAG>",
Instead of manually downloading the SDK code locally and placing it in their project's folder structure.
Which issue(s) this PR fixes:
Closes #2338
Special notes for your reviewer:
I took some educated guesses about the desired values for fields like the version number, author name/url etc. Any of these can be changed without an issue - just let me know what is preferred.
Added: