agones
agones copied to clipboard
googleforgames
Nodejs counters and lists
What type of PR is this? /kind feature
What this PR does / Why we need it:
- Counts SDK
- Lists SDK
- Unit tests
Which issue(s) this PR fixes: Closes #3645
Special notes for your reviewer:
- The example would require a local game server created with a specifically named counter so unsure whether to continue with that
- Conformance tests are not done yet although there seems to be overlap with the Node.js example, however the example does a lot more
Build Failed :scream:
Build Id: cd4e2795-51eb-4952-82ad-43e4d70abc71
To get permission to view the Cloud Build view, join the agones-discuss Google Group.
It is a bit difficult to work with the CounterUpdateRequest in JavaScript due to the type google.protobuf.Int64Value instead of int64
From alpha.protoc
// A representation of a Counter Update Request.
message CounterUpdateRequest {
option (google.api.resource) = {
type: "agones.dev/CounterUpdateRequest"
pattern: "counterUpdateRequests/{counterUpdateRequest}"
};
// The name of the Counter to update
string name = 1;
// The value to set the Counter Count
google.protobuf.Int64Value count = 2;
// The value to set the Counter Capacity
google.protobuf.Int64Value capacity = 3;
// countDiff tracks if a Counter Update Request is CountIncrement (positive), CountDecrement
// (negative), 0 if a CountSet or CapacitySet request
int64 countDiff = 4;
}
I got it working with the following but this seems like an internal type. It's a bit hacky to require/import at a nested path because it is not a supported export of the library, so will break if the library gets refactored#
const jspbWrappers = require('google-protobuf/google/protobuf/wrappers_pb');
...
const capacity = new jspbWrappers.Int64Value();
capacity.setValue(amount);
I'm wondering why we don't use int64 instead as we use this in other places in the SDK (edited)
I'm wondering why we don't use int64 instead as we use this in other places in the SDK
The google.protobuf.Int64Value is there so that it will be a nullable object rather than a primitive type that has a default value (0 in the case of Golang). In the case of updating the Count or Capacity 0 is a meaningful value, so we don't want 0 to be the default. 0 for the "diff" isn't meaningful, since that's just adding the diff to the Count.
Thank you. Reading around, it also seems that google.protobuf.Int64Value is a recommended way to use a nullable int, so I wonder if there's a better way to import wrappers_pb in JavaScript. It may just be an issue with the library not exporting this useful type the correct way.
For my curiosity I was wondering if we could use a wrapper type e.g. the Count that we use in SetPlayerCapacity?
// Store a count variable.
message Count {
int64 count = 1;
}
Also did we consider using -1 as a special value for count and capacity in the update to mean unspecified, as these would be invalid values? I don't know if these could be set by default to -1, so not setting them would leave them at -1.
So we use these standard wrappers all over the place - I asked the gRPC team what the canonical way to import the standard wrappers is and will get back to you.
Feedback I got was basically "if const jspbWrappers = require('google-protobuf/google/protobuf/wrappers_pb'); works, then that is the correct solution, and why that is there."
So I think we should move forward with this approach 👍🏻
Thank you and so it seems the library is missing some code as we should be able to do the following so a library user does not need to know the internal path
const jspbWrappers = require('google-protobuf').jspbWrappers
Or in the more modern format
const {jspbWrappers} = require('google-protobuf')
But yes, can go ahead with the current solution
https://github.com/protocolbuffers/protobuf-javascript/issues/197
That's a new one
ERROR: (gcloud.container.clusters.get-credentials) ResponseError: code=404, message=Not found: projects/agones-images/locations/asia-east1/clusters/standard-e2e-test-cluster-1-26.
You can ignore that.
Restarting test.
Build Failed :scream:
Build Id: 14d2f577-ac55-42ea-a735-4d181de07309
To get permission to view the Cloud Build view, join the agones-discuss Google Group.
Oooh, you will need to rebase against main - there is no 1.26 cluster anymore.
@steven-supersolid we're updating the SDK, so that most of the methods that return (bool, error) are now simply returning (error). I updated the issue #3645. (Discussion: #3581#discussion_r1505135182). Also tracked in #3737.
Great, I think that is closer to the rest of the API. Will update
@steven-supersolid we moved Counts and Lists from Alpha to Beta #3806. This is the last major expected change (until it goes into stable, but I imagine that will be several more releases away).
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Build Failed :scream:
Build Id: baf65795-fc80-450a-aab1-3f854d6fcac9
To get permission to view the Cloud Build view, join the agones-discuss Google Group.
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
Build Failed :scream:
Build Id: bc348b7c-98a7-4bbf-94f7-47dc55230ebb
To get permission to view the Cloud Build view, join the agones-discuss Google Group.
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
Build Failed :scream:
Build Id: 83334b65-8146-4cdd-b98c-f02f81b9834c
To get permission to view the Cloud Build view, join the agones-discuss Google Group.
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
Build Failed :scream:
Build Id: 5e6e21d7-437b-460b-8525-5b9fa6128756
To get permission to view the Cloud Build view, join the agones-discuss Google Group.
Tests are failing due new spys failing on the alpha client since merging in main and these also fail locally so perhaps client methods got renamed
It looks like it is due to removing Counts and Lists from alpha #3806 to beta. There is no beta protobuf file in JavaScript yet so need to generate one and update the sdk and tests
There's a make gen-all-sdk-grpc command in the build directory, but it looks like it's ineffective for nodejs. I opened a separate issue #3820 for the autogeneration not working.
https://github.com/googleforgames/agones/blob/1b06e945ec086d14ceffdb02ef818b768e206b6a/build/includes/sdk.mk#L60-L69
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
Build Failed :scream:
Build Id: df6eba49-e0b1-414a-8a65-b93b9eeb117a
To get permission to view the Cloud Build view, join the agones-discuss Google Group.
Once #3825 is done I'll merge main in and tests should pass I need to update the example code to use the new beta SDK
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.