-
Notifications
You must be signed in to change notification settings - Fork 810
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
Conversation
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:
|
pkg/sdkserver/localsdk.go
Outdated
@@ -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 == "" { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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:
|
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perfect 👍🏻
pkg/sdkserver/localsdk.go
Outdated
@@ -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{ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Build Failed 😱 Build Id: 081f7135-55ae-435a-8bf0-79e32939e89a To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
There was a problem hiding this 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.
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:
|
What type of PR is this?
/kind documentation
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #3630
Special notes for your reviewer: