Fix update counter to return correct values#4324
Conversation
|
Build Succeeded 🥳 Build Id: adeda24c-8a2b-42a2-be8d-4c7d6560f7fd 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: |
|
Add a test that validates this change ? |
Hi @aditya-shantanu I added a new test case to verify the changes. |
pkg/sdkserver/sdkserver_test.go
Outdated
| CountDiff: 15, | ||
| }}, | ||
| }, | ||
| want: agonesv1.CounterStatus{Count: int64(25), Capacity: int64(100)}, |
There was a problem hiding this comment.
nit: Change this to something other than the value before it so you are sure it got updated.
There was a problem hiding this comment.
Updated CountDiff to 7. The initial counter value remains 10, consistent with existing test cases.
|
Build Succeeded 🥳 Build Id: e566ba42-3582-488a-a274-d570bd21a3d5 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: 1586d105-bb6a-4b29-8f16-c3881fb4c0cb 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 for this! I would like to take this for a spin when integrated with our simple gameserver container we use for e2e testing. We don't have a great way of automating testing this, so it may need to be manual 🙃 |
markmandel
left a comment
There was a problem hiding this comment.
Don't actually need to test this with simple-game-server now I look at this code - just have one more follow up question. 👍🏻
There was a problem hiding this comment.
Pull Request Overview
This PR updates the UpdateCounter method to return the projected counter state instead of an empty beta.Counter{} object. This allows SDK clients to immediately see the expected state after their update request without needing to make a separate GetCounter call.
Key Changes:
- Modified
UpdateCounterto return projected counter state via newprojectCounterStatehelper function - Added test case "projected state returned" to verify the returned projected state matches expected values
- Test validates that a counter update with
CountDiff: 7correctly returns projectedCount: 17
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/sdkserver/sdkserver.go | Added projectCounterState function to calculate projected counter state and updated UpdateCounter to return projected state instead of empty Counter |
| pkg/sdkserver/sdkserver_test.go | Added "starfish" counter fixture and "projected state returned" test case to validate UpdateCounter returns correct projected state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Build Succeeded 🥳 Build Id: 0eec7b90-5683-45d8-85c3-8714eb95edab 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.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I think this is good to go. @igooch did you want to do another review? |
|
Build Failed 😭 Build Id: 56b57b3f-07df-4e45-98f1-139e872f2fb5 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
/gcbrun |
|
Build Succeeded 🥳 Build Id: 2a7ede8e-656c-4ed2-920a-1c9c59b32b48 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: |
* Fix update counter to return correct values * added testcase * updated test value * updated tests --------- Co-authored-by: Sivasankaran R <sivasankaranr@google.com>
What type of PR is this?
/kind bug
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #4220
Special notes for your reviewer: