Simple Game Server: Add \n to Counters and Lists Response #3589

Kalaiselvi84 · 2024-01-11T04:01:43Z

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 #3586

Special notes for your reviewer:

Kalaiselvi84 · 2024-01-11T04:11:18Z

I only changed GET_COUNTER_COUNT. Could you please confirm if this approach is correct? I'm unable to create a fleet with Counters and List, so I couldn't test it fully, but TestCounters passed with this change on my local.

agones-bot · 2024-01-11T04:31:59Z

Build Failed 😱

Build Id: a16f7069-4ac7-406e-8019-5ea68bc354fa

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

igooch

The build will fail until the example simple-game-server image has been updated (otherwise the tests pull from the old image and don't "see" any changes to the simple-game-server/main.go file.) Although we should wait to update the image until all the tests pass locally.

test/e2e/gameserver_test.go

examples/simple-game-server/main.go

agones-bot · 2024-01-12T22:01:43Z

Build Failed 😱

Build Id: b027e7a3-fd45-49e3-883f-195f4c93a437

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

Kalaiselvi84 · 2024-01-17T22:13:54Z

Tests TestCounters and TestLists have succeeded. Log details - https://gist.github.com/Kalaiselvi84/72d9950899deb9a00eb087e592ebf515

agones-bot · 2024-01-17T22:47:58Z

Build Failed 😱

Build Id: 3ee7fa76-94ab-4e6f-99e6-01a9ac3fd1a0

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

test/e2e/gameserver_test.go

igooch

Looks good. The only item that looked off was there appeared to be an empty error message for the GET_LIST_VALUES when the passed List does not exist.

~/agones/examples$ nc -u 34.168.36.96 7477
INCREMENT_COUNTER games 1
SUCCESS: true
GET_COUNTER_COUNT games
COUNTER: 2
DECREMENT_COUNTER sessions 1
SUCCESS: true
GET_COUNTER_COUNT sessions
COUNTER: 0
SET_COUNTER_COUNT sessions 10000
ERROR: false
SET_COUNTER_COUNT sessions 10
SUCCESS: true
GET_COUNTER_COUNT sessions
COUNTER: 10
GET_COUNTER_CAPACITY sessions
CAPACITY: 1000
SET_COUNTER_CAPACITY games 99
SUCCESS: true
GET_COUNTER_CAPACITY games
CAPACITY: 99
GET_LIST_CAPACITY rooms
CAPACITY: 333
SET_LIST_CAPACITY rooms 100
SUCCESS: true
GET_LIST_CAPACITY rooms
CAPACITY: 100
LIST_CONTAINS room1
ERROR: Invalid LIST_CONTAINS, should have 2 arguments
LIST_CONTAINS rooms room1
SUCCESS: true
LIST_CONTAINS rooms room_1
false
ACK: 
GET_LIST_LENGTH rooms
LENGTH: 3
GET_LIST_VALUES rooms
VALUES: room1,room2,room3
APPEND_LIST_VALUE rooms room4
SUCCESS: true
GET_LIST_VALUES abc
ERROR: 

GET_LIST_VALUES rooms
VALUES: room1,room2,room3,room4
DELETE_LIST_VALUE room1
ERROR: Invalid DELETE_LIST_VALUE, should have 2 arguments
DELETE_LIST_VALUE rooms room1
SUCCESS: true

Kalaiselvi84 · 2024-01-19T21:50:57Z

Right, also the response for the LIST_CONTAINS method should be prepended with ERROR: which is missing.

LIST_CONTAINS rooms room_1
Error: false

Will look into it.

igooch · 2024-01-19T22:02:05Z

Could you also update some typos in the readme too? They're missing the second "

agones/examples/simple-game-server/README.md

Lines 42 to 47 in e52f20a

    
           | "SET_LIST_CAPACITY     | Returns if the List was set to a new Capacity successfully (true) or not (false)         | 
        
           | "LIST_CONTAINS         | Returns true if the given value is in the given List, false otherwise                    | 
        
           | "GET_LIST_LENGTH       | Returns the length (number of values) of the given List as a string                      | 
        
           | "GET_LIST_VALUES       | Return the values in the given List as a comma delineated string                         | 
        
           | "APPEND_LIST_VALUE     | Returns if the given value was successfuly added to the List (true) or not (false)       | 
        
           | "DELETE_LIST_VALUE     | Rreturns if the given value was successfuly deleted from the List (true) or not (false)  |

agones-bot · 2024-01-19T23:20:08Z

Build Failed 😱

Build Id: 16e003c0-d733-4979-b4d3-cef8765c21ab

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

agones-bot · 2024-01-24T00:01:25Z

Build Failed 😱

Build Id: bb0851ce-a0ee-4b5c-8fd9-ad0465f9998c

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

Kalaiselvi84 · 2024-01-24T19:52:33Z

I have made updates to the getListValues and listContains methods. However, I am unable to test these changes locally, or all the negative scenarios appear to be functioning as expected. However, I'm unsure why none of the methods are returning a success message.

Please review it and share your thoughts.

markmandel · 2024-01-29T07:09:31Z

examples/simple-game-server/main.go

+	if len(values) > 0 {
+        return "VALUES: " + strings.Join(values, ",") + "\n", nil
+    }
+	return "Empty List" + strings.Join(values, ",") + "\n", err


Suggested change

return "Empty List" + strings.Join(values, ",") + "\n", err

return "VALUES: <none>\n", err

Or something like that? To be consistent with the other responses of having a "DESCRIPTOR:RESULT" format.

The response for the GetListValues empty is "VALUES: <none>\n", and the ERROR: INVALID LIST NAME response is returned for the list name if the given list name is incorrect.

$ nc -u localhost 7654
GET_COUNTER_COUNT rooms
COUNTER: 1
GET_LIST_VALUES players
VALUES: test0,test1,test2
LIST_CONTAINS players test1
FOUND: true
GET_LIST_VALUES abc
ERROR: INVALID LIST NAME
DELETE_LIST_VALUE test0
ERROR: Invalid DELETE_LIST_VALUE, should have 2 arguments
DELETE_LIST_VALUE players test0
SUCCESS: true
DELETE_LIST_VALUE players test1
SUCCESS: true
DELETE_LIST_VALUE players test2
SUCCESS: true
GET_LIST_VALUES players
VALUES:

agones-bot · 2024-01-29T18:32:04Z

Build Failed 😱

Build Id: d3c6ea83-1495-4ffd-843f-2d00759bf8e6

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

agones-bot · 2024-01-29T19:03:51Z

Build Failed 😱

Build Id: a4e6ace8-2853-4797-b7d6-93c6a6ea3635

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

igooch

It looks like the updated image reference is missing in a couple places like test/e2e/framework.go.

Kalaiselvi84 · 2024-01-29T23:59:15Z

It looks like the updated image reference is missing in a couple places like test/e2e/framework.go.

Will update the new image in the test/e2e directory once the image pushed to prod(doing it now).

agones-bot · 2024-01-30T00:14:05Z

Build Failed 😱

Build Id: 95dd8212-b214-4d84-999f-cff162a0a619

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

Kalaiselvi84 · 2024-01-30T00:55:09Z

A new image has been pushed for the simple-game-server in prod, and the necessary files have been updated with this new image.

agones-bot · 2024-01-30T01:17:54Z

Build Failed 😱

Build Id: a5518d8b-eabc-4538-b057-d3a63a9fc32d

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

Kalaiselvi84 · 2024-01-30T02:29:35Z

I've pushed a new image to the simple-game-server in production. However, I'm not really sure about the reason for the build failure. Am I missing anything that needs updating? If we can't fix this build issue, should we delete the new tag from the simple-game-server since tomorrow is the release day?

agones-bot · 2024-01-30T04:15:38Z

Build Succeeded 👏

Build Id: 0472e475-800f-45f1-9bd5-5b9e29b1f174

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):

