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

SDK Matrices for NodeJS, C#, CPP, REST #3548

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

Kalaiselvi84
Copy link
Contributor

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation

/kind feature

/kind hotfix
/kind release

What this PR does / Why we need it:
Included SDK functionality table for NodeJS, CSharp, REST, and CPP.

Which issue(s) this PR fixes:

Work on #2716

Special notes for your reviewer:

@github-actions github-actions bot added kind/feature New features for Agones size/S labels Dec 18, 2023
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 7daaf50b-1995-4d8b-b06d-0d10c8bb6c47

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@gongmax
Copy link
Collaborator

gongmax commented Dec 18, 2023

one failed unit test

--- FAIL: TestControllerGameServersNodeState (10.00s)
    controller_test.go:442: 
        	Error Trace:	/go/src/agones.dev/agones/pkg/metrics/controller_test.go:442
        	Error:      	Condition never satisfied
        	Test:       	TestControllerGameServersNodeState
    controller_test.go:74: 
        	Error Trace:	/go/src/agones.dev/agones/pkg/metrics/controller_test.go:74
        	            				/go/src/agones.dev/agones/pkg/metrics/controller_test.go:460
        	Error:      	Expected value not to be nil.
        	Test:       	TestControllerGameServersNodeState
        	Messages:   	No metric found with name: gameservers_node_count
{"message":"Wait for cache sync","severity":"info","time":"2023-12-18T19:04:29.818819854Z"}
FAIL

Retrying...

Copy link
Collaborator

@gongmax gongmax left a comment

Choose a reason for hiding this comment

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

Looking at the doc for other SDKs, the function matrix is at the top. Eg https://agones.dev/site/docs/guides/client-sdks/rust/#sdk-functionality. Can you follow the same pattern?

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 893aa74f-417c-4c9d-9b47-445a8c0ab3fa

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:

  • git fetch https://github.com/googleforgames/agones.git pull/3548/head:pr_3548 && git checkout pr_3548
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-0493526-amd64

@Kalaiselvi84
Copy link
Contributor Author

Looking at the doc for other SDKs, the function matrix is at the top. Eg https://agones.dev/site/docs/guides/client-sdks/rust/#sdk-functionality. Can you follow the same pattern?

I've made changes to NodeJS, C#, and CPP. Could you please confirm that the changes I have made are the correct way for REST?

@Kalaiselvi84
Copy link
Contributor Author

Kalaiselvi84 commented Dec 18, 2023

@igooch Could you please confirm if the following is the correct approach for the REST SDK functionality

| Player           | GetPlayerCapacity               | ✔️          |
| Player           | SetPlayerCapacity               | ✔️          |
| Player           | PlayerConnect                     | ✔️          |
| Player           | GetConnectedPlayers         | ✔️          |
| Player           | IsPlayerConnected              | ✔️          |
| Player           | GetPlayerCount                   | ✔️          |
| Player           | PlayerDisconnect                | ✔️          |
| Counter          | UpdateCounter                   | ✔️          |
| Counter          | GetCounter                      | ✔️          |
| List             | UpdateList                       | ✔️          |
| List             | GetList                             | ✔️          |
| List             | AddListValue                   | ✔️          |
| List             | RemoveListValue            | ✔️          |

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 85920a4b-9124-4d86-9cc9-c9aea9375c0e

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@gongmax
Copy link
Collaborator

gongmax commented Dec 18, 2023

Failed e2e test:

gameserverallocation_test.go:1417: 
VERBOSE:         	Error Trace:	/go/src/agones.dev/agones/test/e2e/gameserverallocation_test.go:1417
VERBOSE:         	Error:      	Not equal: 
VERBOSE:         	            	expected: 100
VERBOSE:         	            	actual  : 99
VERBOSE:         	Test:       	TestGameServerAllocationDuringMultipleAllocationClients
VERBOSE: --- FAIL: TestGameServerAllocationDuringMultipleAllocationClients (41.23s)


| Area | Action | Implemented |
|-----------|--------------------------------------------------------|-------------|
| Counters | GET /v1alpha1/counters/{name} | ✔️ |
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part looks good to me. Do we want to add rows for other funcations like the other SDKs?

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 the same question 🙋‍♀️

@igooch Looking forward to your inputs

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not entirely sure what Mark envisioned here, but I would start similarly to the "Action" column like we have in https://agones.dev/site/docs/guides/client-sdks/go/. Then below we can include the Path, Method, Body for each action. The actions for Counters areGetCounter UpdateCounter and for Lists GetList UpdateList AddListValue RemoveListValue.

I would also include the existing actions "Ready", "Health" etc. that are named below in this doc. (The generated client API is at agones/test/sdk/restapi/swagger/api_sdk.go if you want to see the method names.)

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 13ca924e-b844-4d3e-bdec-ce9eaac7974b

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:

  • git fetch https://github.com/googleforgames/agones.git pull/3548/head:pr_3548 && git checkout pr_3548
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-e61e1a4-amd64


