-
Notifications
You must be signed in to change notification settings - Fork 817
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
Alpha SDK and example for Node.js (Player tracking) #1658
Conversation
/assign @markmandel |
Build Failed 😱 Build Id: 81ebcefa-a4e7-4bf3-8f00-610f7f4261bd To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 8840c494-0742-4c9f-9daf-ab5c127648f5 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
# Conflicts: # examples/nodejs-simple/package-lock.json # examples/nodejs-simple/package.json
Build Failed 😱 Build Id: 61d95d49-1ec7-405e-9692-122f84525004 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: defb51e1-5003-4f33-9123-2db8d328b619 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 3f59cdfa-a12f-4d54-9ca5-6bbc6fc67b15 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:
|
Just touching base, to see if this work is still ongoing? |
I can take another look this weekend and still mulling over what the canonical approach would be. Why I chose this implementation is that there is an SDK class which has an interface of all available operations and once imported/required it is either a regular SDK or an alpha SDK. The alternative is that it is an SDK which has alpha property/method behind which the alpha features are located. I hadn't considered this and it is more of a composition pattern vs. my initial inheritance approach. Random thoughts:
Perhaps overthinking but it would be useful to have a pattern to follow if we continue to roll out alpha and beta features. |
This is how the Go SDK is working.
Beta would be it's own implementation. So no inheritance required.
Only the new. We can break Alpha (and beta) commands -- there is no guarantee of stability.
That was the idea behind SDK.Alpha().Command() to make that clear.
I can't this being a required at this stage, if so, we can handle that at that time. Does that help? |
Build Failed 😱 Build Id: 77e939d3-0f11-4441-ba3a-77cbee27c9d2 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 2a24a2c2-1803-4ac6-8428-9f9af640a423 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 34c97545-b001-4fe4-a586-cfaefed5a801 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.
This is looking great - just have a minor question and a couple of small nits (2019->2020), but otherwise, i think this is good to go 👍
Co-authored-by: Mark Mandel <markmandel@google.com>
Build Succeeded 👏 Build Id: 0abc2e92-79b2-408e-ad30-f0dbd29f8f88 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 Succeeded 👏 Build Id: 4d04f0e4-a159-448e-a811-85769d764420 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:
|
Just a reminder: This is the only change that is left - this needs to be moved to |
Co-authored-by: Mark Mandel <markmandel@google.com>
Sorry I missed that one when I clicked the handy 'apply suggestion' button before! |
Build Succeeded 👏 Build Id: f01274fb-9e47-4aee-9c8f-012940a3b311 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.
APPROVED 🔥
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 feature
What this PR does / Why we need it:
Adds a Node.js alpha SDK that extends the base SDK with the alpha features of Player Tracking. Also adds an example server to showcase the SDK.
Once the features change stage then they can be moved to the main SDK with the tests and leave the framework for future alpha features.
Special notes for your reviewer:
Relates to #1033
Branched from #1657