Skip to content
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

Docs: Lifecycle Management of Counters and Lists in REST #3560

Merged
merged 14 commits into from
Jan 18, 2024
95 changes: 93 additions & 2 deletions site/content/en/docs/Guides/Client SDKs/rest.md
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,97 @@ Apply an Annotation with the prefix "agones.dev/sdk-" to the backing `GameServer
curl -d '{"key": "foo", "value": "bar"}' -H "Content-Type: application/json" -X PUT http://localhost:${AGONES_SDK_HTTP_PORT}/metadata/annotation
```

### Counters

{{< alpha title="Counters" gate="Counters" >}}

#### Alpha: GetCounter
This function retrieves a specified counter by its name and returns its information.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the suggested changes. Ivy, wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!


##### Example

```bash
curl -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/counters/conformanceTestCounter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have included the Local Development Setup section for Counters and Lists in this document. Could you please review it and share your thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
// Adding test Counter and List for the conformance tests (not nil for LocalSDKServer tests)
if l.gs.Status.Counters == nil {
l.gs.Status.Counters = map[string]*sdk.GameServer_Status_CounterStatus{
"conformanceTestCounter": {Count: 1, Capacity: 10}}
}
if l.gs.Status.Lists == nil {
l.gs.Status.Lists = map[string]*sdk.GameServer_Status_ListStatus{
"conformanceTestList": {Values: []string{"test0", "test1", "test2"}, Capacity: 100}}
}
}

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}}?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@igooch igooch Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sticking with rooms and players for now and considering changes later if needed?

```

Response:
```json
{"name":"conformanceTestCounter", "count":"1", "capacity":"10"}
```

#### Alpha: UpdateCounter
This function updates the specified counter's properties, such as its count and capacity, and returns the updated counter details.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That reads better! Nice.


##### Example

```bash
curl -d '{"count": "5", "capacity": "11"}' -H "Content-Type: application/json" -X PATCH http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/counters/conformanceTestCounter
```

Response:
```json
{"name":"conformanceTestCounter", "count":"5", "capacity":"11"}
```

### Lists

{{< alpha title="Lists" gate="Lists" >}}

#### Alpha: GetList
This function retrieves a specific list identified by its name, returns the list's information.

##### Example
```bash
curl -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/lists/conformanceTestList
```

Response:
```json
{"name":"conformanceTestList", "capacity":"100", "values":["test0", "test1", "test2"]
```

#### Alpha: UpdateList
This function updates the specified list's properties, such as its capacity and values, returns the updated list details.

##### Example

```bash
curl -d '{"capacity": "120", "values": ["test3", "test4"]}' -H "Content-Type: application/json" -X PATCH http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/lists/conformanceTestList
```

Response:
```json
{"name":"conformanceTestList", "capacity":"120", "values":["test3", "test4"]}
```

#### Alpha: AddListValue
This function adds a new value to a specified list and returns the list with this addition.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.


##### Example

```bash
curl -d '{"value": "test9"}' -H "Content-Type: application/json" -X POST http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/lists/conformanceTestList:addValue
```

Response:
```json
{"name":"conformanceTestList", "capacity":"120", "values":["test3", "test4", "test9"]}
```

#### Alpha: RemoveListValue
This function removes a value from a given list and returns updated list.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.


##### Example

```bash
curl -d '{"value": "test3"}' -H "Content-Type: application/json" -X POST http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/lists/conformanceTestList:removeValue
```

Response:
```json
{"name":"conformanceTestList", "capacity":"120", "values":["test4", "test9"]}
```

### Player Tracking

{{< alpha title="Player Tracking" gate="PlayerTracking" >}}
Expand Down Expand Up @@ -305,6 +396,8 @@ Response:
This function retrieves the current player count.
This is always accurate from what has been set through this SDK, even if the value has yet to be updated on the GameServer status resource.

##### Example

```bash
curl -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/alpha/player/count
```
Expand All @@ -314,8 +407,6 @@ Response:
{"count":"2"}
```

##### Example

#### Alpha: IsPlayerConnected

This function returns if the playerID is currently connected to the GameServer. This is always accurate from what has
Expand Down