| Area | Action | Implemented |
|-----------|--------------------------------------------------------|-------------|
| Counters | GET /v1alpha1/counters/{name} | ✔️ |
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not entirely sure what Mark envisioned here, but I would start similarly to the "Action" column like we have in https://agones.dev/site/docs/guides/client-sdks/go/. Then below we can include the Path, Method, Body for each action. The actions for Counters areGetCounter UpdateCounter and for Lists GetList UpdateList AddListValue RemoveListValue.

I would also include the existing actions "Ready", "Health" etc. that are named below in this doc. (The generated client API is at agones/test/sdk/restapi/swagger/api_sdk.go if you want to see the method names.)

| Lifecycle | Allocate | ✔️ |
| Lifecycle | Shutdown | ✔️ |
| Configuration | GameServer | ✔️ |
| Configuration | Watch | ✔️ |
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
| Configuration | Watch | ✔️ |
| Configuration | WatchGameServer | ✔️ |

Comment on lines 167 to 173
| Player Tracking | GetConnectedPlayers | ✔️ |
| Player Tracking | GetPlayerCapacity | ✔️ |
| Player Tracking | GetPlayerCount | ✔️ |
| Player Tracking | IsPlayerConnected | ✔️ |
| Player Tracking | PlayerConnect | ✔️ |
| Player Tracking | PlayerDisconnect | ✔️ |
| Player Tracking | SetPlayerCapacity | ✔️ |
Copy link
Collaborator

@igooch igooch Dec 19, 2023

Choose a reason for hiding this comment

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

I don't see this implemented in agones/sdks/cpp/src/agones/sdk.cc

| Lifecycle | Reserve | ✔️ |
| Lifecycle | Allocate | ✔️ |
| Lifecycle | Shutdown | ✔️ |
| Configuration | GameServer | ✔️ |
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
| Configuration | GameServer | ✔️ |
| Configuration | GetGameServer | ✔️ |

| Lifecycle | Allocate | ✔️ |
| Lifecycle | Shutdown | ✔️ |
| Configuration | GameServer | ✔️ |
| Configuration | Watch | ✔️ |
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
| Configuration | Watch | ✔️ |
| Configuration | WatchGameServer | ✔️ |

| Lifecycle | Reserve | ✔️ |
| Lifecycle | Allocate | ✔️ |
| Lifecycle | Shutdown | ✔️ |
| Configuration | GameServer | ✔️ |
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
| Configuration | GameServer | ✔️ |
| Configuration | GetGameServer | ✔️ |

| Lifecycle | Allocate | ✔️ |
| Lifecycle | Shutdown | ✔️ |
| Configuration | GameServer | ✔️ |
| Configuration | Watch | ✔️ |
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
| Configuration | Watch | ✔️ |
| Configuration | WatchGameServer | ✔️ |

@igooch
Copy link
Collaborator

igooch commented Dec 19, 2023

Failed e2e test:

gameserverallocation_test.go:1417: 
VERBOSE:         	Error Trace:	/go/src/agones.dev/agones/test/e2e/gameserverallocation_test.go:1417
VERBOSE:         	Error:      	Not equal: 
VERBOSE:         	            	expected: 100
VERBOSE:         	            	actual  : 99
VERBOSE:         	Test:       	TestGameServerAllocationDuringMultipleAllocationClients
VERBOSE: --- FAIL: [3552](https://github.com/googleforgames/agones/pull/3552) (41.23s)

This is a relatively frequent flake. We should probably open an issue to reduce the flakiness.

@Kalaiselvi84 Kalaiselvi84 marked this pull request as ready for review December 19, 2023 01:59
@Kalaiselvi84
Copy link
Contributor Author

@igooch I have completed the review changes as suggested by you

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ef111394-7133-46ab-87ce-bd795bc1991f

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:

  • git fetch https://github.com/googleforgames/agones.git pull/3548/head:pr_3548 && git checkout pr_3548
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-5510b04-amd64

Copy link
Collaborator

@gongmax gongmax left a comment

Choose a reason for hiding this comment

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

Only one small comment: can we move these SDK function sections to the top of each doc? similar as the other docs eg: https://agones.dev/site/docs/guides/client-sdks/unreal/#sdk-functionality

@gongmax gongmax enabled auto-merge (squash) December 19, 2023 19:30
@Kalaiselvi84
Copy link
Contributor Author

Only one small comment: can we move these SDK function sections to the top of each doc?

Relocated it to the top of each file.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ec227071-6e03-47d1-91f2-e2f9b2b80155

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:

  • git fetch https://github.com/googleforgames/agones.git pull/3548/head:pr_3548 && git checkout pr_3548
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-b003807-amd64

@gongmax gongmax merged commit e20368b into googleforgames:main Dec 19, 2023
4 checks passed
@Kalaiselvi84 Kalaiselvi84 deleted the sdk-matrixes branch March 15, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New features for Agones size/M size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants