Integrate golangci-lint to run aggregate linting checks for CI (#2536)

* Introduce golangci-lint support

Introduce a root .golangci-lint.yaml file to the repository. This file
is responsible for housing the golangci-lint configuration, and the
various aggregate linters that are enabled/disabled.

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

* .github/workflows: Run the golangci-lint action during the sanity checks

Update the sanity workflow and add the golangci-lint action.

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

* Fix linting violations outlined by golangci-lint

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

Author: timflannagan

.github/workflows/sanity.yaml

@@ -13,7 +13,11 @@ jobs:
       - uses: actions/setup-go@v2
         with:
           go-version: '~1.16'
-      - name: Install goimports
-        run: go install golang.org/x/tools/cmd/goimports@latest
       - name: Run sanity checks
-        run: make vendor && make lint && make diff
+        run: make vendor && make diff
+      - name: Run linting checks
+        uses: "golangci/golangci-lint-action@v2"
+        with:
+          version: "v1.43"
+          skip-go-installation: true
+          skip-pkg-cache: true

.golangci.yaml

@@ -0,0 +1,30 @@
+run:
+  timeout: 5m
+  skip-dirs:
+  - pkg/lib
+  - pkg/api
+  - pkg/fakes
+  - pkg/package-server/apis
+  - test/e2e
+
+linters:
+  enable:
+  - depguard
+  - gofmt
+  - goimports
+  - importas
+  - misspell
+  - stylecheck
+  - tparallel
+  - unconvert
+  - whitespace
+  disable:
+  - errcheck
+
+issues:
+  max-issues-per-linter: 0
+  max-same-issues: 0
+
+output:
+  format: tab
+  sort-results: true

cmd/catalog/main.go

@@ -69,7 +69,7 @@ var (
 	tlsCertPath = flag.String(
 		"tls-cert", "", "Path to use for certificate key (requires tls-key)")
 
-	profiling = flag.Bool("profiling", false, "deprecated")
+	_ = flag.Bool("profiling", false, "deprecated")
 
 	clientCAPath = flag.String("client-ca", "", "path to watch for client ca bundle")
 

cmd/olm/main.go

@@ -63,7 +63,7 @@ var (
 	tlsCertPath = pflag.String(
 		"tls-cert", "", "Path to use for certificate key (requires tls-key)")
 
-	profiling = pflag.Bool("profiling", false, "deprecated")
+	_ = pflag.Bool("profiling", false, "deprecated")
 
 	clientCAPath = pflag.String("client-ca", "", "path to watch for client ca bundle")
 

cmd/olm/manager.go

@@ -118,7 +118,6 @@ func Manager(ctx context.Context, debug bool) (ctrl.Manager, error) {
 		if err = adoptionReconciler.SetupWithManager(mgr); err != nil {
 			return nil, err
 		}
-
 	}
 	setupLog.Info("manager configured")
 

pkg/controller/bundle/bundle_unpacker.go

@@ -384,7 +384,6 @@ const (
 )
 
 func (c *ConfigMapUnpacker) UnpackBundle(lookup *operatorsv1alpha1.BundleLookup, timeout time.Duration) (result *BundleUnpackResult, err error) {
-
 	result = newBundleUnpackResult(lookup)
 
 	// if bundle lookup failed condition already present, then there is nothing more to do
@@ -523,8 +522,8 @@ func (c *ConfigMapUnpacker) pendingContainerStatusMessages(job *batchv1.Job) (st
 	podLabel := map[string]string{BundleUnpackPodLabel: job.GetName()}
 	pods, listErr := c.podLister.Pods(job.GetNamespace()).List(k8slabels.SelectorFromValidatedSet(podLabel))
 	if listErr != nil {
-		c.logger.Errorf("Failed to list pods for job(%s): %v", job.GetName(), listErr)
-		return "", fmt.Errorf("Failed to list pods for job(%s): %v", job.GetName(), listErr)
+		c.logger.Errorf("failed to list pods for job(%s): %v", job.GetName(), listErr)
+		return "", fmt.Errorf("failed to list pods for job(%s): %v", job.GetName(), listErr)
 	}
 
 	// Ideally there should be just 1 pod running but inspect all pods in the pending phase
@@ -570,14 +569,14 @@ func (c *ConfigMapUnpacker) ensureConfigmap(csRef *corev1.ObjectReference, name
 		if err != nil && apierrors.IsAlreadyExists(err) {
 			cm, err = c.client.CoreV1().ConfigMaps(fresh.GetNamespace()).Get(context.TODO(), fresh.GetName(), metav1.GetOptions{})
 			if err != nil {
-				return nil, fmt.Errorf("Failed to retrieve configmap %s: %v", fresh.GetName(), err)
+				return nil, fmt.Errorf("failed to retrieve configmap %s: %v", fresh.GetName(), err)
 			}
 			cm.SetLabels(map[string]string{
 				install.OLMManagedLabelKey: install.OLMManagedLabelValue,
 			})
 			cm, err = c.client.CoreV1().ConfigMaps(cm.GetNamespace()).Update(context.TODO(), cm, metav1.UpdateOptions{})
 			if err != nil {
-				return nil, fmt.Errorf("Failed to update configmap %s: %v", cm.GetName(), err)
+				return nil, fmt.Errorf("failed to update configmap %s: %v", cm.GetName(), err)
 			}
 		}
 	}

pkg/controller/bundle/bundle_unpacker_test.go

@@ -43,7 +43,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateAPIService(caPEM []byte, des
 	} else {
 		csv, ok := i.owner.(*v1alpha1.ClusterServiceVersion)
 		if !ok {
-			return fmt.Errorf("APIServices require a CSV Owner.")
+			return fmt.Errorf("failed to typecast the APIService owner: APIServices require a CSV owner")
 		}
 
 		adoptable, err := IsAPIServiceAdoptable(i.strategyClient.GetOpLister(), csv, apiService)

pkg/controller/install/apiservice.go

@@ -315,7 +315,6 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
 			logger.Warnf("could not update secret %s", secret.GetName())
 			return nil, nil, err
 		}
-
 	} else if k8serrors.IsNotFound(err) {
 		// Create the secret
 		ownerutil.AddNonBlockingOwner(secret, i.owner)

pkg/controller/install/certresources.go

@@ -37,7 +37,7 @@ func selector(t *testing.T, selector string) *metav1.LabelSelector {
 	return s
 }
 
-var staticCerts *certs.KeyPair = nil
+var staticCerts *certs.KeyPair
 
 // staticCertGenerator replaces the CertGenerator to get consistent keys for testing
 func staticCertGenerator(notAfter time.Time, organization string, ca *certs.KeyPair, hosts []string) (*certs.KeyPair, error) {
@@ -124,7 +124,6 @@ func TestInstallCertRequirementsForDeployment(t *testing.T) {
 	assert.NoError(t, err)
 	caHash := certs.PEMSHA256(caPEM)
 	type fields struct {
-		strategyClient         wrappers.InstallStrategyDeploymentInterface
 		owner                  ownerutil.Owner
 		previousStrategy       Strategy
 		templateAnnotations    map[string]string

pkg/controller/install/certresources_test.go

@@ -179,23 +179,6 @@ func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1
 	return
 }
 
-func (i *StrategyDeploymentInstaller) cleanupPrevious(current *v1alpha1.StrategyDetailsDeployment, previous *v1alpha1.StrategyDetailsDeployment) error {
-	previousDeploymentsMap := map[string]struct{}{}
-	for _, d := range previous.DeploymentSpecs {
-		previousDeploymentsMap[d.Name] = struct{}{}
-	}
-	for _, d := range current.DeploymentSpecs {
-		delete(previousDeploymentsMap, d.Name)
-	}
-	log.Debugf("preparing to cleanup: %s", previousDeploymentsMap)
-	// delete deployments in old strategy but not new
-	var err error = nil
-	for name := range previousDeploymentsMap {
-		err = i.strategyClient.DeleteDeployment(name)
-	}
-	return err
-}
-
 func (i *StrategyDeploymentInstaller) Install(s Strategy) error {
 	strategy, ok := s.(*v1alpha1.StrategyDetailsDeployment)
 	if !ok {
@@ -235,11 +218,6 @@ func (i *StrategyDeploymentInstaller) CheckInstalled(s Strategy) (installed bool
 }
 
 func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []v1alpha1.StrategyDeploymentSpec) error {
-	var depNames []string
-	for _, dep := range deploymentSpecs {
-		depNames = append(depNames, dep.Name)
-	}
-
 	// Check the owner is a CSV
 	csv, ok := i.owner.(*v1alpha1.ClusterServiceVersion)
 	if !ok {
@@ -273,7 +251,7 @@ func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []v1al
 
 		// check annotations
 		if len(i.templateAnnotations) > 0 && dep.Spec.Template.Annotations == nil {
-			return StrategyError{Reason: StrategyErrReasonAnnotationsMissing, Message: fmt.Sprintf("no annotations found on deployment")}
+			return StrategyError{Reason: StrategyErrReasonAnnotationsMissing, Message: "no annotations found on deployment"}
 		}
 		for key, value := range i.templateAnnotations {
 			if actualValue, ok := dep.Spec.Template.Annotations[key]; !ok {
@@ -286,11 +264,11 @@ func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []v1al
 		// check that the deployment spec hasn't changed since it was created
 		labels := dep.GetLabels()
 		if len(labels) == 0 {
-			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment doesn't have a spec hash, update it")}
+			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment %s doesn't have a spec hash, update it", dep.Name)}
 		}
 		existingDeploymentSpecHash, ok := labels[DeploymentSpecHashLabelKey]
 		if !ok {
-			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment doesn't have a spec hash, update it")}
+			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment %s doesn't have a spec hash, update it", dep.Name)}
 		}
 
 		_, calculatedDeploymentHash, err := i.deploymentForSpec(spec.Name, spec.Spec, labels)

pkg/controller/install/deployment.go

@@ -67,7 +67,6 @@ func (c *CSVRuleChecker) RuleSatisfied(sa *corev1.ServiceAccount, namespace stri
 		if decision == authorizer.DecisionDeny || decision == authorizer.DecisionNoOpinion {
 			return false, nil
 		}
-
 	}
 
 	return true, nil

pkg/controller/install/rule_checker.go

@@ -23,7 +23,6 @@ import (
 )
 
 func TestRuleSatisfied(t *testing.T) {
-
 	csv := &v1alpha1.ClusterServiceVersion{}
 	csv.SetName("barista-operator")
 	csv.SetUID(types.UID("barista-operator"))
@@ -606,7 +605,6 @@ func NewFakeCSVRuleChecker(k8sObjs []runtime.Object, csv *v1alpha1.ClusterServic
 	ruleChecker := NewCSVRuleChecker(roleInformer.Lister(), roleBindingInformer.Lister(), clusterRoleInformer.Lister(), clusterRoleBindingInformer.Lister(), csv)
 
 	return ruleChecker, nil
-
 }
 
 func Objs(roles []*rbacv1.Role, roleBindings []*rbacv1.RoleBinding, clusterRoles []*rbacv1.ClusterRole, clusterRoleBindings []*rbacv1.ClusterRoleBinding) []runtime.Object {

pkg/controller/install/rule_checker_test.go

@@ -24,18 +24,18 @@ func ValidWebhookRules(rules []admissionregistrationv1.RuleWithOperations) error
 
 		// protect OLM resources
 		if contains(apiGroupMap, "*") {
-			return fmt.Errorf("Webhook rules cannot include all groups")
+			return fmt.Errorf("webhook rules cannot include all groups")
 		}
 
 		if contains(apiGroupMap, "operators.coreos.com") {
-			return fmt.Errorf("Webhook rules cannot include the OLM group")
+			return fmt.Errorf("webhook rules cannot include the OLM group")
 		}
 
 		// protect Admission Webhook resources
 		if contains(apiGroupMap, "admissionregistration.k8s.io") {
 			resourceGroupMap := listToMap(rule.Resources)
 			if contains(resourceGroupMap, "*") || contains(resourceGroupMap, "MutatingWebhookConfiguration") || contains(resourceGroupMap, "ValidatingWebhookConfiguration") {
-				return fmt.Errorf("Webhook rules cannot include MutatingWebhookConfiguration or ValidatingWebhookConfiguration resources")
+				return fmt.Errorf("webhook rules cannot include MutatingWebhookConfiguration or ValidatingWebhookConfiguration resources")
 			}
 		}
 	}
@@ -58,7 +58,7 @@ func contains(m map[string]struct{}, tar string) bool {
 func (i *StrategyDeploymentInstaller) createOrUpdateWebhook(caPEM []byte, desc v1alpha1.WebhookDescription) error {
 	operatorGroups, err := i.strategyClient.GetOpLister().OperatorsV1().OperatorGroupLister().OperatorGroups(i.owner.GetNamespace()).List(labels.Everything())
 	if err != nil || len(operatorGroups) != 1 {
-		return fmt.Errorf("Error retrieving OperatorGroup info")
+		return fmt.Errorf("error retrieving OperatorGroup info")
 	}
 	ogNamespacelabelSelector, err := operatorGroups[0].NamespaceLabelSelector()
 	if err != nil {
@@ -188,15 +188,14 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by
 	// get a list of owned CRDs
 	csv, ok := i.owner.(*v1alpha1.ClusterServiceVersion)
 	if !ok {
-		return fmt.Errorf("ConversionWebhook owner must be a ClusterServiceVersion")
+		return fmt.Errorf("unable to manage conversion webhook: conversion webhook owner must be a ClusterServiceVersion")
 	}
-
 	if !isSingletonOperator(*csv) {
-		return fmt.Errorf("CSVs with conversion webhooks must support only AllNamespaces")
+		return fmt.Errorf("unable to manage conversion webhook: CSVs with conversion webhooks must support only AllNamespaces")
 	}
 
 	if len(desc.ConversionCRDs) == 0 {
-		return fmt.Errorf("Conversion Webhook must have at least one CRD specified")
+		return fmt.Errorf("unable to manager conversion webhook: conversion webhook must have at least one CRD specified")
 	}
 
 	// iterate over all the ConversionCRDs
@@ -205,7 +204,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by
 			// Get existing CRD on cluster
 			crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
 			if err != nil {
-				return fmt.Errorf("Unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err)
+				return fmt.Errorf("unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err)
 			}
 
 			// check if this CRD is an owned CRD
@@ -217,7 +216,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by
 				}
 			}
 			if !foundCRD {
-				return fmt.Errorf("CSV %s does not own CRD %s", csv.GetName(), conversionCRD)
+				return fmt.Errorf("csv %s does not own CRD %s", csv.GetName(), conversionCRD)
 			}
 
 			// crd.Spec.Conversion.Strategy specifies how custom resources are converted between versions.
@@ -230,7 +229,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by
 			// By default the strategy is none
 			// Reference:
 			// 	- https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
-			if crd.Spec.PreserveUnknownFields != false {
+			if crd.Spec.PreserveUnknownFields {
 				return fmt.Errorf("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion")
 			}
 

pkg/controller/install/webhook.go

@@ -37,7 +37,6 @@ type AdoptionReconciler struct {
 	client.Client
 
 	log     logr.Logger
-	mu      sync.RWMutex
 	factory decorators.OperatorFactory
 }
 
@@ -268,7 +267,7 @@ func (r *AdoptionReconciler) adopt(ctx context.Context, operator *decorators.Ope
 
 	cObj, ok := component.(client.Object)
 	if !ok {
-		return fmt.Errorf("Unable to typecast runtime.Object to client.Object")
+		return fmt.Errorf("unable to typecast runtime.Object to client.Object")
 	}
 
 	if err := r.Get(ctx, types.NamespacedName{Namespace: m.GetNamespace(), Name: m.GetName()}, cObj); err != nil {
@@ -290,7 +289,7 @@ func (r *AdoptionReconciler) adopt(ctx context.Context, operator *decorators.Ope
 		// Only update if freshly adopted
 		pCObj, ok := candidate.(client.Object)
 		if !ok {
-			return fmt.Errorf("Unable to typecast runtime.Object to client.Object")
+			return fmt.Errorf("unable to typecast runtime.Object to client.Object")
 		}
 		return r.Patch(ctx, pCObj, client.MergeFrom(cObj))
 	}
@@ -301,7 +300,7 @@ func (r *AdoptionReconciler) adopt(ctx context.Context, operator *decorators.Ope
 func (r *AdoptionReconciler) disown(ctx context.Context, operator *decorators.Operator, component runtime.Object) error {
 	cObj, ok := component.(client.Object)
 	if !ok {
-		return fmt.Errorf("Unable to typecast runtime.Object to client.Object")
+		return fmt.Errorf("unable to typecast runtime.Object to client.Object")
 	}
 	candidate := component.DeepCopyObject()
 	disowned, err := operator.DisownComponent(candidate)
@@ -318,15 +317,15 @@ func (r *AdoptionReconciler) disown(ctx context.Context, operator *decorators.Op
 	r.log.V(1).Info("component disowned", "component", candidate)
 	uCObj, ok := candidate.(client.Object)
 	if !ok {
-		return fmt.Errorf("Unable to typecast runtime.Object to client.Object")
+		return fmt.Errorf("unable to typecast runtime.Object to client.Object")
 	}
 	return r.Patch(ctx, uCObj, client.MergeFrom(cObj))
 }
 
 func (r *AdoptionReconciler) disownFromAll(ctx context.Context, component runtime.Object) error {
 	cObj, ok := component.(client.Object)
 	if !ok {
-		return fmt.Errorf("Unable to typecast runtime.Object to client.Object")
+		return fmt.Errorf("unable to typecast runtime.Object to client.Object")
 	}
 	var operators []decorators.Operator
 	for _, name := range decorators.OperatorNames(cObj.GetLabels()) {
@@ -362,7 +361,7 @@ func (r *AdoptionReconciler) adoptees(ctx context.Context, operator decorators.O
 	for _, list := range componentLists {
 		cList, ok := list.(client.ObjectList)
 		if !ok {
-			return nil, fmt.Errorf("Unable to typecast runtime.Object to client.ObjectList")
+			return nil, fmt.Errorf("unable to typecast runtime.Object to client.ObjectList")
 		}
 		if err := r.List(ctx, cList, opt); err != nil {
 			return nil, err