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

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

Merged
merged 30 commits into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
cf21235
Simple Game Server: Add \n to Counters and Lists Response
Kalaiselvi84 Jan 11, 2024
772642a
gofmt
Kalaiselvi84 Jan 11, 2024
e660318
Changes in functions
Kalaiselvi84 Jan 12, 2024
55cd3bc
fine tuning
Kalaiselvi84 Jan 17, 2024
6e65329
changes
Kalaiselvi84 Jan 17, 2024
7505d70
Merge branch 'main' into pr-3586
Kalaiselvi84 Jan 17, 2024
56e4880
missing quotes
Kalaiselvi84 Jan 19, 2024
8522d9c
Change in listContains and getListValues
Kalaiselvi84 Jan 23, 2024
eaf5273
Merge branch 'main' into pr-3586
Kalaiselvi84 Jan 23, 2024
17416c7
update main.go and gameserver_test.go files
Kalaiselvi84 Jan 24, 2024
e666444
NO Values for getListValues
Kalaiselvi84 Jan 24, 2024
6e5f883
review change
Kalaiselvi84 Jan 24, 2024
7f598b4
changes
Kalaiselvi84 Jan 24, 2024
1488685
Merge branch 'main' into pr-3586
Kalaiselvi84 Jan 24, 2024
24220ae
NO Values when list empty
Kalaiselvi84 Jan 25, 2024
0228561
update list contains
Kalaiselvi84 Jan 25, 2024
de736fe
modify listContains and getListValues
Kalaiselvi84 Jan 25, 2024
cd894b9
Merge branch 'main' into pr-3586
Kalaiselvi84 Jan 25, 2024
a0b3558
Rename success with FOUND
Kalaiselvi84 Jan 26, 2024
903628e
Merge branch 'main' into pr-3586
Kalaiselvi84 Jan 26, 2024
c8b19a1
Merge branch 'main' into pr-3586
Kalaiselvi84 Jan 26, 2024
204abaf
Merge branch 'pr-3586' of https://github.com/Kalaiselvi84/agones into…
Kalaiselvi84 Jan 26, 2024
4d55bf2
Review changes
Kalaiselvi84 Jan 29, 2024
6b77e26
Merge branch 'main' into pr-3586
Kalaiselvi84 Jan 29, 2024
337ab0d
bump simple-game-server image
Kalaiselvi84 Jan 29, 2024
4570f0f
bump simple-game-server image outside example/simple-game-server
Kalaiselvi84 Jan 30, 2024
51e518c
adjust expected value
Kalaiselvi84 Jan 30, 2024
9af8290
update response for counters and lists in fleet_test.go
Kalaiselvi84 Jan 30, 2024
f7fdae7
modify listContains
Kalaiselvi84 Jan 30, 2024
b591f8f
bump simple-game-server
Kalaiselvi84 Jan 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions examples/simple-game-server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ There are some text commands you can send the server to affect its behavior:
| "GET_COUNTER_CAPACITY" | Returns the Capacity of the given Counter |
| "SET_COUNTER_CAPACITY" | Sets the Capacity of the given Counter to the given amount |
| "GET_LIST_CAPACITY" | Returns the Capacity of the given List |
| "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) |
| "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) |

## Configuration

Expand Down
45 changes: 32 additions & 13 deletions examples/simple-game-server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -687,8 +687,9 @@ func getCounterCount(s *sdk.SDK, counterName string) (string, error) {
count, err := s.Alpha().GetCounterCount(counterName)
if err != nil {
log.Printf("Error getting Counter %s Count: %s", counterName, err)
return strconv.FormatInt(count, 10), err
}
return strconv.FormatInt(count, 10), err
return "COUNTER: " + strconv.FormatInt(count, 10) + "\n", nil
}

// incrementCounter returns the if the Counter Count was incremented successfully (true) or not (false)
Expand All @@ -701,8 +702,9 @@ func incrementCounter(s *sdk.SDK, counterName string, amount string) (string, er
ok, err := s.Alpha().IncrementCounter(counterName, amountInt)
if err != nil {
log.Printf("Error incrementing Counter %s Count by amount %d: %s", counterName, amountInt, err)
return strconv.FormatBool(ok), err
}
return strconv.FormatBool(ok), err
return "SUCCESS: " + strconv.FormatBool(ok) + "\n", nil
}

