Enable ginkgolinter and fix findings (#126)

* Enable ginkgolinter and fix findings

ginkgolinter finds bugs and enforces standards of using the ginkgo and
gomega packages. See more details here:
https://github.com/nunnatsa/ginkgolinter

This PR enables the ginkgolinter in the .golangci.yml, and fixes all the
new finding from running golangci-lint.

Note: all the finding were auto fixed by running the ginkgolinter cli
with the `-fix` flag.

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

* Fix the assertMetrics test helper function

The assertMetrics function in
handler/instrumented_enqueue_object_test.go, wasn't really doing
anything. The switch-case compared a pointer value to another pointer,
so the no case was actually ever selected, because the addresses were
not the same.

Tested with a debugger - no value were ever selected.

This commit fixes this test by selecting the value rather than the
address.

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

* Simplify error check in tests

Replace this patter:
```golang
err := someFuncRetOnlyErr()
Expect(err).ToNot(HaveOccurred()
```
With
```golang
Expect(someFuncRetOnlyErr()).To(Succeed())
```

Also, use the `MatchError` gomega matcher when checking errors.

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

---------

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

Author: nunnatsa

.golangci.yml

@@ -7,6 +7,7 @@ linters:
   - asciicheck
   - bodyclose
   - errorlint
+  - ginkgolinter
   - gofmt
   - goimports
   - gosec

conditions/conditions_test.go

@@ -64,25 +64,21 @@ var _ = Describe("Condition", func() {
 			}
 
 			// Create Operator Condition
-			err := os.Setenv(operatorCondEnvVar, "operator-condition-test")
-			Expect(err).NotTo(HaveOccurred())
+			Expect(os.Setenv(operatorCondEnvVar, "operator-condition-test")).To(Succeed())
 			readNamespace = func() (string, error) {
 				return ns, nil
 			}
 
 			// create a new client
 			sch := runtime.NewScheme()
-			err = apiv2.AddToScheme(sch)
-			Expect(err).NotTo(HaveOccurred())
+			Expect(apiv2.AddToScheme(sch)).To(Succeed())
 			cl = fake.NewClientBuilder().WithScheme(sch).WithStatusSubresource(operatorCond).Build()
 
 			// create an operator Condition resource
-			err = cl.Create(ctx, operatorCond)
-			Expect(err).NotTo(HaveOccurred())
+			Expect(cl.Create(ctx, operatorCond)).To(Succeed())
 
 			// Update its status
-			err = cl.Status().Update(ctx, operatorCond)
-			Expect(err).NotTo(HaveOccurred())
+			Expect(cl.Status().Update(ctx, operatorCond)).To(Succeed())
 		})
 
 		AfterEach(func() {
@@ -107,14 +103,12 @@ var _ = Describe("Condition", func() {
 				Expect(err).NotTo(HaveOccurred())
 
 				con, err := c.Get(ctx)
-				Expect(err).To(HaveOccurred())
+				Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("conditionType %v not found", conditionBar))))
 				Expect(con).To(BeNil())
-				Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("conditionType %v not found", conditionBar)))
 			})
 
 			It("should error when operator Condition is not present in cluster", func() {
-				err := os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")
-				Expect(err).NotTo(HaveOccurred())
+				Expect(os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")).To(Succeed())
 
 				By("setting the status of a new condition")
 				c, err := NewCondition(cl, conditionFoo)
@@ -131,13 +125,11 @@ var _ = Describe("Condition", func() {
 				By("setting the status of an existing condition")
 				c, err := NewCondition(cl, conditionFoo)
 				Expect(err).NotTo(HaveOccurred())
-				err = c.Set(ctx, metav1.ConditionFalse, WithReason("not_in_foo_state"), WithMessage("test"))
-				Expect(err).NotTo(HaveOccurred())
+				Expect(c.Set(ctx, metav1.ConditionFalse, WithReason("not_in_foo_state"), WithMessage("test"))).To(Succeed())
 
 				By("fetching the condition from cluster")
 				op := &apiv2.OperatorCondition{}
-				err = cl.Get(ctx, objKey, op)
-				Expect(err).NotTo(HaveOccurred())
+				Expect(cl.Get(ctx, objKey, op)).To(Succeed())
 
 				By("checking if the condition has been updated")
 				res := op.Spec.Conditions[0]
@@ -150,22 +142,19 @@ var _ = Describe("Condition", func() {
 				By("setting the status of a new condition")
 				c, err := NewCondition(cl, conditionBar)
 				Expect(err).NotTo(HaveOccurred())
-				err = c.Set(ctx, metav1.ConditionTrue, WithReason("in_bar_state"), WithMessage("test"))
-				Expect(err).NotTo(HaveOccurred())
+				Expect(c.Set(ctx, metav1.ConditionTrue, WithReason("in_bar_state"), WithMessage("test"))).To(Succeed())
 
 				By("fetching the condition from cluster")
 				op := &apiv2.OperatorCondition{}
-				err = cl.Get(ctx, objKey, op)
-				Expect(err).NotTo(HaveOccurred())
+				Expect(cl.Get(ctx, objKey, op)).To(Succeed())
 
 				By("checking if the condition has been updated")
 				res := op.Spec.Conditions
-				Expect(len(res)).To(BeEquivalentTo(2))
+				Expect(res).To(HaveLen(2))
 				Expect(meta.IsStatusConditionTrue(res, string(conditionBar))).To(BeTrue())
 			})
 			It("should error when operator Condition is not present in cluster", func() {
-				err := os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")
-				Expect(err).NotTo(HaveOccurred())
+				Expect(os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")).To(Succeed())
 
 				By("setting the status of a new condition")
 				c, err := NewCondition(cl, conditionBar)

conditions/factory_test.go

@@ -36,8 +36,7 @@ var _ = Describe("NewCondition", func() {
 	var cl client.Client
 	BeforeEach(func() {
 		sch := runtime.NewScheme()
-		err := apiv2.AddToScheme(sch)
-		Expect(err).NotTo(HaveOccurred())
+		Expect(apiv2.AddToScheme(sch)).To(Succeed())
 		cl = fake.NewClientBuilder().WithScheme(sch).Build()
 	})
 
@@ -56,8 +55,7 @@ var _ = Describe("InClusterFactory", func() {
 
 	BeforeEach(func() {
 		sch := runtime.NewScheme()
-		err := apiv2.AddToScheme(sch)
-		Expect(err).NotTo(HaveOccurred())
+		Expect(apiv2.AddToScheme(sch)).To(Succeed())
 		cl = fake.NewClientBuilder().WithScheme(sch).Build()
 		f = InClusterFactory{cl}
 	})
@@ -73,8 +71,7 @@ var _ = Describe("InClusterFactory", func() {
 
 func testNewCondition(fn func(apiv2.ConditionType) (Condition, error)) {
 	It("should create a new condition", func() {
-		err := os.Setenv(operatorCondEnvVar, "test-operator-condition")
-		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Setenv(operatorCondEnvVar, "test-operator-condition")).To(Succeed())
 		readNamespace = func() (string, error) {
 			return "default", nil
 		}
@@ -85,8 +82,7 @@ func testNewCondition(fn func(apiv2.ConditionType) (Condition, error)) {
 	})
 
 	It("should error when namespacedName cannot be found", func() {
-		err := os.Unsetenv(operatorCondEnvVar)
-		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Unsetenv(operatorCondEnvVar)).To(Succeed())
 
 		c, err := fn(conditionFoo)
 		Expect(err).To(HaveOccurred())
@@ -96,32 +92,27 @@ func testNewCondition(fn func(apiv2.ConditionType) (Condition, error)) {
 
 func testGetNamespacedName(fn func() (*types.NamespacedName, error)) {
 	It("should error when name of the operator condition cannot be found", func() {
-		err := os.Unsetenv(operatorCondEnvVar)
-		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Unsetenv(operatorCondEnvVar)).To(Succeed())
 
 		objKey, err := fn()
-		Expect(err).To(HaveOccurred())
+		Expect(err).To(MatchError(ContainSubstring("could not determine operator condition name")))
 		Expect(objKey).To(BeNil())
-		Expect(err.Error()).To(ContainSubstring("could not determine operator condition name"))
 	})
 
 	It("should error when object namespace cannot be found", func() {
-		err := os.Setenv(operatorCondEnvVar, "test")
-		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Setenv(operatorCondEnvVar, "test")).To(Succeed())
 
 		readNamespace = func() (string, error) {
 			return "", os.ErrNotExist
 		}
 
 		objKey, err := fn()
-		Expect(err).To(HaveOccurred())
+		Expect(err).To(MatchError(ContainSubstring("get operator condition namespace: file does not exist")))
 		Expect(objKey).To(BeNil())
-		Expect(err.Error()).To(ContainSubstring("get operator condition namespace: file does not exist"))
 	})
 
 	It("should return the right namespaced name from SA namespace file", func() {
-		err := os.Setenv(operatorCondEnvVar, "test")
-		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Setenv(operatorCondEnvVar, "test")).To(Succeed())
 
 		readNamespace = func() (string, error) {
 			return "testns", nil
@@ -135,6 +126,5 @@ func testGetNamespacedName(fn func() (*types.NamespacedName, error)) {
 }
 
 func deleteCondition(ctx context.Context, client client.Client, obj client.Object) {
-	err := client.Delete(ctx, obj)
-	Expect(err).NotTo(HaveOccurred())
+	Expect(client.Delete(ctx, obj)).To(Succeed())
 }

handler/enqueue_annotation_test.go

@@ -56,8 +56,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
 
 		podOwner.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Kind: "Pod"})
 
-		err := SetOwnerAnnotations(podOwner, pod)
-		Expect(err).To(BeNil())
+		Expect(SetOwnerAnnotations(podOwner, pod)).To(Succeed())
 		instance = EnqueueRequestForAnnotation{
 			Type: schema.GroupKind{
 				Group: "",
@@ -92,8 +91,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
 				},
 			}
 
-			err := SetOwnerAnnotations(podOwner, repl)
-			Expect(err).To(BeNil())
+			Expect(SetOwnerAnnotations(podOwner, repl)).To(Succeed())
 
 			evt := event.CreateEvent{
 				Object: repl,
@@ -248,8 +246,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
 			newPod.Name = pod.Name + "2"
 			newPod.Namespace = pod.Namespace + "2"
 
-			err := SetOwnerAnnotations(podOwner, pod)
-			Expect(err).To(BeNil())
+			Expect(SetOwnerAnnotations(podOwner, pod)).To(Succeed())
 
 			evt := event.UpdateEvent{
 				ObjectOld: pod,
@@ -337,8 +334,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
 			newPod.Name = pod.Name + "2"
 			newPod.Namespace = pod.Namespace + "2"
 
-			err := SetOwnerAnnotations(podOwner, pod)
-			Expect(err).To(BeNil())
+			Expect(SetOwnerAnnotations(podOwner, pod)).To(Succeed())
 
 			var podOwner2 = &corev1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
@@ -348,8 +344,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
 			}
 			podOwner2.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Kind: "Pod"})
 
-			err = SetOwnerAnnotations(podOwner2, newPod)
-			Expect(err).To(BeNil())
+			Expect(SetOwnerAnnotations(podOwner2, newPod)).To(Succeed())
 
 			evt := event.UpdateEvent{
 				ObjectOld: pod,
@@ -389,16 +384,15 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
 				},
 			}
 
-			err := SetOwnerAnnotations(podOwner, nd)
-			Expect(err).To(BeNil())
+			Expect(SetOwnerAnnotations(podOwner, nd)).To(Succeed())
 
 			expected := map[string]string{
 				"my-test-annotation":     "should-keep",
 				NamespacedNameAnnotation: "podOwnerNs/podOwnerName",
 				TypeAnnotation:           schema.GroupKind{Group: "", Kind: "Pod"}.String(),
 			}
 
-			Expect(len(nd.GetAnnotations())).To(Equal(3))
+			Expect(nd.GetAnnotations()).To(HaveLen(3))
 			Expect(nd.GetAnnotations()).To(Equal(expected))
 		})
 		It("should return error when the owner Kind is not present", func() {
@@ -409,8 +403,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
 			}
 
 			podOwner.SetGroupVersionKind(schema.GroupVersionKind{Group: "Pod", Kind: ""})
-			err := SetOwnerAnnotations(podOwner, nd)
-			Expect(err).NotTo(BeNil())
+			Expect(SetOwnerAnnotations(podOwner, nd)).ToNot(Succeed())
 		})
 		It("should return an error when the owner Name is not set", func() {
 			nd := &corev1.Node{
@@ -426,8 +419,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() {
 			}
 
 			ownerNew.SetGroupVersionKind(schema.GroupVersionKind{Group: "Pod", Kind: ""})
-			err := SetOwnerAnnotations(ownerNew, nd)
-			Expect(err).NotTo(BeNil())
+			Expect(SetOwnerAnnotations(ownerNew, nd)).ToNot(Succeed())
 		})
 	})
 })

handler/instrumented_enqueue_object_test.go

@@ -79,7 +79,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() {
 			// verify metrics
 			gauges, err := registry.Gather()
 			Expect(err).NotTo(HaveOccurred())
-			Expect(len(gauges)).To(Equal(1))
+			Expect(gauges).To(HaveLen(1))
 			assertMetrics(gauges[0], 1, []*corev1.Pod{pod})
 		})
 	})
@@ -114,7 +114,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() {
 				// verify metrics
 				gauges, err := registry.Gather()
 				Expect(err).NotTo(HaveOccurred())
-				Expect(len(gauges)).To(Equal(0))
+				Expect(gauges).To(BeEmpty())
 			})
 		})
 		Context("when a gauge does not exist", func() {
@@ -139,7 +139,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() {
 				// verify metrics
 				gauges, err := registry.Gather()
 				Expect(err).NotTo(HaveOccurred())
-				Expect(len(gauges)).To(Equal(0))
+				Expect(gauges).To(BeEmpty())
 			})
 		})
 
@@ -174,7 +174,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() {
 			// verify metrics
 			gauges, err := registry.Gather()
 			Expect(err).NotTo(HaveOccurred())
-			Expect(len(gauges)).To(Equal(1))
+			Expect(gauges).To(HaveLen(1))
 			assertMetrics(gauges[0], 2, []*corev1.Pod{newpod, pod})
 		})
 	})