https://9af8290-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3589/head:pr_3589 && git checkout pr_3589
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.38.0-dev-9af8290-amd64

Kalaiselvi84 · 2024-01-30T04:20:27Z

Log for passed tests - https://gist.github.com/Kalaiselvi84/4196d647e4c069bbe3b1237e8033ca66

markmandel

Congrats on getting this to pass!

I think one of my review comments kept getting lost, so I tried to make it as clear as possible. I think that's about it on review though. Lemme know if that's not clear. Shouldn't need to delete 0.26, since the current release isn't using it.

Since K8s clusters will cache existing image tags for a while, so once the review change has made, we will need to increment the version number and push again, as we can't force remove it from all clusters.

markmandel · 2024-01-30T04:21:50Z

examples/simple-game-server/main.go

+		return strconv.FormatBool(ok), err
+	}	
+	if ok {
+		return "FOUND: " + strconv.FormatBool(ok) + "\n", nil


I think this review comment got lost somewhere - but this result should be the following.

func listContains(s *sdk.SDK, listName string, value string) (string, error) { log.Printf("Getting List %s contains value %s", listName, value) ok, err := s.Alpha().ListContains(listName, value) if err != nil { log.Printf("Error getting List %s contains value %s: %s", listName, value, err) return strconv.FormatBool(ok), err } return "FOUND: " + strconv.FormatBool(ok) + "\n", nil }

Not finding a value shouldn't be an error - it's just a search function.

markmandel · 2024-01-30T04:22:23Z

test/e2e/gameserver_test.go

 		},
 		"ListContains false": {
 			msg:  "LIST_CONTAINS games game0",
-			want: "false",
+			want: "ERROR: false\n",


This test will need to change with the update to the response.

Sorry, I thought the response for ListContains false should be ERROR: false. I have now updated it as per your suggestion. The response for the ListContains is FOUND: true and for ListContains false it is FOUND: false. Does this look good to you?

👍🏻 no worries - had a feeling my suggestion wasn't 100% clear. I feel like this will break the e2e test, since a new image.

This all LGTM! We just need it to pass! Probably best bet is to increment the tag again everywhere and push a new image.

Will increment the tag now

I've pushed the new image to the agones-images project and updated the necessary files with the new tag.

Success! The build has finished without any issues.

agones-bot · 2024-01-30T05:34:02Z

Build Failed 😱

Build Id: 9c0b19fa-d44b-4c1f-a241-ccd35430a178

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

agones-bot · 2024-01-30T06:50:57Z

Build Succeeded 👏

Build Id: 24a044f2-fb30-48cf-b679-0a539b825789

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):

https://b591f8f-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3589/head:pr_3589 && git checkout pr_3589
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.38.0-dev-b591f8f-amd64

markmandel

LGTM! @igooch giving you a last right of refusal, but if you are happy, I'm happy to merge just in time before release 👍🏻

Kalaiselvi84 added 2 commits January 11, 2024 03:56

Simple Game Server: Add \n to Counters and Lists Response

cf21235

gofmt

772642a

Kalaiselvi84 requested a review from igooch January 11, 2024 04:01

github-actions bot added kind/cleanup Refactoring code, fixing up documentation, etc size/S labels Jan 11, 2024

igooch reviewed Jan 12, 2024

View reviewed changes

test/e2e/gameserver_test.go Outdated Show resolved Hide resolved

examples/simple-game-server/main.go Outdated Show resolved Hide resolved

Changes in functions

e660318

github-actions bot added the size/M label Jan 12, 2024

Kalaiselvi84 added 2 commits January 17, 2024 21:49

fine tuning

55cd3bc

changes

6e65329

Kalaiselvi84 requested a review from markmandel January 17, 2024 22:14

Kalaiselvi84 marked this pull request as ready for review January 17, 2024 22:14

Merge branch 'main' into pr-3586

7505d70

markmandel reviewed Jan 17, 2024