// decrementCounter returns the if the Counter Count was decremented successfully (true) or not (false)
Expand All @@ -715,8 +717,9 @@ func decrementCounter(s *sdk.SDK, counterName string, amount string) (string, er
ok, err := s.Alpha().DecrementCounter(counterName, amountInt)
if err != nil {
log.Printf("Error decrementing Counter %s Count by amount %d: %s", counterName, amountInt, err)
return strconv.FormatBool(ok), err
}
return strconv.FormatBool(ok), err
return "SUCCESS: " + strconv.FormatBool(ok) + "\n", nil
}

// setCounterCount returns the if the Counter was set to a new Count successfully (true) or not (false)
Expand All @@ -729,8 +732,9 @@ func setCounterCount(s *sdk.SDK, counterName string, amount string) (string, err
ok, err := s.Alpha().SetCounterCount(counterName, amountInt)
if err != nil {
log.Printf("Error setting Counter %s Count by amount %d: %s", counterName, amountInt, err)
return strconv.FormatBool(ok), err
}
return strconv.FormatBool(ok), err
return "SUCCESS: " + strconv.FormatBool(ok) + "\n", nil
}

// getCounterCapacity returns the Capacity of the given Counter as a string
Expand All @@ -739,8 +743,9 @@ func getCounterCapacity(s *sdk.SDK, counterName string) (string, error) {
count, err := s.Alpha().GetCounterCapacity(counterName)
if err != nil {
log.Printf("Error getting Counter %s Capacity: %s", counterName, err)
return strconv.FormatInt(count, 10), err
}
return strconv.FormatInt(count, 10), err
return "CAPACITY: " + strconv.FormatInt(count, 10) + "\n", nil
}

// setCounterCapacity returns the if the Counter was set to a new Capacity successfully (true) or not (false)
Expand All @@ -753,8 +758,9 @@ func setCounterCapacity(s *sdk.SDK, counterName string, amount string) (string,
ok, err := s.Alpha().SetCounterCapacity(counterName, amountInt)
if err != nil {
log.Printf("Error setting Counter %s Capacity to amount %d: %s", counterName, amountInt, err)
return strconv.FormatBool(ok), err
}
return strconv.FormatBool(ok), err
return "SUCCESS: " + strconv.FormatBool(ok) + "\n", nil
}

// getListCapacity returns the Capacity of the given List as a string
Expand All @@ -763,8 +769,9 @@ func getListCapacity(s *sdk.SDK, listName string) (string, error) {
capacity, err := s.Alpha().GetListCapacity(listName)
if err != nil {
log.Printf("Error getting List %s Capacity: %s", listName, err)
return strconv.FormatInt(capacity, 10), err
}
return strconv.FormatInt(capacity, 10), err
return "CAPACITY: " + strconv.FormatInt(capacity, 10) + "\n", nil
}

// setListCapacity returns if the List was set to a new Capacity successfully (true) or not (false)
Expand All @@ -777,8 +784,9 @@ func setListCapacity(s *sdk.SDK, listName string, amount string) (string, error)
ok, err := s.Alpha().SetListCapacity(listName, amountInt)
if err != nil {
log.Printf("Error setting List %s Capacity to amount %d: %s", listName, amountInt, err)
return strconv.FormatBool(ok), err
}
return strconv.FormatBool(ok), err
return "SUCCESS: " + strconv.FormatBool(ok) + "\n", nil
}

// listContains returns true if the given value is in the given List, false otherwise
Expand All @@ -787,8 +795,12 @@ func listContains(s *sdk.SDK, listName string, value string) (string, error) {
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
}
if ok {
return "FOUND: " + strconv.FormatBool(ok) + "\n", nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
return strconv.FormatBool(ok), err
return "ERROR: " + strconv.FormatBool(ok) + "\n", err
}

// getListLength returns the length (number of values) of the given List as a string
Expand All @@ -797,8 +809,9 @@ func getListLength(s *sdk.SDK, listName string) (string, error) {
length, err := s.Alpha().GetListLength(listName)
if err != nil {
log.Printf("Error getting List %s length: %s", listName, err)
return strconv.Itoa(length), err
}
return strconv.Itoa(length), err
return "LENGTH: " + strconv.Itoa(length) + "\n", nil
}

// getListValues return the values in the given List as a comma delineated string
Expand All @@ -807,8 +820,12 @@ func getListValues(s *sdk.SDK, listName string) (string, error) {
values, err := s.Alpha().GetListValues(listName)
if err != nil {
log.Printf("Error getting List %s values: %s", listName, err)
return strings.Join(values, ","), err
}
return strings.Join(values, ",") + "\n", err
if len(values) > 0 {
return "VALUES: " + strings.Join(values, ",") + "\n", nil
}
return "Empty List" + strings.Join(values, ",") + "\n", err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you review the getListValues method? I'm unsure if anything is missing. In negative test cases, it only returns 'ERROR:' without further details. Is this behavior expected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's normal for the examples/simply-game-server/main.go code. This code only sends back the return value, not any return error. And in the case of GetListValues the return value is nil. We probably should pipe through the error message, but that's not in the code as it stands.

func tcpHandleCommand(conn net.Conn, txt string, s *sdk.SDK, cancel context.CancelFunc) {
log.Printf("TCP txt: %v", txt)
response, addACK, err := handleResponse(txt, s, cancel)
if err != nil {
response = "ERROR: " + response + "\n"
} else if addACK {
response = "ACK TCP: " + response + "\n"
}
tcpRespond(conn, response)
if response == "EXIT" {
exit(s)
}
}
is where it only returns the response.

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
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

}

// appendListValue returns if the given value was successfuly added to the List (true) or not (false)
Expand All @@ -817,8 +834,9 @@ func appendListValue(s *sdk.SDK, listName string, value string) (string, error)
ok, err := s.Alpha().AppendListValue(listName, value)
if err != nil {
log.Printf("Error appending Value %s to List %s: %s", value, listName, err)
return strconv.FormatBool(ok), err
}
return strconv.FormatBool(ok), err
return "SUCCESS: " + strconv.FormatBool(ok) + "\n", nil
}

// deleteListValue returns if the given value was successfuly deleted from the List (true) or not (false)
Expand All @@ -827,8 +845,9 @@ func deleteListValue(s *sdk.SDK, listName string, value string) (string, error)
ok, err := s.Alpha().DeleteListValue(listName, value)
if err != nil {
log.Printf("Error deleting Value %s to List %s: %s", value, listName, err)
return strconv.FormatBool(ok), err
}
return strconv.FormatBool(ok), err
return "SUCCESS: " + strconv.FormatBool(ok) + "\n", nil
}

