Docs: Lifecycle Management of Counters and Lists in REST #3560
Conversation
|
Build Succeeded 👏 Build Id: 7fbcf4ed-b79d-410d-9dd3-ce2fb9273b5d 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:
|
| ##### Example | ||
|
|
||
| ```bash | ||
| curl -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/counters/conformanceTestCounter |
There was a problem hiding this comment.
We should probably mention in the example that conformanceTestCounter is predefined in the local SDK Server, or go with the more generic example of {name} or {counterName}.
There was a problem hiding this comment.
I have included the Local Development Setup section for Counters and Lists in this document. Could you please review it and share your thoughts?
There was a problem hiding this comment.
I would add an explainer under each example that uses a key, something like:
In this example, we retrieve the counter under the key "conformanceTestCounter" to make it extra clear.
Since people don't always read the top level docs 😃
As an improvement to the docs and likely the local sdk experience, @igooch I didn't catch this earlier, but any objection to changing:
agones/pkg/sdkserver/localsdk.go
Lines 147 to 158 in 1e78d88
To using some more generic keys? Maybe players for the list and rooms for the count?
We don't actually use these values for conformance tests anyway, so this might be a bit confusing, and I think would make examples easier to understand. WDYT? If so, we can do this in a separate PR (and will need to feature shortcode the new docs) - if you like we can file a new issue.
There was a problem hiding this comment.
I would add an explainer under each example that uses a key, something like:
In this example, we retrieve the counter under the key "conformanceTestCounter" to make it extra clear.
I have included this information in a Note section located under both the Counters and Lists titles, following the Warning section. This will avoid the repetition right? WDYT?
To using some more generic keys? Maybe players for the list and rooms for the count?
Do you want to replace "conformanceTestCounter": {Count: 1, Capacity: 10}} with "rooms": {Count: 1, Capacity: 10}} and "conformanceTestList": {Values: []string{"test0", "test1", "test2"}, Capacity: 100}} with "players": {Values: []string{"player1", "player2", "player3"}, Capacity: 100}}?
There was a problem hiding this comment.
We don't actually use these values for conformance tests anyway
I'm silly - of course we use this for conformance tests 🤦🏻 . Ignore that comment. But I am wondering if we change the key anyway, to make it easier for end users, and make our conformance testing match. WDYT?
For these docs, we don't have to conform to the local SDK for the example, so I think we can change the key to something more generic like I suggest above. I'd like to get agreement on the code changes from @igooch before we make the code changes though to the local sdk server (and either way, we should handle that in a separate PR).
Sound good?
There was a problem hiding this comment.
My personal preference is to have the example match what we have in the localSDKServer for a better user experience. That way the user can run the example code and it works without them needing to make changes. My general gripe with generic examples is that they implicitly assume the user knows what and where to make changes without any accompanying instructions. That being said the key "conformanceTestCounter" probably doesn't mean anything to the user either, so I'd rename the keys along the lines of "TestCounterKey", "TestPlayerCounter", etc.
I'm also fine leaving the examples more generic here, as long as we have a step by step guide elsewhere that is more accessible to the user that needs more context on how and where to make changes.
There was a problem hiding this comment.
My personal preference is to have the example match what we have in the localSDKServer for a better user experience.
100% agreed - no disagreement there - sounds like we're just talking about what names to give keys. This change I've suggested above I would like to see as an interim step until we can align it again next release with a future PR to update the local sdk server and conformance tests.
That being said the key "conformanceTestCounter" probably doesn't mean anything to the user either, so I'd rename the keys along the lines of "TestCounterKey", "TestPlayerCounter", etc.
So i would argue that both "test", "key", and "counter" are redundant in the name, and just confuse things a bit more, and it's easier to give a real world use case for the local SDK user that is easy to understand (and will align with the eventual integration pattern docs I'll write - primarily one for "rooms" and one for "player tracking" - as well as the examples we have in the yaml files etc.).
That being said, I would be super happy if as part of the description for each method, there is a reminder for what the key is that is being used, since reminders are great and redundancy in docs is definitely not a bad thing, I'll write up a small suggestion, lemme know what you think.
There was a problem hiding this comment.
Are we sticking with rooms and players for now and considering changes later if needed?
|
Build Succeeded 👏 Build Id: 575f1965-3219-4c6c-ba1a-bcb7e010db0b 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:
|
| ##### Example | ||
|
|
||
| ```bash | ||
| curl -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/counters/conformanceTestCounter |
There was a problem hiding this comment.
I would add an explainer under each example that uses a key, something like:
In this example, we retrieve the counter under the key "conformanceTestCounter" to make it extra clear.
Since people don't always read the top level docs 😃
As an improvement to the docs and likely the local sdk experience, @igooch I didn't catch this earlier, but any objection to changing:
agones/pkg/sdkserver/localsdk.go
Lines 147 to 158 in 1e78d88
To using some more generic keys? Maybe players for the list and rooms for the count?
We don't actually use these values for conformance tests anyway, so this might be a bit confusing, and I think would make examples easier to understand. WDYT? If so, we can do this in a separate PR (and will need to feature shortcode the new docs) - if you like we can file a new issue.
|
Build Succeeded 👏 Build Id: a55590d0-3c86-4ead-8013-5f06f62d5e5a 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:
|
| ##### Example | ||
|
|
||
| ```bash | ||
| curl -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/counters/conformanceTestCounter |
There was a problem hiding this comment.
We don't actually use these values for conformance tests anyway
I'm silly - of course we use this for conformance tests 🤦🏻 . Ignore that comment. But I am wondering if we change the key anyway, to make it easier for end users, and make our conformance testing match. WDYT?
For these docs, we don't have to conform to the local SDK for the example, so I think we can change the key to something more generic like I suggest above. I'd like to get agreement on the code changes from @igooch before we make the code changes though to the local sdk server (and either way, we should handle that in a separate PR).
Sound good?
|
I think I've made all the changes you suggested.👍🏻 |
|
Build Succeeded 👏 Build Id: a9152c79-5494-471f-ad46-1dadd9412128 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.
Some minor pieces, but otherwise, LGTM! Also waiting on approval from @igooch as well 👍🏻
|
All the changes are in place.👍🏻 |
|
Build Succeeded 👏 Build Id: 93f7cf90-31b8-4d82-a9cd-7ac9c1f0c7cc 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:
|
| For your own Counter REST requests, replace the value `rooms` with your own key in the path. | ||
|
|
||
| ##### Alpha: GetCounter | ||
| This function retrieves a specified counter by its name and returns its information. |
There was a problem hiding this comment.
| This function retrieves a specified counter by its name and returns its information. | |
| This function retrieves a specified counter by its key, `rooms`, and returns its information. |
@igooch @Kalaiselvi84 how do you feel about this suggestion? (and then repeating it through all the below REST calls as well) -- then it's doubly reinforced what the key is, both above and inline in the description?
There was a problem hiding this comment.
I've made the suggested changes. Ivy, wdyt?
| ``` | ||
|
|
||
| ##### Alpha: UpdateCounter | ||
| This function updates the specified counter's properties, such as its count and capacity, and returns the updated counter details. |
There was a problem hiding this comment.
| This function updates the specified counter's properties, such as its count and capacity, and returns the updated counter details. | |
| This function updates the counter with the key `rooms`'s properties, such as its count and capacity, and returns the updated counter details. |
Just another example of the change style.
There was a problem hiding this comment.
Would it be a good idea if we rephrase like this?
This function updates the properties of the counter with the key rooms, such as its count and capacity, and returns the updated counter details.
|
Build Failed 😱 Build Id: 2845ea46-9e71-4c9e-a043-15cd0ff6a729 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 👏 Build Id: 9924baa4-352d-43e0-8b02-543feb1785a9 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: d41667c5-8e7d-4173-bbfe-79d2e98242db 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:
|
| ``` | ||
|
|
||
| ##### Alpha: UpdateList | ||
| This function updates the specified list's properties, such as its capacity and values, returns the updated list details. This will overwrite all existing List.Values with the update list request values. Use addValue or removeValue for modifying the List.Values field. |
There was a problem hiding this comment.
Can we please do players in this List section like we did for rooms above, and then I think this PR is good to go.
| This function updates the specified list's properties, such as its capacity and values, returns the updated list details. This will overwrite all existing List.Values with the update list request values. Use addValue or removeValue for modifying the List.Values field. | |
| This function updates the list's properties with the key `players`, such as its capacity and values, returns the updated list details. This will overwrite all existing List.Values with the update list request values. Use addValue or removeValue for modifying the List.Values field. |
| ``` | ||
|
|
||
| ##### Alpha: AddListValue | ||
| This function adds a new value to a specified list and returns the list with this addition. |
There was a problem hiding this comment.
| This function adds a new value to a specified list and returns the list with this addition. | |
| This function adds a new value to a list with the key `players` and returns the list with this addition. |
| ``` | ||
|
|
||
| ##### Alpha: RemoveListValue | ||
| This function removes a value from a given list and returns updated list. |
There was a problem hiding this comment.
| This function removes a value from a given list and returns updated list. | |
| This function removes a value from the list with the key `players` and returns updated list. |
|
Build Succeeded 👏 Build Id: be91ee68-94b5-4238-8f54-b400e50ec67d 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: c6df7750-16e1-4ef1-8401-ef2918bc4fa7 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 documentation
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #
Special notes for your reviewer: