Implements UpdateList, AddListValue, and RemoveListValue in the SDK S…

…erver (#3445)

* Implements UpdateList, AddValue, and RemoveValue in the SDK Server

* Adds SDK Server List Tests

* Uses GetList within AddListValue and RemoveListValue and adds more testing

* Removes unnecessary error messages when merging two lists

Adds a const for the List maximum capacity

---------

Co-authored-by: Mengye (Max) Gong <8364575+gongmax@users.noreply.github.com>

pkg/apis/agones/v1/gameserver.go

@@ -23,6 +23,7 @@ import (
 	"agones.dev/agones/pkg"
 	"agones.dev/agones/pkg/apis"
 	"agones.dev/agones/pkg/apis/agones"
+	"agones.dev/agones/pkg/util/apiserver"
 	"agones.dev/agones/pkg/util/runtime"
 	"github.com/mattbaird/jsonpatch"
 	"github.com/pkg/errors"
@@ -919,7 +920,7 @@ func (gs *GameServer) UpdateCounterCapacity(name string, capacity int64) error {
 
 // UpdateListCapacity updates the ListStatus Capacity to the given capacity.
 func (gs *GameServer) UpdateListCapacity(name string, capacity int64) error {
-	if capacity < 0 || capacity > 1000 {
+	if capacity < 0 || capacity > apiserver.ListMaxCapacity {
 		return errors.Errorf("unable to UpdateListCapacity: Name %s, Capacity %d. Capacity must be between 0 and 1000, inclusive", name, capacity)
 	}
 	if list, ok := gs.Status.Lists[name]; ok {
@@ -932,31 +933,27 @@ func (gs *GameServer) UpdateListCapacity(name string, capacity int64) error {
 
 // AppendListValues adds unique values to the ListStatus Values list.
 func (gs *GameServer) AppendListValues(name string, values []string) error {
-	if len(values) == 0 {
-		return errors.Errorf("unable to AppendListValues: Name %s, Values %s. No values to append", name, values)
+	if values == nil {
+		return errors.Errorf("unable to AppendListValues: Name %s, Values %s. Values must not be nil", name, values)
 	}
 	if list, ok := gs.Status.Lists[name]; ok {
-		mergedList := mergeRemoveDuplicates(list.Values, values)
-		// TODO: Truncate and apply up to cutoff
-		if len(mergedList) > int(list.Capacity) {
-			return errors.Errorf("unable to AppendListValues: Name %s, Values %s. Appended list length %d exceeds list capacity %d", name, values, len(mergedList), list.Capacity)
-		}
-		// If all given values are duplicates we give an error warning.
-		if len(mergedList) == len(list.Values) {
-			return errors.Errorf("unable to AppendListValues: Name %s, Values %s. All appended values are duplicates of the existing list", name, values)
-		}
-		// If only some values are duplicates, those duplicate values are silently dropped.
+		mergedList := MergeRemoveDuplicates(list.Values, values)
+		// Any duplicate values are silently dropped.
 		list.Values = mergedList
+		// Truncate values if more than capacity
+		if len(list.Values) > int(list.Capacity) {
+			list.Values = append([]string{}, list.Values[:list.Capacity]...)
+		}
 		gs.Status.Lists[name] = list
 		return nil
 	}
 	return errors.Errorf("unable to AppendListValues: Name %s, Values %s. List not found in GameServer %s", name, values, gs.ObjectMeta.GetName())
 }
 
-// mergeRemoveDuplicates merges two lists and removes any duplicate values.
+// MergeRemoveDuplicates merges two lists and removes any duplicate values.
 // Maintains ordering, so new values from list2 are appended to the end of list1.
 // Returns a new list with unique values only.
-func mergeRemoveDuplicates(list1 []string, list2 []string) []string {
+func MergeRemoveDuplicates(list1 []string, list2 []string) []string {
 	uniqueList := []string{}
 	listMap := make(map[string]bool)
 	for _, v1 := range list1 {

pkg/apis/agones/v1/gameserver_test.go

@@ -1897,6 +1897,8 @@ func TestGameServerUpdateListCapacity(t *testing.T) {
 func TestGameServerAppendListValues(t *testing.T) {
 	t.Parallel()
 
+	var nilSlice []string
+
 	testCases := map[string]struct {
 		gs      GameServer
 		name    string
@@ -1960,37 +1962,22 @@ func TestGameServerAppendListValues(t *testing.T) {
 			},
 			wantErr: false,
 		},
-		"append no values no-op with error": {
+		"append nil values": {
 			gs: GameServer{Status: GameServerStatus{
 				Lists: map[string]ListStatus{
 					"blings": {
 						Values:   []string{"bling1"},
 						Capacity: 10,
 					}}}},
 			name:   "blings",
-			values: []string{},
+			values: nilSlice,
 			want: ListStatus{
 				Values:   []string{"bling1"},
 				Capacity: 10,
 			},
 			wantErr: true,
 		},
-		"append only duplicates no-op with error": {
-			gs: GameServer{Status: GameServerStatus{
-				Lists: map[string]ListStatus{
-					"slings": {
-						Values:   []string{"slings1", "sling2"},
-						Capacity: 4,
-					}}}},
-			name:   "blings",
-			values: []string{},
-			want: ListStatus{
-				Values:   []string{"slings1", "sling2"},
-				Capacity: 4,
-			},
-			wantErr: true,
-		},
-		"append too many values no-op with error": {
+		"append too many values truncates list": {
 			gs: GameServer{Status: GameServerStatus{
 				Lists: map[string]ListStatus{
 					"bananaslugs": {
@@ -2000,10 +1987,10 @@ func TestGameServerAppendListValues(t *testing.T) {
 			name:   "bananaslugs",
 			values: []string{"bananaslug4", "bananaslug5", "bananaslug6"},
 			want: ListStatus{
-				Values:   []string{"bananaslugs1", "bananaslug2", "bananaslug3"},
+				Values:   []string{"bananaslugs1", "bananaslug2", "bananaslug3", "bananaslug4", "bananaslug5"},
 				Capacity: 5,
 			},
-			wantErr: true,
+			wantErr: false,
 		},
 	}
 
@@ -2054,7 +2041,7 @@ func TestMergeRemoveDuplicates(t *testing.T) {
 
 	for test, testCase := range testCases {
 		t.Run(test, func(t *testing.T) {
-			got := mergeRemoveDuplicates(testCase.str1, testCase.str2)
+			got := MergeRemoveDuplicates(testCase.str1, testCase.str2)
 			assert.Equal(t, testCase.want, got)
 		})
 	}

pkg/apis/allocation/v1/gameserverallocation_test.go

@@ -1006,7 +1006,7 @@ func TestGameServerListActions(t *testing.T) {
 					}}}},
 			wantErr: false,
 		},
-		"update list values and capacity - value add fails": {
+		"update list values and capacity - value add truncates silently": {
 			la: ListAction{
 				AddValues: []string{"fairy1", "fairy3"},
 				Capacity:  int64Pointer(2),
@@ -1024,7 +1024,7 @@ func TestGameServerListActions(t *testing.T) {
 						Values:   []string{"fairy1", "fairy2"},
 						Capacity: 2,
 					}}}},
-			wantErr: true,
+			wantErr: false,
 		},
 	}
 

pkg/gameserverallocations/allocator_test.go

@@ -397,7 +397,7 @@ func TestAllocatorApplyAllocationToGameServerCountsListsActions(t *testing.T) {
 				}},
 			wantLists: map[string]agonesv1.ListStatus{
 				"players": {
-					Values:   []string{},
+					Values:   []string{"x7un"},
 					Capacity: 1,
 				}},
 		},

pkg/sdkserver/localsdk.go

@@ -29,6 +29,7 @@ import (
 	"agones.dev/agones/pkg/sdk"
 	"agones.dev/agones/pkg/sdk/alpha"
 	"agones.dev/agones/pkg/sdk/beta"
+	"agones.dev/agones/pkg/util/apiserver"
 	"agones.dev/agones/pkg/util/runtime"
 	"github.com/fsnotify/fsnotify"
 	"github.com/mennanov/fmutils"
@@ -661,8 +662,7 @@ func (l *LocalSDKServer) UpdateList(ctx context.Context, in *alpha.UpdateListReq
 		return nil, errors.Errorf("INVALID_ARGUMENT. Field Mask Path(s): %v are invalid for List. Use valid field name(s): %v", in.UpdateMask.GetPaths(), in.List.ProtoReflect().Descriptor().Fields())
 	}
 
-	// TODO: Pull in variable Max Capacity from CRD instead of hard-coded number here.
-	if in.List.Capacity < 0 || in.List.Capacity > 1000 {
+	if in.List.Capacity < 0 || in.List.Capacity > apiserver.ListMaxCapacity {
 		return nil, errors.Errorf("OUT_OF_RANGE. Capacity must be within range [0,1000]. Found Capacity: %d", in.List.Capacity)
 	}