// doHealth sends the regular Health Pings
Expand Down
66 changes: 33 additions & 33 deletions test/e2e/gameserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1365,93 +1365,93 @@ func TestCounters(t *testing.T) {
}{
"GetCounterCount": {
msg: "GET_COUNTER_COUNT games",
want: "1",
want: "COUNTER: 1\n",
},
"GetCounterCount Counter Does Not Exist": {
msg: "GET_COUNTER_COUNT fame",
want: "ERROR: -1\n",
},
"IncrementCounter": {
msg: "INCREMENT_COUNTER foo 10",
want: "true",
want: "SUCCESS: true\n",
counterName: "foo",
wantCount: "20",
wantCount: "COUNTER: 20\n",
},
"IncrementCounter Past Capacity": {
msg: "INCREMENT_COUNTER games 50",
want: "ERROR: false\n",
counterName: "games",
wantCount: "1",
wantCount: "COUNTER: 1\n",
},
"IncrementCounter Negative": {
msg: "INCREMENT_COUNTER games -1",
want: "ERROR: false\n",
counterName: "games",
wantCount: "1",
wantCount: "COUNTER: 1\n",
},
"IncrementCounter Counter Does Not Exist": {
msg: "INCREMENT_COUNTER same 1",
want: "ERROR: false\n",
},
"DecrementCounter": {
msg: "DECREMENT_COUNTER bar 10",
want: "true",
want: "SUCCESS: true\n",
counterName: "bar",
wantCount: "0",
wantCount: "COUNTER: 0\n",
},
"DecrementCounter Past Capacity": {
msg: "DECREMENT_COUNTER games 2",
want: "ERROR: false\n",
counterName: "games",
wantCount: "1",
wantCount: "COUNTER: 1\n",
},
"DecrementCounter Negative": {
msg: "DECREMENT_COUNTER games -1",
want: "ERROR: false\n",
counterName: "games",
wantCount: "1",
wantCount: "COUNTER: 1\n",
},
"DecrementCounter Counter Does Not Exist": {
msg: "DECREMENT_COUNTER lame 1",
want: "ERROR: false\n",
},
"SetCounterCount": {
msg: "SET_COUNTER_COUNT baz 0",
want: "true",
want: "SUCCESS: true\n",
counterName: "baz",
wantCount: "0",
wantCount: "COUNTER: 0\n",
},
"SetCounterCount Past Capacity": {
msg: "SET_COUNTER_COUNT games 51",
want: "ERROR: false\n",
counterName: "games",
wantCount: "1",
wantCount: "COUNTER: 1\n",
},
"SetCounterCount Past Zero": {
msg: "SET_COUNTER_COUNT games -1",
want: "ERROR: false\n",
counterName: "games",
wantCount: "1",
wantCount: "COUNTER: 1\n",
},
"GetCounterCapacity": {
msg: "GET_COUNTER_CAPACITY games",
want: "50",
want: "CAPACITY: 50\n",
},
"GetCounterCapacity Counter Does Not Exist": {
msg: "GET_COUNTER_CAPACITY dame",
want: "ERROR: -1\n",
},
"SetCounterCapacity": {
msg: "SET_COUNTER_CAPACITY qux 0",
want: "true",
want: "SUCCESS: true\n",
counterName: "qux",
wantCapacity: "0",
wantCapacity: "CAPACITY: 0\n",
},
"SetCounterCapacity Past Zero": {
msg: "SET_COUNTER_CAPACITY games -42",
want: "ERROR: false\n",
counterName: "games",
wantCount: "1",
wantCount: "COUNTER: 1\n",
},
}
// nolint:dupl // Linter errors on lines are duplicate of TestLists
Expand Down Expand Up @@ -1525,69 +1525,69 @@ func TestLists(t *testing.T) {
}{
"GetListCapacity": {
msg: "GET_LIST_CAPACITY games",
want: "50",
want: "CAPACITY: 50\n",
},
"SetListCapacity": {
msg: "SET_LIST_CAPACITY foo 1000",
want: "true",
want: "SUCCESS: true\n",
listName: "foo",
wantCapacity: "1000",
wantCapacity: "CAPACITY: 1000\n",
},
"SetListCapacity past 1000": {
msg: "SET_LIST_CAPACITY games 1001",
want: "ERROR: false\n",
listName: "games",
wantCapacity: "50",
wantCapacity: "CAPACITY: 50\n",
},
"SetListCapacity negative": {
msg: "SET_LIST_CAPACITY games -1",
want: "ERROR: false\n",
listName: "games",
wantCapacity: "50",
wantCapacity: "CAPACITY: 50\n",
},
"ListContains": {
msg: "LIST_CONTAINS games game2",
want: "true",
want: "FOUND: true\n",
},
"ListContains false": {
msg: "LIST_CONTAINS games game0",
want: "false",
want: "ERROR: false\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Kalaiselvi84 Kalaiselvi84 Jan 30, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will increment the tag now

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 pushed the new image to the agones-images project and updated the necessary files with the new tag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤞🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Success! The build has finished without any issues.

},
"GetListLength": {
msg: "GET_LIST_LENGTH games",
want: "2",
want: "LENGTH: 2\n",
},
"GetListValues": {
msg: "GET_LIST_VALUES games",
want: "game1,game2\n",
want: "VALUES: game1,game2\n",
},
"GetListValues empty": {
msg: "GET_LIST_VALUES foo",
want: "\n",
want: "Empty List\n",
},
"AppendListValue": {
msg: "APPEND_LIST_VALUE bar bar3",
want: "true",
want: "SUCCESS: true\n",
listName: "bar",
wantLength: "3",
wantLength: "LENGTH: 3\n",
},
"AppendListValue past capacity": {
msg: "APPEND_LIST_VALUE baz baz2",
want: "ERROR: false\n",
listName: "baz",
wantLength: "1",
wantLength: "LENGTH: 1\n",
},
"DeleteListValue": {
msg: "DELETE_LIST_VALUE qux qux3",
want: "true",
want: "SUCCESS: true\n",
listName: "qux",
wantLength: "3",
wantLength: "LENGTH: 3\n",
},
"DeleteListValue value does not exist": {
msg: "DELETE_LIST_VALUE games game4",
want: "ERROR: false\n",
listName: "games",
wantLength: "2",
wantLength: "LENGTH: 2\n",
},
}
// nolint:dupl // Linter errors on lines are duplicate of TestCounters
Expand Down
Loading