-
Notifications
You must be signed in to change notification settings - Fork 775
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
refactor simple game server #3817
base: main
Are you sure you want to change the base?
refactor simple game server #3817
Conversation
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Build Succeeded 👏 Build Id: 72700498-8b76-4a75-8bea-5879ed4cd3a9 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks good! Some nits, and some other ideas to try.
@@ -0,0 +1,331 @@ | |||
package main |
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.
Apache licence header please.
func ready(s *sdk.SDK) { | ||
err := s.Ready() | ||
if err != nil { | ||
log.Fatalf("Could not send ready message") | ||
} | ||
} |
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 I am noting that we have a pattern that looks like the above quite a bit - execute a function on the SDK, if it fails, log.Fatalf(...)
Not 100% sure how useful this would be - as it may only apply in a few places, but something like:
func runSDK(f func() err, failMsg string) {
if err := f(); err != nil {
log.Fatalf(failMsg + ": %v", err)
}
}
Then this function could pretty much go away and you could do inline in handleReady
func handleReady(s *sdk.SDK, parts []string, _ ...context.CancelFunc) (response string, addACK bool, responseError error) {
runSDK(func() err { return s.Ready() }, "Could not send ready message")
}
Since it's a closure, it could also handle anything taking in a series of arguments.
Where it goes fall over is if a SDK function returns several return values -- so it may not be worth it. But figured I'd show you my general idea, and see if it made sense -- save writing two functions for each command.
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 I am noting that we have a pattern that looks like the above quite a bit - execute a function on the SDK, if it fails,
log.Fatalf(...)
Not 100% sure how useful this would be - as it may only apply in a few places, but something like:
func runSDK(f func() err, failMsg string) { if err := f(); err != nil { log.Fatalf(failMsg + ": %v", err) } }Then this function could pretty much go away and you could do inline in
handleReady
func handleReady(s *sdk.SDK, parts []string, _ ...context.CancelFunc) (response string, addACK bool, responseError error) { runSDK(func() err { return s.Ready() }, "Could not send ready message") }Since it's a closure, it could also handle anything taking in a series of arguments.
Where it goes fall over is if a SDK function returns several return values -- so it may not be worth it. But figured I'd show you my general idea, and see if it made sense -- save writing two functions for each command.
Ahan! I got it. I have added all the sdk functions inside the handlers.
@@ -0,0 +1,371 @@ | |||
package main |
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.
rename suggestion: handlers.go
@@ -0,0 +1,331 @@ | |||
package main |
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.
Rename suggestions : sdk.go
(don't love it) - but helpers.go
is a bit ambiguous. sdk.go
gives some idea that this is where the sdk gets called.
Either that or pull it all into handers.go
and put the SDK command under each handler? (or if need be, make it inline?)
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.
Rename suggestions :
sdk.go
(don't love it) - buthelpers.go
is a bit ambiguous.sdk.go
gives some idea that this is where the sdk gets called.Either that or pull it all into
handers.go
and put the SDK command under each handler? (or if need be, make it inline?)
I have pulled everything inside handlers.go
. sdk.go isn't required.
Thank you so much for your guidance.
8b83ca3
to
7a03c71
Compare
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Build Succeeded 👏 Build Id: c54e02bb-849e-45cd-83ba-9885bf4fa113 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Will need an increment on the version number:
version := 0.32 |
Will also need to update all the references to simple-game-server - but may want to test locally with some e2e tests before updating everything as we'll need to push a new version to prod.
@@ -0,0 +1,581 @@ | |||
// Copyright 2020 Google LLC All Rights Reserved. |
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.
// Copyright 2020 Google LLC All Rights Reserved. | |
// Copyright 2024 Google LLC All Rights Reserved. |
7a03c71
to
253638e
Compare
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
I had tested few functionalities mentioned in this file: https://github.com/googleforgames/agones/blob/main/test/e2e/gameserver_test.go#L717. Everything works perfectly to me. I was communicated with gameserver (https://agones.dev/site/docs/getting-started/create-gameserver/#3-connect-to-the-gameserver) the response was as expected. Please, let me know what else i am missing for e2e test. |
Build Failed 😱 Build Id: 2eb3c057-1ad5-4c69-9e20-40a830f1b884 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
253638e
to
7c8810f
Compare
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Build Failed 😱 Build Id: bdcd983b-76c0-4aba-be39-cbf32599be5f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Hi @markmandel,
Could you please guide why this is failing. There are other functions also failing in allocator_test.go file. |
It's possible it's failing because the simple game server image 0.33 is not in the Agones artifact registry yet. You can test if that's the issue locally by pushing the simple game server image to your dev project's artifact registry https://github.com/googleforgames/agones/blob/main/examples/simple-game-server/Makefile and temporarily changing the image to your repo image agones/test/e2e/framework/framework.go Line 153 in bff72b1
|
As part of this PR, try out the approach(es) in #3836 and see if they work for you 👍🏻 |
7c8810f
to
636f807
Compare
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Build Failed 😱 Build Id: ded8d76c-7d0f-4290-bf8e-6b840b5f906b To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
636f807
to
3ee108f
Compare
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Build Failed 😱 Build Id: 93ea63f6-853d-4104-a5af-6fa11f8b1d70 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
@@ -64,7 +64,7 @@ KIND_PROFILE ?= agones | |||
KIND_CONTAINER_NAME=$(KIND_PROFILE)-control-plane | |||
|
|||
# Game Server image to use while doing end-to-end tests | |||
GS_TEST_IMAGE ?= us-docker.pkg.dev/agones-images/examples/simple-game-server:0.32 | |||
GS_TEST_IMAGE ?= us-central1-docker.pkg.dev/agones-ashutoshnsingh/example/simple-game-server:0.33-dev-linux-amd64 |
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 expect this won't work, since the agones-images
project doesn't have access to your artifact registry.
You could make it public if yo u desired though 😄 at least temporarily.
3ee108f
to
e2f17f4
Compare
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Build Failed 😱 Build Id: cbe7023a-7c96-4571-870d-a23ed4285820 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
What type of PR is this? This is a refactor of existing code of simple game server present in examples folder.
What this PR does / Why we need it:
Which issue(s) this PR fixes: #3813
Closes #3813
Special notes for your reviewer: