Add some more golangci-lint linters and fix the issues they find (#3234)

.github/workflows/ci-go.yml

@@ -11,6 +11,9 @@ on:
 permissions:
   contents: read
 
+env:
+  GOFLAGS: -tags=aro,containers_image_openpgp,exclude_graphdriver_btrfs,exclude_graphdriver_devicemapper
+
 jobs:
   ci-from-docker:
     runs-on: ubuntu-latest
@@ -30,11 +33,6 @@ jobs:
   vendor-check:
     runs-on: ubuntu-latest
     steps:
-    - name: Install dependencies
-      run: |
-        sudo apt-get update
-        sudo apt-get install libbtrfs-dev libgpgme-dev libdevmapper-dev
-
     - name: Checkout repository
       uses: actions/checkout@v4
 

.github/workflows/codeql-analysis.yml

@@ -15,6 +15,9 @@ permissions:
   contents: read
   security-events: write
 
+env:
+  GOFLAGS: -tags=aro,containers_image_openpgp,exclude_graphdriver_btrfs,exclude_graphdriver_devicemapper
+
 jobs:
   analyze:
     name: Analyze
@@ -32,12 +35,6 @@ jobs:
             runs-on: ['ubuntu-latest']
 
     steps:
-    - name: Install dependencies
-      if: matrix.language == 'go'
-      run: |
-        sudo apt-get update
-        sudo apt-get install libbtrfs-dev libgpgme-dev libdevmapper-dev
-
     - name: Checkout repository
       uses: actions/checkout@v4
 

.github/workflows/golint.yml

@@ -16,11 +16,6 @@ jobs:
     name: golangci-lint
     runs-on: ubuntu-latest
     steps:
-    - name: Install dependencies
-      run: |
-        sudo apt-get update
-        sudo apt-get install libbtrfs-dev libgpgme-dev libdevmapper-dev
-
     - name: Checkout repository
       uses: actions/checkout@v4
 
@@ -32,18 +27,13 @@ jobs:
     - name: Run golangci-lint
       uses: golangci/golangci-lint-action@v3
       with:
-        version: v1.49.0
+        version: v1.55.1
         args: -v --timeout 15m
 
   validate-go:
     name: validate-go
     runs-on: ubuntu-latest
     steps:
-    - name: Install dependencies
-      run: |
-        sudo apt-get update
-        sudo apt-get install libbtrfs-dev libgpgme-dev libdevmapper-dev
-
     - name: Checkout repository
       uses: actions/checkout@v4
 

.golangci.yml

@@ -13,6 +13,7 @@ run:
   go: "1.18"
 
 issues:
+  max-same-issues: 0
   exclude-rules:
     - linters:
         - staticcheck
@@ -185,3 +186,8 @@ linters:
     - stylecheck
     - unconvert
     - importas
+    - tenv
+    - musttag
+    - ginkgolinter
+    - testifylint
+    - usestdlibvars

pkg/cluster/loadbalancerprofile_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	mock_network "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/network"
@@ -124,7 +125,7 @@ func TestGetDesiredOutboundIPs(t *testing.T) {
 			outboundIPs, err := tt.m.getDesiredOutboundIPs(ctx)
 			assert.Equal(t, tt.expectedErr, err, "Unexpected error exception")
 			// results are not deterministic when scaling down so just check desired length
-			assert.Equal(t, tt.m.doc.OpenShiftCluster.Properties.NetworkProfile.LoadBalancerProfile.ManagedOutboundIPs.Count, len(outboundIPs))
+			assert.Len(t, outboundIPs, tt.m.doc.OpenShiftCluster.Properties.NetworkProfile.LoadBalancerProfile.ManagedOutboundIPs.Count)
 		})
 	}
 }
@@ -1007,7 +1008,7 @@ func TestReconcileLoadBalancerProfile(t *testing.T) {
 			if tt.expectedErr != nil {
 				assert.Contains(t, tt.expectedErr, err, "Unexpected error exception")
 			} else {
-				assert.Equal(t, nil, err, "Unexpected error exception")
+				require.NoError(t, err, "Unexpected error exception")
 			}
 			assert.Equal(t, &tt.expectedLoadBalancerProfile, &tt.m.doc.OpenShiftCluster.Properties.NetworkProfile.LoadBalancerProfile)
 		})

pkg/containerinstall/install_test.go

@@ -54,16 +54,14 @@ var _ = Describe("Podman", Ordered, func() {
 
 	It("can pull images", func() {
 		_, err = images.Pull(conn, TEST_PULLSPEC, (&images.PullOptions{}).WithPolicy("missing"))
-		Expect(err).To(BeNil())
+		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("can create a secret", func() {
-
 		secret, err = secrets.Create(
 			conn, bytes.NewBufferString("hello\n"),
 			(&secrets.CreateOptions{}).WithName(containerName))
-		Expect(err).To(BeNil())
-
+		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("can start a container", func() {
@@ -85,12 +83,12 @@ var _ = Describe("Podman", Ordered, func() {
 		s.Entrypoint = []string{"/bin/bash", "-c", "cat testfile"}
 
 		containerID, err = runContainer(conn, log, s)
-		Expect(err).To(BeNil())
+		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("can wait for completion", func() {
 		exit, err := containers.Wait(conn, containerID, (&containers.WaitOptions{}).WithCondition([]define.ContainerStatus{define.ContainerStateExited}))
-		Expect(err).To(BeNil())
+		Expect(err).ToNot(HaveOccurred())
 		Expect(exit).To(BeEquivalentTo(0), "exit code was %d, not 0", exit)
 	})
 
@@ -99,7 +97,7 @@ var _ = Describe("Podman", Ordered, func() {
 		Eventually(func(g Gomega) {
 			hook.Reset()
 			err = getContainerLogs(conn, log, containerID)
-			g.Expect(err).To(BeNil())
+			g.Expect(err).ToNot(HaveOccurred())
 			entries := []map[string]types.GomegaMatcher{
 				{
 					"msg":   Equal("stdout: hello\n"),
@@ -108,19 +106,19 @@ var _ = Describe("Podman", Ordered, func() {
 			}
 
 			err = testlog.AssertLoggingOutput(hook, entries)
-			g.Expect(err).To(BeNil())
+			g.Expect(err).ToNot(HaveOccurred())
 		}).WithTimeout(10 * time.Second).WithPolling(time.Second).Should(Succeed())
 	})
 
 	AfterAll(func() {
 		if containerID != "" {
 			_, err = containers.Remove(conn, containerID, (&containers.RemoveOptions{}).WithForce(true))
-			Expect(err).To(BeNil())
+			Expect(err).ToNot(HaveOccurred())
 		}
 
 		if secret != nil {
 			err = secrets.Remove(conn, secret.ID)
-			Expect(err).To(BeNil())
+			Expect(err).ToNot(HaveOccurred())
 		}
 
 		if cancel != nil {

pkg/dbtoken/server.go

@@ -97,7 +97,7 @@ func NewServer(
 }
 
 func healthCheck(w http.ResponseWriter, r *http.Request) {
-	w.WriteHeader(200)
+	w.WriteHeader(http.StatusOK)
 }
 
 func (s *server) Run(ctx context.Context) error {

pkg/metrics/statsd/metrics.go

@@ -25,11 +25,11 @@ type metric struct {
 // MarshalJSON marshals a metric into JSON format.
 func (m *metric) MarshalJSON() ([]byte, error) {
 	return json.Marshal(struct {
-		Metric    string
-		Account   string `json:"Account,omitempty"`
-		Namespace string `json:"Namespace,omitempty"`
-		Dims      map[string]string
-		TS        string
+		Metric    string            `json:"Metric"`
+		Account   string            `json:"Account,omitempty"`
+		Namespace string            `json:"Namespace,omitempty"`
+		Dims      map[string]string `json:"Dims"`
+		TS        string            `json:"TS"`
 	}{
 		Metric:    m.name,
 		Account:   m.account,

pkg/monitor/cluster/podconditions_test.go

@@ -292,7 +292,7 @@ func TestEmitPodContainerRestartCounter(t *testing.T) {
 	mon._emitPodContainerRestartCounter(ps)
 
 	// Matches the number of emitted messages
-	assert.Equal(t, 3, len(hook.Entries))
+	assert.Len(t, hook.Entries, 3)
 
 	// the order of the log entries does not seem to be stable, so testing one entry only
 	// and no test for specific values, except for the metric

pkg/portal/cluster_test.go

@@ -70,7 +70,7 @@ func TestClusterList(t *testing.T) {
 		dbOpenShiftClusters: dbOpenShiftClusters,
 	}
 
-	req, err := http.NewRequest("GET", "/api/clusters", nil)
+	req, err := http.NewRequest(http.MethodGet, "/api/clusters", nil)
 	if err != nil {
 		t.Error(err)
 	}
@@ -191,7 +191,7 @@ func TestClusterDetail(t *testing.T) {
 		dbOpenShiftClusters: dbOpenShiftClusters,
 	}
 
-	req, err := http.NewRequest("GET", "/api/00000000-0000-0000-0000-000000000000/resourcegroupname/succeeded", nil)
+	req, err := http.NewRequest(http.MethodGet, "/api/00000000-0000-0000-0000-000000000000/resourcegroupname/succeeded", nil)
 	if err != nil {
 		t.Error(err)
 	}

pkg/portal/regions_test.go

@@ -7,7 +7,6 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
-	"os"
 	"testing"
 
 	"github.com/go-test/deep"
@@ -32,9 +31,9 @@ func TestRegionListPublic(t *testing.T) {
 		dbOpenShiftClusters: dbOpenShiftClusters,
 	}
 
-	os.Setenv("AZURE_ENVIRONMENT", azureclient.PublicCloud.Environment.Name)
+	t.Setenv("AZURE_ENVIRONMENT", azureclient.PublicCloud.Environment.Name)
 
-	req, err := http.NewRequest("GET", "/api/regions", nil)
+	req, err := http.NewRequest(http.MethodGet, "/api/regions", nil)
 	if err != nil {
 		t.Error(err)
 	}
@@ -243,9 +242,9 @@ func TestRegionListFF(t *testing.T) {
 		dbOpenShiftClusters: dbOpenShiftClusters,
 	}
 
-	os.Setenv("AZURE_ENVIRONMENT", azureclient.USGovernmentCloud.Environment.Name)
+	t.Setenv("AZURE_ENVIRONMENT", azureclient.USGovernmentCloud.Environment.Name)
 
-	req, err := http.NewRequest("GET", "/api/regions", nil)
+	req, err := http.NewRequest(http.MethodGet, "/api/regions", nil)
 	if err != nil {
 		t.Error(err)
 	}

pkg/portal/security_test.go

@@ -233,7 +233,7 @@ func TestSecurity(t *testing.T) {
 			checkResponse: func(t *testing.T, authenticated, elevated bool, resp *http.Response) {
 				if authenticated && !elevated {
 					var e struct {
-						Error string
+						Error string `json:"error,omitempty"`
 					}
 					err := json.NewDecoder(resp.Body).Decode(&e)
 					if err != nil {

pkg/util/arm/types.go

@@ -29,7 +29,7 @@ type TemplateParameter struct {
 
 // Resource represents an ARM template resource
 type Resource struct {
-	Resource interface{}
+	Resource interface{} `json:"-"`
 
 	Name       string                 `json:"name,omitempty"`
 	Type       string                 `json:"type,omitempty"`

pkg/util/dynamichelper/dynamichelper_test.go

@@ -51,7 +51,7 @@ func TestEsureDeleted(t *testing.T) {
 		NegotiatedSerializer: resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer,
 		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
 			switch req.Method {
-			case "DELETE":
+			case http.MethodDelete:
 				switch req.URL.Path {
 				case "/apis/metal3.io/v1alpha1/namespaces/test-ns-1/configmap/test-name-1":
 					return &http.Response{StatusCode: http.StatusNotFound}, nil

pkg/util/installer/certs.go

@@ -22,8 +22,8 @@ type CertKeyInterface interface {
 // CertKey contains the private key and the cert.
 // See openshift/installer/pkg/asset/tls/certkey.go
 type CertKey struct {
-	CertRaw []byte
-	KeyRaw  []byte
+	CertRaw []byte `json:"CertRaw"`
+	KeyRaw  []byte `json:"KeyRaw"`
 }
 
 // Cert returns the certificate.

pkg/util/pem/pem_test.go

@@ -15,12 +15,12 @@ import (
 
 var _ = Describe("PEM", func() {
 	validCaKey, validCaCerts, err := utiltls.GenerateKeyAndCertificate("validca", nil, nil, true, false)
-	Expect(err).To(BeNil())
+	Expect(err).ToNot(HaveOccurred())
 
 	Describe("encoding keys", func() {
 		It("succeeds", func() {
 			keyOut, err := Encode(validCaKey)
-			Expect(err).To(BeNil())
+			Expect(err).ToNot(HaveOccurred())
 			Expect(keyOut).To(ContainSubstring("BEGIN RSA PRIVATE KEY"))
 		})
 	})
@@ -28,7 +28,7 @@ var _ = Describe("PEM", func() {
 	Describe("encoding single certificate", func() {
 		It("succeeds", func() {
 			certsOut, err := Encode(validCaCerts...)
-			Expect(err).To(BeNil())
+			Expect(err).ToNot(HaveOccurred())
 			Expect(certsOut).To(ContainSubstring("BEGIN CERTIFICATE"))
 		})
 	})
@@ -36,7 +36,7 @@ var _ = Describe("PEM", func() {
 	Describe("encoding multiple certificates", func() {
 		It("succeeds", func() {
 			certsOut, err := Encode(validCaCerts[0], validCaCerts[0])
-			Expect(err).To(BeNil())
+			Expect(err).ToNot(HaveOccurred())
 			Expect(bytes.Count(certsOut, []byte("BEGIN CERTIFICATE"))).To(Equal(2))
 		})
 	})

pkg/util/pullsecret/extract_test.go

@@ -15,7 +15,7 @@ var _ = Describe("Extract()", func() {
 		pullSecret := "{\"auths\": {\"example.com\": {\"auth\": \"dGVzdHVzZXI6dGVzdHBhc3M=\"}}}"
 
 		correctlyExtracted, err := Extract(pullSecret, "example.com")
-		Expect(err).To(BeNil())
+		Expect(err).ToNot(HaveOccurred())
 		Expect(correctlyExtracted).To(Equal(&UserPass{Username: "testuser", Password: "testpass"}))
 	})
 

test/e2e/admin_portal.go

@@ -204,7 +204,7 @@ var _ = Describe("Admin Portal E2E Testing", func() {
 
 		regions, err := regionList.FindElements(selenium.ByTagName, "li")
 		Expect(err).ToNot(HaveOccurred())
-		Expect(len(regions)).To(Equal(NUMBER_OF_REGIONS))
+		Expect(regions).To(HaveLen(NUMBER_OF_REGIONS))
 
 		for _, region := range regions {
 			link, err := region.FindElement(selenium.ByTagName, "a")

test/e2e/adminapi_drainnode.go

@@ -26,7 +26,7 @@ var _ = Describe("[Admin API] Cordon and Drain node actions", func() {
 			LabelSelector: "node-role.kubernetes.io/worker",
 		})
 		Expect(err).NotTo(HaveOccurred())
-		Expect(len(nodes.Items)).Should(BeNumerically(">", 0))
+		Expect(nodes.Items).ShouldNot(BeEmpty())
 		node := nodes.Items[0]
 		nodeName := node.Name
 

test/e2e/adminapi_redeployvm.go

@@ -39,7 +39,7 @@ var _ = Describe("[Admin API] VM redeploy action", func() {
 		By("picking the first VM to redeploy")
 		vms, err := clients.VirtualMachines.List(ctx, clusterResourceGroup)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(vms).NotTo(HaveLen(0))
+		Expect(vms).NotTo(BeEmpty())
 		vm := vms[0]
 		log.Infof("selected vm: %s", *vm.Name)
 

test/e2e/disk_encryption.go

@@ -55,13 +55,13 @@ var _ = Describe("Encryption at host", func() {
 		By("listing all VMs for the test cluster")
 		vms, err := clients.VirtualMachines.List(ctx, clusterResourceGroup)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(vms).NotTo(HaveLen(0))
+		Expect(vms).NotTo(BeEmpty())
 
 		By("checking the encryption property on each VM")
 		for _, vm := range vms {
 			Expect(vm.SecurityProfile).To(Not(BeNil()))
 			Expect(vm.SecurityProfile.EncryptionAtHost).To(Not(BeNil()))
-			Expect(*vm.SecurityProfile.EncryptionAtHost).To(Equal(true))
+			Expect(*vm.SecurityProfile.EncryptionAtHost).To(BeTrue())
 		}
 	})
 })
@@ -98,7 +98,7 @@ var _ = Describe("Disk encryption at rest", func() {
 		By("listing all VMs")
 		vms, err := clients.VirtualMachines.List(ctx, clusterResourceGroup)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(vms).NotTo(HaveLen(0))
+		Expect(vms).NotTo(BeEmpty())
 
 		// We have to get the disks by VM, because when getting all disks by resource group,
 		// we do not get recently created disks, see https://github.com/Azure/azure-cli/issues/17123
@@ -117,8 +117,8 @@ var _ = Describe("Disk encryption at rest", func() {
 		Expect(sc.Annotations["storageclass.kubernetes.io/is-default-class"]).To(Equal("true"))
 
 		By("making sure the encrypted storage class uses worker disk encryption set")
-		expectedDiskEncryptionSetID := ((*oc.OpenShiftClusterProperties.WorkerProfiles)[0].DiskEncryptionSetID)
+		expectedDiskEncryptionSetID := *((*oc.OpenShiftClusterProperties.WorkerProfiles)[0].DiskEncryptionSetID)
 		Expect(sc.Parameters).NotTo(BeNil())
-		Expect(sc.Parameters["diskEncryptionSetID"]).NotTo(Equal(expectedDiskEncryptionSetID))
+		Expect(sc.Parameters["diskEncryptionSetID"]).To(Equal(expectedDiskEncryptionSetID))
 	})
 })

test/e2e/list.go

@@ -25,7 +25,7 @@ var _ = Describe("List clusters", func() {
 				break
 			}
 		}
-		Expect(found).To(Equal(true))
+		Expect(found).To(BeTrue())
 	})
 
 	// listByResourceGroup test marked Pending (X), don't reenable until ARM caching issue is fixed, see https://github.com/Azure/ARO-RP/pull/1995
@@ -42,6 +42,6 @@ var _ = Describe("List clusters", func() {
 				break
 			}
 		}
-		Expect(found).To(Equal(true))
+		Expect(found).To(BeTrue())
 	})
 })

test/e2e/monitor.go

@@ -28,6 +28,6 @@ var _ = Describe("Monitor", func() {
 
 		By("running the monitor once")
 		errs := mon.Monitor(ctx)
-		Expect(errs).To(HaveLen(0))
+		Expect(errs).To(BeEmpty())
 	})
 })

test/e2e/operations.go

@@ -14,6 +14,6 @@ var _ = Describe("List operations", func() {
 	It("must return the correct static operations", func(ctx context.Context) {
 		opList, err := clients.Operations.List(ctx)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(len(opList) > 0).To(BeTrue())
+		Expect(opList).ToNot(BeEmpty())
 	})
 })

test/e2e/operator.go

@@ -279,9 +279,9 @@ var _ = Describe("ARO Operator - Conditions", func() {
 			co, err := clients.ConfigClient.ConfigV1().ClusterOperators().Get(ctx, "aro", metav1.GetOptions{})
 			g.Expect(err).NotTo(HaveOccurred())
 
-			g.Expect(cov1Helpers.IsStatusConditionTrue(co.Status.Conditions, configv1.OperatorAvailable))
-			g.Expect(cov1Helpers.IsStatusConditionFalse(co.Status.Conditions, configv1.OperatorProgressing))
-			g.Expect(cov1Helpers.IsStatusConditionFalse(co.Status.Conditions, configv1.OperatorDegraded))
+			g.Expect(cov1Helpers.IsStatusConditionTrue(co.Status.Conditions, configv1.OperatorAvailable)).To(BeTrue())
+			g.Expect(cov1Helpers.IsStatusConditionFalse(co.Status.Conditions, configv1.OperatorProgressing)).To(BeTrue())
+			g.Expect(cov1Helpers.IsStatusConditionFalse(co.Status.Conditions, configv1.OperatorDegraded)).To(BeTrue())
 		}).WithContext(ctx).WithTimeout(DefaultEventuallyTimeout).WithTimeout(timeout).Should(Succeed())
 	})
 })

test/e2e/update.go

@@ -82,7 +82,7 @@ var _ = Describe("Update cluster Managed Outbound IPs", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		By("checking effectiveOutboundIPs has been updated")
-		Expect(len(*oc.NetworkProfile.LoadBalancerProfile.EffectiveOutboundIps)).To(Equal(5))
+		Expect(*oc.NetworkProfile.LoadBalancerProfile.EffectiveOutboundIps).To(HaveLen(5))
 
 		By("checking outbound-rule-4 has required number IPs")
 		lb, err := clients.LoadBalancers.Get(ctx, rgName, lbName, "")
@@ -99,7 +99,7 @@ var _ = Describe("Update cluster Managed Outbound IPs", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		By("checking effectiveOutboundIPs has been updated")
-		Expect(len(*oc.NetworkProfile.LoadBalancerProfile.EffectiveOutboundIps)).To(Equal(1))
+		Expect(*oc.NetworkProfile.LoadBalancerProfile.EffectiveOutboundIps).To(HaveLen(1))
 
 		By("checking outbound-rule-4 has required number of IPs")
 		lb, err = clients.LoadBalancers.Get(ctx, rgName, lbName, "")

test/util/kubeadminkubeconfig/kubetoken.go

@@ -60,7 +60,7 @@ func getTokenURLFromConsoleURL(consoleURL string) (*url.URL, error) {
 }
 
 func getAuthorizedToken(ctx context.Context, tokenURL *url.URL, username, password string) (string, error) {
-	req, err := http.NewRequestWithContext(ctx, "GET", tokenURL.String(), nil)
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, tokenURL.String(), nil)
 	if err != nil {
 		return "", err
 	}