Update golangci-lint

update these to latest
github.com/daixiang0/[email protected] above
sigs.k8s.io/kustomize/kustomize/[email protected] above
github.com/securego/gosec/v2/cmd/[email protected] above

Fix unit test lint issues

Signed-off-by: yiraeChristineKim <[email protected]>

Author: yiraeChristineKim

Diff for: build/common/Makefile.common.mk

@@ -7,7 +7,7 @@ CONTROLLER_GEN_VERSION := v0.16.3
 # https://github.com/kubernetes-sigs/kustomize/releases/latest
 KUSTOMIZE_VERSION := v5.4.3
 # https://github.com/golangci/golangci-lint/releases/latest
-GOLANGCI_VERSION := v1.52.2
+GOLANGCI_VERSION := v1.55.1
 # https://github.com/mvdan/gofumpt/releases/latest
 GOFUMPT_VERSION := v0.7.0
 # https://github.com/daixiang0/gci/releases/latest

Diff for: build/common/config/.golangci.yml

@@ -193,6 +193,7 @@ issues:
     - return statements should not be cuddled if block has more than two lines
     - declarations should never be cuddled
     - don't use leading k in Go names
+    - "require-error: for error assertions use require"
 
   exclude-rules:
     # Allow dot imports in the tests.

Diff for: controllers/configurationpolicy_controller.go

