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

Local SDK: Counters and Lists #3660

Merged
merged 5 commits into from Mar 1, 2024
Merged

Conversation

Kalaiselvi84
Copy link
Contributor

@Kalaiselvi84 Kalaiselvi84 commented Feb 17, 2024

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:

Which issue(s) this PR fixes:

Closes #3630

Special notes for your reviewer:

@github-actions github-actions bot added kind/documentation Documentation for Agones size/S labels Feb 17, 2024
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 1b7b8f5e-3440-4d18-b6b9-8bcc3b63436e

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/3660/head:pr_3660 && git checkout pr_3660
  • 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.39.0-dev-bbc57ec-amd64

@Kalaiselvi84 Kalaiselvi84 changed the title local SDK: counter and lists Local SDK: Counters and Lists Feb 21, 2024
@@ -146,11 +147,11 @@ func NewLocalSDKServer(filePath string, testSdkName string) (*LocalSDKServer, er
}
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 {
if l.gs.Status.Counters == nil && filePath == "" {
Copy link
Member

Choose a reason for hiding this comment

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

So this default setting of counters and lists should be part of defaultGs() rather than here.

But, down in this section, we should check if the feature flag is enabled and l.gs.Status.Counters == nil and if so, set it to an empty map.

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 changes to defaultGs() and NewLocalSDKServer() as you suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markmandel Could you kindly review the changes I've made and provide your feedback?

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e280bcc3-93b6-4cf0-a351-961722d1cc8f

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/3660/head:pr_3660 && git checkout pr_3660
  • 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.39.0-dev-bb4b4bc-amd64

if l.gs.Status.Counters == nil {
l.gs.Status.Counters = map[string]*sdk.GameServer_Status_CounterStatus{
"rooms": {Count: 1, Capacity: 10}}
l.gs.Status.Counters = make(map[string]*sdk.GameServer_Status_CounterStatus)
Copy link
Member

Choose a reason for hiding this comment

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

This is perfect 👍🏻

@@ -69,6 +70,12 @@ func defaultGs() *sdk.GameServer {
State: "Ready",
Address: "127.0.0.1",
Ports: []*sdk.GameServer_Status_Port{{Name: "default", Port: 7777}},
Counters: map[string]*sdk.GameServer_Status_CounterStatus{
Copy link
Member

Choose a reason for hiding this comment

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

So this should only be added when runtime.FeatureEnabled(runtime.FeatureCountsAndLists).

I expect you'll need to set the &sdk.GameServer to a temporary variable before returning.

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 conditionally added the Counters and Lists when the feature is enabled and the value of sdk.GameServer is stored in a temporary variable.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 081f7135-55ae-435a-8bf0-79e32939e89a

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

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Approved. This can come out of draft - looks like the e2e error is the old

    gameserverallocation_test.go:1417: 
        	Error Trace:	/go/src/agones.dev/agones/test/e2e/gameserverallocation_test.go:1417
        	Error:      	Not equal: 
        	            	expected: 100
        	            	actual  : 99
        	Test:       	TestGameServerAllocationDuringMultipleAllocationClients

Flake.

@Kalaiselvi84 Kalaiselvi84 marked this pull request as ready for review February 29, 2024 17:41
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 4aecc026-7e39-4553-93c1-185e7d953e44

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/3660/head:pr_3660 && git checkout pr_3660
  • 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.39.0-dev-7295f8d-amd64

@igooch igooch merged commit 42b1a92 into googleforgames:main Mar 1, 2024
4 checks passed
@Kalaiselvi84 Kalaiselvi84 deleted the i-3630 branch March 15, 2024 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Documentation for Agones size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom gameserver configuration not working on containerized local SDK
4 participants