Updates actions to return single error

pkg/apis/agones/v1/common.go

@@ -26,7 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
-// nolint:stylecheck
+// Block of const Error messages and GameServerAllocation Counter actions
 const (
 	ErrContainerRequired        = "Container is required when using multiple containers in the pod template"
 	ErrHostPort                 = "HostPort cannot be specified with a Dynamic or Passthrough PortPolicy"
@@ -36,7 +36,7 @@ const (
 	ErrContainerNameInvalid     = "Container must be empty or the name of a container in the pod template"
 	// GameServerAllocationIncrement is a Counter Action that indiciates the Counter's Count should be incremented at Allocation.
 	GameServerAllocationIncrement string = "Increment"
-	// GameServerAllocationIncrement is a Counter Action that indiciates the Counter's Count should be decremented at Allocation.
+	// GameServerAllocationDecrement is a Counter Action that indiciates the Counter's Count should be decremented at Allocation.
 	GameServerAllocationDecrement string = "Decrement"
 )
 

pkg/apis/agones/v1/gameserver.go

@@ -929,92 +929,99 @@ func (gs *GameServer) Patch(delta *GameServer) ([]byte, error) {
 
 // UpdateCount increments or decrements a CounterStatus on a Game Server by the given amount.
 func (gs *GameServer) UpdateCount(name string, action string, amount int64) error {
-	err := fmt.Errorf("unable to UpdateCount: Name %s, Action %s, Amount %d", name, action, amount)
-	if !(action == GameServerAllocationIncrement || action == GameServerAllocationDecrement) || amount < 0 {
-		return err
+	if !(action == GameServerAllocationIncrement || action == GameServerAllocationDecrement) {
+		return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Allocation action must be one of %s or %s", name, action, amount, GameServerAllocationIncrement, GameServerAllocationDecrement)
+	}
+	if amount < 0 {
+		return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Amount must be greater than 0", name, action, amount)
 	}
 	if counter, ok := gs.Status.Counters[name]; ok {
 		cnt := counter.Count
 		if action == GameServerAllocationIncrement {
 			cnt += amount
 			// only check for Count > Capacity when incrementing
 			if cnt > counter.Capacity {
-				return err
+				return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Incremented Count %d is greater than available capacity %d", name, action, amount, cnt, counter.Capacity)
 			}
-		}
-		if action == GameServerAllocationDecrement {
+		} else {
 			cnt -= amount
-		}
-		if cnt < 0 {
-			return err
+			// only check for Count < 0 when decrementing
+			if cnt < 0 {
+				return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Decremented Count %d is less than 0", name, action, amount, cnt)
+			}
 		}
 		counter.Count = cnt
 		gs.Status.Counters[name] = counter
 		return nil
 	}
-	return err
+	return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Counter not found in GameServer %s", name, action, amount, gs.ObjectMeta.GetName())
 }
 
 // UpdateCounterCapacity updates the CounterStatus Capacity to the given capacity.
 func (gs *GameServer) UpdateCounterCapacity(name string, capacity int64) error {
-	err := fmt.Errorf("unable to UpdateCounterCapacity: Name %s, Capacity %d", name, capacity)
 	if capacity < 0 {
-		return err
+		return errors.Errorf("unable to UpdateCounterCapacity: Name %s, Capacity %d. Capacity must be greater than or equal to 0", name, capacity)
 	}
 	if counter, ok := gs.Status.Counters[name]; ok {
 		counter.Capacity = capacity
 		gs.Status.Counters[name] = counter
 		return nil
 	}
-	return err
+	return errors.Errorf("unable to UpdateCounterCapacity: Name %s, Capacity %d. Counter not found in GameServer %s", name, capacity, gs.ObjectMeta.GetName())
 }
 
 // UpdateListCapacity updates the ListStatus Capacity to the given capacity.
 func (gs *GameServer) UpdateListCapacity(name string, capacity int64) error {
-	err := fmt.Errorf("unable to UpdateListCapacity: Name %s, Capacity %d", name, capacity)
 	if capacity < 0 || capacity > 1000 {
-		return err
+		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 {
 		list.Capacity = capacity
 		gs.Status.Lists[name] = list
 		return nil
 	}
-	return err
+	return errors.Errorf("unable to UpdateListCapacity: Name %s, Capacity %d. List not found in GameServer %s", name, capacity, gs.ObjectMeta.GetName())
 }
 
-// AppendListValues adds given the values to the ListStatus Values list. Any duplicates are silently
-// dropped.
+// AppendListValues adds unique values to the ListStatus Values list.
 func (gs *GameServer) AppendListValues(name string, values []string) error {
-	err := fmt.Errorf("unable to AppendListValues: Name %s, Values %s", name, values)
 	if len(values) == 0 {
-		return err
+		return errors.Errorf("unable to AppendListValues: Name %s, Values %s. No values to append", name, values)
 	}
 	if list, ok := gs.Status.Lists[name]; ok {
-		vals := []string{}
-		// Maintain ordering so new values are appended to the end of list.Values
-		vals = append(vals, list.Values...)
-		vals = append(vals, values...)
-		vals = removeDuplicates(vals)
-		if (len(vals) > int(list.Capacity)) || (len(vals) == len(list.Values)) {
-			return err
-		}
-		list.Values = vals
+		mergedList := mergeRemoveDuplicates(list.Values, values)
+		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.
+		list.Values = mergedList
 		gs.Status.Lists[name] = list
 		return nil
 	}
-	return err
+	return errors.Errorf("unable to AppendListValues: Name %s, Values %s. List not found in GameServer %s", name, values, gs.ObjectMeta.GetName())
 }
 
-// removeDuplicates removes any duplicate values from a list. Returns new list with unique values only.
-func removeDuplicates(values []string) []string {
+// 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 {
+	uniqueList := []string{}
 	listMap := make(map[string]bool)
-	uniqueValues := []string{}
-	for _, v := range values {
-		if _, ok := listMap[v]; !ok {
-			uniqueValues = append(uniqueValues, v)
-			listMap[v] = true
+	for _, v1 := range list1 {
+		if _, ok := listMap[v1]; !ok {
+			uniqueList = append(uniqueList, v1)
+			listMap[v1] = true
+		}
+	}
+	for _, v2 := range list2 {
+		if _, ok := listMap[v2]; !ok {
+			uniqueList = append(uniqueList, v2)
+			listMap[v2] = true
 		}
 	}
-	return uniqueValues
+	return uniqueList
 }

pkg/apis/agones/v1/gameserver_test.go

@@ -2050,34 +2050,39 @@ func TestGameServerAppendListValues(t *testing.T) {
 	}
 }
 
-func TestRemoveDuplicates(t *testing.T) {
+func TestMergeRemoveDuplicates(t *testing.T) {
 	t.Parallel()
 
 	testCases := map[string]struct {
-		values []string
-		want   []string
+		str1 []string
+		str2 []string
+		want []string
 	}{
-		"empty string array": {
-			values: []string{},
-			want:   []string{},
+		"empty string arrays": {
+			str1: []string{},
+			str2: []string{},
+			want: []string{},
 		},
 		"no duplicates": {
-			values: []string{"one", "two", "three"},
-			want:   []string{"one", "two", "three"},
+			str1: []string{"one"},
+			str2: []string{"two", "three"},
+			want: []string{"one", "two", "three"},
 		},
 		"remove one duplicate": {
-			values: []string{"one", "one", "one"},
-			want:   []string{"one"},
+			str1: []string{"one", "one", "one"},
+			str2: []string{"one", "one", "one"},
+			want: []string{"one"},
 		},
 		"remove multiple duplicates": {
-			values: []string{"one", "two", "one", "one", "two"},
-			want:   []string{"one", "two"},
+			str1: []string{"one", "two"},
+			str2: []string{"two", "one"},
+			want: []string{"one", "two"},
 		},
 	}
 
 	for test, testCase := range testCases {
 		t.Run(test, func(t *testing.T) {
-			got := removeDuplicates(testCase.values)
+			got := mergeRemoveDuplicates(testCase.str1, testCase.str2)
 			assert.Equal(t, testCase.want, got)
 		})
 	}

pkg/apis/allocation/v1/gameserverallocation.go

@@ -15,6 +15,7 @@
 package v1
 
 import (
+	"errors"
 	"fmt"
 
 	"agones.dev/agones/pkg/apis"
@@ -295,41 +296,41 @@ func (s *GameServerSelector) matchCounters(gs *agonesv1.GameServer) bool {
 }
 
 // CounterActions attempts to peform any actions from the CounterAction on the GameServer Counter.
-// Returns a string list of any actions that could not be performed.
-func (ca *CounterAction) CounterActions(counter string, gs *agonesv1.GameServer) []string {
-	errors := []string{}
+// Returns the errors of any actions that could not be performed.
+func (ca *CounterAction) CounterActions(counter string, gs *agonesv1.GameServer) error {
+	var errs error
 	if ca.Capacity != nil {
 		capErr := gs.UpdateCounterCapacity(counter, *ca.Capacity)
 		if capErr != nil {
-			errors = append(errors, capErr.Error())
+			errs = errors.Join(errs, capErr)
 		}
 	}
 	if ca.Action != nil && ca.Amount != nil {
 		cntErr := gs.UpdateCount(counter, *ca.Action, *ca.Amount)
 		if cntErr != nil {
-			errors = append(errors, cntErr.Error())
+			errs = errors.Join(errs, cntErr)
 		}
 	}
-	return errors
+	return errs
 }
 
 // ListActions attempts to peform any actions from the ListAction on the GameServer List.
 // Returns a string list of any actions that could not be performed.
-func (la *ListAction) ListActions(list string, gs *agonesv1.GameServer) []string {
-	errors := []string{}
+func (la *ListAction) ListActions(list string, gs *agonesv1.GameServer) error {
+	var errs error
 	if la.Capacity != nil {
 		capErr := gs.UpdateListCapacity(list, *la.Capacity)
 		if capErr != nil {
-			errors = append(errors, capErr.Error())
+			errs = errors.Join(errs, capErr)
 		}
 	}
 	if la.AddValues != nil && len(la.AddValues) > 0 {
 		cntErr := gs.AppendListValues(list, la.AddValues)
 		if cntErr != nil {
-			errors = append(errors, cntErr.Error())
+			errs = errors.Join(errs, cntErr)
 		}
 	}
-	return errors
+	return errs
 }
 
 // matchLists returns true if there is a match for the ListSelector in the GameServerStatus