View reviewed changes

test/e2e/gameserver_test.go Show resolved Hide resolved

Kalaiselvi84 marked this pull request as draft January 19, 2024 19:53

igooch reviewed Jan 19, 2024

View reviewed changes

missing quotes

56e4880

Kalaiselvi84 added 2 commits January 23, 2024 23:06

Change in listContains and getListValues

8522d9c

Merge branch 'main' into pr-3586

eaf5273

update main.go and gameserver_test.go files

17416c7

markmandel reviewed Jan 29, 2024

View reviewed changes

Kalaiselvi84 added 2 commits January 29, 2024 18:18

Review changes

4d55bf2

Merge branch 'main' into pr-3586

6b77e26

bump simple-game-server image

337ab0d

igooch reviewed Jan 29, 2024

View reviewed changes

bump simple-game-server image outside example/simple-game-server

4570f0f

Kalaiselvi84 requested a review from gongmax January 30, 2024 00:50

Kalaiselvi84 added 2 commits January 30, 2024 02:38

adjust expected value

51e518c

update response for counters and lists in fleet_test.go

9af8290

markmandel reviewed Jan 30, 2024

View reviewed changes

igooch enabled auto-merge (squash) January 30, 2024 04:26

Kalaiselvi84 marked this pull request as draft January 30, 2024 04:31

auto-merge was automatically disabled January 30, 2024 04:31
Pull request was converted to draft

modify listContains

f7fdae7

bump simple-game-server

b591f8f

Kalaiselvi84 marked this pull request as ready for review January 30, 2024 05:57

markmandel approved these changes Jan 30, 2024

View reviewed changes

igooch merged commit 2477589 into googleforgames:main Jan 30, 2024
4 checks passed

Kalaiselvi84 deleted the pr-3586 branch March 15, 2024 01:03

	return "Empty List" + strings.Join(values, ",") + "\n", err
	return "VALUES: <none>\n", err

Simple Game Server: Add \n to Counters and Lists Response #3589

Simple Game Server: Add \n to Counters and Lists Response #3589

Conversation

Kalaiselvi84 commented Jan 11, 2024 • edited Loading

Kalaiselvi84 commented Jan 11, 2024

agones-bot commented Jan 11, 2024

igooch left a comment

Choose a reason for hiding this comment

agones-bot commented Jan 12, 2024

Kalaiselvi84 commented Jan 17, 2024

agones-bot commented Jan 17, 2024

igooch left a comment

Choose a reason for hiding this comment

Kalaiselvi84 commented Jan 19, 2024 • edited Loading

igooch commented Jan 19, 2024

agones-bot commented Jan 19, 2024

agones-bot commented Jan 24, 2024

Kalaiselvi84 commented Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Jan 29, 2024

agones-bot commented Jan 29, 2024

igooch left a comment

Choose a reason for hiding this comment

Kalaiselvi84 commented Jan 29, 2024

agones-bot commented Jan 30, 2024

Kalaiselvi84 commented Jan 30, 2024

agones-bot commented Jan 30, 2024

Kalaiselvi84 commented Jan 30, 2024

agones-bot commented Jan 30, 2024

Kalaiselvi84 commented Jan 30, 2024

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kalaiselvi84 Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Jan 30, 2024

agones-bot commented Jan 30, 2024

markmandel left a comment

Choose a reason for hiding this comment

Kalaiselvi84 commented Jan 11, 2024 •

edited

Loading

Kalaiselvi84 commented Jan 19, 2024 •

edited

Loading

Kalaiselvi84 commented Jan 24, 2024 •

edited

Loading

Kalaiselvi84 Jan 30, 2024 •

edited

Loading