@@ -1429,7 +1429,7 @@ func (r *ConfigurationPolicyReconciler) determineDesiredObjects(
 		objSelector, err := metav1.LabelSelectorAsSelector(objectSelector)
 		if err != nil {
 			log.Error(err, "Failed to select the resources",
-				"objectSelector", fmt.Sprint(objectT.ObjectSelector.String()))
+				"objectSelector", objectT.ObjectSelector.String())
 
 			msg := fmt.Sprintf(
 				"Error parsing provided objectSelector in the object-template at index [%d]: %v",
@@ -1471,7 +1471,7 @@ func (r *ConfigurationPolicyReconciler) determineDesiredObjects(
 
 			if err != nil {
 				log.Error(err, "Failed to fetch the resources",
-					"objectSelector", fmt.Sprint(objectT.ObjectSelector.String()))
+					"objectSelector", objectT.ObjectSelector.String())
 
 				msg := fmt.Sprintf(
 					"Error listing resources with provided objectSelector in the object-template at index [%d]: %v",

Diff for: controllers/configurationpolicy_controller_test.go

@@ -105,7 +105,7 @@ func TestCompareSpecs(t *testing.T) {
 			"name":  "nginx",
 		},
 	}
-	assert.Equal(t, reflect.DeepEqual(merged, mergedExpected), true)
+	assert.True(t, reflect.DeepEqual(merged, mergedExpected))
 
 	spec1 = map[string]interface{}{
 		"containers": map[string]interface{}{
@@ -196,11 +196,13 @@ func TestConvertPolicyStatusToStringLongMsg(t *testing.T) {
 func TestMergeArraysMustHave(t *testing.T) {
 	t.Parallel()
 
-	testcases := map[string]struct {
+	type TestCaseStruct struct {
 		desiredList  []interface{}
 		currentList  []interface{}
 		expectedList []interface{}
-	}{
+	}
+
+	testcases := map[string]TestCaseStruct{
 		"merge array with existing element into array preserves array": {
 			[]interface{}{
 				map[string]interface{}{"a": "apple", "b": "boy"},
@@ -287,28 +289,34 @@ func TestMergeArraysMustHave(t *testing.T) {
 		},
 	}
 
+	runTest := func(t *testing.T, test TestCaseStruct) {
+		t.Helper()
+
+		actualMergedList := mergeArrays(test.desiredList, test.currentList, "musthave", true)
+		assert.Equal(t, fmt.Sprintf("%+v", test.expectedList), fmt.Sprintf("%+v", actualMergedList))
+		assert.True(t, checkListsMatch(test.expectedList, actualMergedList))
+	}
+
 	for testName, test := range testcases {
-		testName := testName
-		test := test
+		local := test
 
 		t.Run(testName, func(t *testing.T) {
 			t.Parallel()
-
-			actualMergedList := mergeArrays(test.desiredList, test.currentList, "musthave", true)
-			assert.Equal(t, fmt.Sprintf("%+v", test.expectedList), fmt.Sprintf("%+v", actualMergedList))
-			assert.True(t, checkListsMatch(test.expectedList, actualMergedList))
+			runTest(t, local)
 		})
 	}
 }
 
 func TestMergeArraysMustOnlyHave(t *testing.T) {
 	t.Parallel()
 
-	testcases := map[string]struct {
+	type TestCaseStruct struct {
 		desiredList  []interface{}
 		currentList  []interface{}
 		expectedList []interface{}
-	}{
+	}
+
+	testcases := map[string]TestCaseStruct{
 		"merge array with one element into array with two becomes new array": {
 			[]interface{}{
 				map[string]interface{}{"a": "apple", "b": "boy"},
@@ -374,16 +382,20 @@ func TestMergeArraysMustOnlyHave(t *testing.T) {
 		},
 	}
 
+	runTest := func(t *testing.T, test TestCaseStruct) {
+		t.Helper()
+
+		actualMergedList := mergeArrays(test.desiredList, test.currentList, "mustonlyhave", true)
+		assert.Equal(t, fmt.Sprintf("%+v", test.expectedList), fmt.Sprintf("%+v", actualMergedList))
+		assert.True(t, checkListsMatch(test.expectedList, actualMergedList))
+	}
+
 	for testName, test := range testcases {
-		testName := testName
-		test := test
+		local := test
 
 		t.Run(testName, func(t *testing.T) {
 			t.Parallel()
-
-			actualMergedList := mergeArrays(test.desiredList, test.currentList, "mustonlyhave", true)
-			assert.Equal(t, fmt.Sprintf("%+v", test.expectedList), fmt.Sprintf("%+v", actualMergedList))
-			assert.True(t, checkListsMatch(test.expectedList, actualMergedList))
+			runTest(t, local)
 		})
 	}
 }
@@ -602,22 +614,22 @@ func TestAddRelatedObject(t *testing.T) {
 	related := relatedList[0]
 
 	// get the related object and validate what we added is in the status
-	assert.True(t, related.Compliant == string(policyv1.Compliant))
-	assert.True(t, related.Reason == "reason")
-	assert.True(t, related.Object.APIVersion == scopedGVR.GroupVersion().String())
-	assert.True(t, related.Object.Kind == "ConfigurationPolicy")
-	assert.True(t, related.Object.Metadata.Name == name)
-	assert.True(t, related.Object.Metadata.Namespace == namespace)
+	assert.Equal(t, string(policyv1.Compliant), related.Compliant)
+	assert.Equal(t, "reason", related.Reason)
+	assert.Equal(t, scopedGVR.GroupVersion().String(), related.Object.APIVersion)
+	assert.Equal(t, "ConfigurationPolicy", related.Object.Kind)
+	assert.Equal(t, name, related.Object.Metadata.Name)
+	assert.Equal(t, namespace, related.Object.Metadata.Namespace)
 
 	// add the same object and make sure the existing one is overwritten
 	reason = "new"
 	compliant = false
 	relatedList = addRelatedObjects(compliant, scopedGVR, "ConfigurationPolicy", namespace, []string{name}, reason, nil)
 	related = relatedList[0]
 
-	assert.True(t, len(relatedList) == 1)
-	assert.True(t, related.Compliant == string(policyv1.NonCompliant))
-	assert.True(t, related.Reason == "new")
+	assert.Len(t, relatedList, 1)
+	assert.Equal(t, string(policyv1.NonCompliant), related.Compliant)
+	assert.Equal(t, "new", related.Reason)
 
 	// add a new related object and make sure the entry is appended
 	name = "bar"
@@ -626,11 +638,11 @@ func TestAddRelatedObject(t *testing.T) {
 		addRelatedObjects(compliant, scopedGVR, "ConfigurationPolicy", namespace, []string{name}, reason, nil)...,
 	)
 
-	assert.True(t, len(relatedList) == 2)
+	assert.Len(t, relatedList, 2)
 
 	related = relatedList[1]
 
-	assert.True(t, related.Object.Metadata.Name == name)
+	assert.Equal(t, name, related.Object.Metadata.Name)
 }
 
 func TestUpdatedRelatedObjects(t *testing.T) {
@@ -670,15 +682,15 @@ func TestUpdatedRelatedObjects(t *testing.T) {
 	)
 
 	r.updatedRelatedObjects(policy, relatedList)
-	assert.True(t, relatedList[0].Object.Metadata.Name == "bar")
+	assert.Equal(t, "bar", relatedList[0].Object.Metadata.Name)
 
 	// append another object named bar but also with namespace bar
 	relatedList = append(relatedList, addRelatedObjects(
 		true, scopedGVR, "ConfigurationPolicy", "bar", []string{name}, "reason", nil)...,
 	)
 
 	r.updatedRelatedObjects(policy, relatedList)
-	assert.True(t, relatedList[0].Object.Metadata.Namespace == "bar")
+	assert.Equal(t, "bar", relatedList[0].Object.Metadata.Namespace)
 
 	// clear related objects and test sorting with no namespace
 	scopedGVR.Namespaced = false
@@ -690,7 +702,7 @@ func TestUpdatedRelatedObjects(t *testing.T) {
 	)
 
 	r.updatedRelatedObjects(policy, relatedList)
-	assert.True(t, relatedList[0].Object.Metadata.Name == "bar")
+	assert.Equal(t, "bar", relatedList[0].Object.Metadata.Name)
 }
 
 func TestCreateStatus(t *testing.T) {
@@ -1023,14 +1035,14 @@ func TestCreateStatus(t *testing.T) {
 	}
 
 	for _, test := range testcases {
-		test := test
+		local := test
 
-		t.Run(test.testName, func(t *testing.T) {
-			compliant, reason, msg := createStatus(test.resourceName, test.namespaceToEvent)
+		t.Run(local.testName, func(t *testing.T) {
+			compliant, reason, msg := createStatus(local.resourceName, local.namespaceToEvent)
 
-			assert.Equal(t, test.expectedCompliant, compliant)
-			assert.Equal(t, test.expectedReason, reason)
-			assert.Equal(t, test.expectedMsg, msg)
+			assert.Equal(t, local.expectedCompliant, compliant)
+			assert.Equal(t, local.expectedReason, reason)
+			assert.Equal(t, local.expectedMsg, msg)
 		})
 	}
 }
@@ -1122,7 +1134,7 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 	lastEvaluatedTwelveHoursAgo := &sync.Map{}
 	lastEvaluatedTwelveHoursAgo.Store(policy.UID, twelveHoursAgo)
 
-	tests := []struct {
+	type TestCaseStruct struct {
 		testDescription          string
 		lastEvaluated            string
 		lastEvaluatedGeneration  int64
@@ -1134,7 +1146,9 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		cleanupImmediately       bool
 		finalizers               []string
 		lastEvaluatedCache       *sync.Map
-	}{
+	}
+
+	tests := []TestCaseStruct{
 		{
 			"Just evaluated and the generation is unchanged",
 			inFuture,
@@ -1410,40 +1424,46 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		},
 	}
 
-	for _, test := range tests {
-		test := test
+	runTest := func(t *testing.T, test TestCaseStruct) {
+		t.Helper()
 
-		t.Run(
-			test.testDescription,
-			func(t *testing.T) {
-				t.Parallel()
-				policy := policy.DeepCopy()
+		policyCopy := policy.DeepCopy()
 
-				policy.Status.LastEvaluated = test.lastEvaluated
-				policy.Status.LastEvaluatedGeneration = test.lastEvaluatedGeneration
-				policy.Spec.EvaluationInterval = test.evaluationInterval
-				policy.Status.ComplianceState = test.complianceState
-				policy.ObjectMeta.DeletionTimestamp = test.deletionTimestamp
-				policy.ObjectMeta.Finalizers = test.finalizers
+		policyCopy.Status.LastEvaluated = test.lastEvaluated
+		policyCopy.Status.LastEvaluatedGeneration = test.lastEvaluatedGeneration
+		policyCopy.Spec.EvaluationInterval = test.evaluationInterval
+		policyCopy.Status.ComplianceState = test.complianceState
+		policyCopy.ObjectMeta.DeletionTimestamp = test.deletionTimestamp
+		policyCopy.ObjectMeta.Finalizers = test.finalizers
 
-				r := &ConfigurationPolicyReconciler{
-					SelectorReconciler: &fakeSR{},
-					lastEvaluatedCache: *test.lastEvaluatedCache, //nolint: govet
-				}
+		r := &ConfigurationPolicyReconciler{
+			SelectorReconciler: &fakeSR{},
+			lastEvaluatedCache: *test.lastEvaluatedCache, //nolint: govet
+		}
 
-				actual, actualDuration := r.shouldEvaluatePolicy(policy, test.cleanupImmediately)
+		actual, actualDuration := r.shouldEvaluatePolicy(policyCopy, test.cleanupImmediately)
 
-				if actual != test.expected {
-					t.Fatalf("expected %v but got %v", test.expected, actual)
-				}
+		if actual != test.expected {
+			t.Fatalf("expected %v but got %v", test.expected, actual)
+		}
 
-				if test.expectedPositiveDuration && actualDuration <= 0 {
-					t.Fatalf("expected a positive duration but got %v", actualDuration.String())
-				}
+		if test.expectedPositiveDuration && actualDuration <= 0 {
+			t.Fatalf("expected a positive duration but got %v", actualDuration.String())
+		}
 
-				if !test.expectedPositiveDuration && actualDuration > 0 {
-					t.Fatalf("expected a zero duration but got %v", actualDuration.String())
-				}
+		if !test.expectedPositiveDuration && actualDuration > 0 {
+			t.Fatalf("expected a zero duration but got %v", actualDuration.String())
+		}
+	}
+
+	for _, test := range tests {
+		local := test
+
+		t.Run(
+			test.testDescription,
+			func(t *testing.T) {
+				t.Parallel()
+				runTest(t, local)
 			},
 		)
 	}

Diff for: controllers/configurationpolicy_utils.go

@@ -239,7 +239,7 @@ func checkFieldsWithSort(
 				}
 
 				// An error indicates the value is a regular string, so check equality normally
-				if fmt.Sprint(oVal) != fmt.Sprint(mVal) {
+				if fmt.Sprint(oVal) != mVal {
 					return false
 				}
 			} else {

Diff for: controllers/configurationpolicy_utils_test.go

@@ -56,7 +56,7 @@ func TestFormatTemplateStringAnnotation(t *testing.T) {
 	}
 
 	policyTemplateFormatted := formatMetadata(policyTemplate)
-	assert.Equal(t, policyTemplateFormatted["annotations"], "not-an-annotation")
+	assert.Equal(t, "not-an-annotation", policyTemplateFormatted["annotations"])
 }
 
 func TestAddConditionToStatusNeverEvalInterval(t *testing.T) {
@@ -86,18 +86,18 @@ func TestAddConditionToStatusNeverEvalInterval(t *testing.T) {
 				addConditionToStatus(policy, 0, test.compliancy == policyv1.Compliant, "Some reason", "Some message")
 
 				details := policy.Status.CompliancyDetails
-				assert.Equal(t, len(details), 1)
+				assert.Len(t, details, 1)
 
 				detail := details[0]
 				conditions := detail.Conditions
-				assert.Equal(t, len(conditions), 1)
+				assert.Len(t, conditions, 1)
 
 				condition := conditions[0]
 				lowercaseCompliance := strings.ToLower(string(test.compliancy))
 				expectedMsg := `Some message. This policy will not be evaluated again due to ` +
 					fmt.Sprintf(`spec.evaluationInterval.%s being set to "never".`, lowercaseCompliance)
 
-				assert.Equal(t, condition.Message, expectedMsg)
+				assert.Equal(t, expectedMsg, condition.Message)
 			},
 		)
 	}