@@ -183,7 +183,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() {
 		It("should fill out map with values from given objects", func() {
 			labelMap := getResourceLabels(pod)
 			Expect(labelMap).ShouldNot(BeEmpty())
-			Expect(len(labelMap)).To(Equal(5))
+			Expect(labelMap).To(HaveLen(5))
 			Expect(labelMap["name"]).To(Equal(pod.GetObjectMeta().GetName()))
 			Expect(labelMap["namespace"]).To(Equal(pod.GetObjectMeta().GetNamespace()))
 			Expect(labelMap["group"]).To(Equal(pod.GetObjectKind().GroupVersionKind().Group))
@@ -195,28 +195,30 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() {
 
 func assertMetrics(gauge *dto.MetricFamily, count int, pods []*corev1.Pod) {
 	// need variables to compare the pointers
-	name := "name"
-	namespace := "namespace"
-	g := "group"
-	v := "version"
-	k := "kind"
-
-	Expect(len(gauge.Metric)).To(Equal(count))
+	const (
+		name      = "name"
+		namespace = "namespace"
+		g         = "group"
+		v         = "version"
+		k         = "kind"
+	)
+
+	Expect(gauge.Metric).To(HaveLen(count))
 	for i := 0; i < count; i++ {
 		Expect(*gauge.Metric[i].Gauge.Value).To(Equal(float64(pods[i].GetObjectMeta().GetCreationTimestamp().UTC().Unix())))
 
 		for _, l := range gauge.Metric[i].Label {
-			switch l.Name {
-			case &name:
-				Expect(l.Value).To(Equal(pods[i].GetObjectMeta().GetName()))
-			case &namespace:
-				Expect(l.Value).To(Equal(pods[i].GetObjectMeta().GetNamespace()))
-			case &g:
-				Expect(l.Value).To(Equal(pods[i].GetObjectKind().GroupVersionKind().Group))
-			case &v:
-				Expect(l.Value).To(Equal(pods[i].GetObjectKind().GroupVersionKind().Version))
-			case &k:
-				Expect(l.Value).To(Equal(pods[i].GetObjectKind().GroupVersionKind().Kind))
+			switch *l.Name {
+			case name:
+				Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectMeta().GetName())))
+			case namespace:
+				Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectMeta().GetNamespace())))
+			case g:
+				Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectKind().GroupVersionKind().Group)))
+			case v:
+				Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectKind().GroupVersionKind().Version)))
+			case k:
+				Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectKind().GroupVersionKind().Kind)))
 			}
 		}
 	}

internal/utils/utils_test.go

@@ -38,7 +38,7 @@ var _ = Describe("Helpers test", func() {
 
 			// test
 			namespace, err := GetOperatorNamespace()
-			Expect(err).Should(BeNil())
+			Expect(err).ShouldNot(HaveOccurred())
 			Expect(namespace).To(Equal("testnamespace"))
 		})
 		It("should trim whitespace from namespace", func() {
@@ -48,7 +48,7 @@ var _ = Describe("Helpers test", func() {
 
 			// test
 			namespace, err := GetOperatorNamespace()
-			Expect(err).Should(BeNil())
+			Expect(err).ShouldNot(HaveOccurred())
 			Expect(namespace).To(Equal("testnamespace"))
 		})
 	})