(ci): add linting + fix existing errors (#99)

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
operator-framework · Jun 20, 2023 · 748a2a4 · 748a2a4
1 parent 83da08b
commit 748a2a4
Show file tree

Hide file tree

Showing 13 changed files with 1,056 additions and 20 deletions.
diff --git a/.bingo/Variables.mk b/.bingo/Variables.mk
@@ -35,6 +35,12 @@ $(GINKGO): $(BINGO_DIR)/ginkgo.mod
 	@echo "(re)installing $(GOBIN)/ginkgo-v2.9.7"
 	@cd $(BINGO_DIR) && GOWORK=off $(GO) build -mod=mod -modfile=ginkgo.mod -o=$(GOBIN)/ginkgo-v2.9.7 "github.com/onsi/ginkgo/v2/ginkgo"
 
+GOLANGCI_LINT := $(GOBIN)/golangci-lint-v1.53.3
+$(GOLANGCI_LINT): $(BINGO_DIR)/golangci-lint.mod
+	@# Install binary/ries using Go 1.14+ build command. This is using bwplotka/bingo-controlled, separate go module with pinned dependencies.
+	@echo "(re)installing $(GOBIN)/golangci-lint-v1.53.3"
+	@cd $(BINGO_DIR) && GOWORK=off $(GO) build -mod=mod -modfile=golangci-lint.mod -o=$(GOBIN)/golangci-lint-v1.53.3 "github.com/golangci/golangci-lint/cmd/golangci-lint"
+
 GORELEASER := $(GOBIN)/goreleaser-v1.16.2
 $(GORELEASER): $(BINGO_DIR)/goreleaser.mod
 	@# Install binary/ries using Go 1.14+ build command. This is using bwplotka/bingo-controlled, separate go module with pinned dependencies.

diff --git a/.bingo/golangci-lint.mod b/.bingo/golangci-lint.mod
@@ -0,0 +1,5 @@
+module _ // Auto generated by https://github.com/bwplotka/bingo. DO NOT EDIT
+
+go 1.20
+
+require github.com/golangci/golangci-lint v1.53.3 // cmd/golangci-lint
diff --git a/.bingo/golangci-lint.sum b/.bingo/golangci-lint.sum
diff --git a/.bingo/variables.env b/.bingo/variables.env
@@ -14,6 +14,8 @@ CONTROLLER_GEN="${GOBIN}/controller-gen-v0.11.4"
 
 GINKGO="${GOBIN}/ginkgo-v2.9.7"
 
+GOLANGCI_LINT="${GOBIN}/golangci-lint-v1.53.3"
+
 GORELEASER="${GOBIN}/goreleaser-v1.16.2"
 
 KIND="${GOBIN}/kind-v0.15.0"

diff --git a/.github/workflows/sanity.yaml b/.github/workflows/sanity.yaml
@@ -15,13 +15,14 @@ jobs:
       - uses: actions/setup-go@v4
         with:
           go-version-file: "go.mod"
-      - uses: actions/cache@v3
-        with:
-          path: |
-            ~/.cache/go-build
-            ~/go/pkg/mod
-          key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
-          restore-keys: |
-            ${{ runner.os }}-go-
       - name: Run verification checks
         run: make verify
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-go@v4
+        with:
+          go-version-file: "go.mod"
+      - name: Run lint checks
+        run: make lint GOLANGCI_LINT_ARGS="--out-format github-actions"
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -0,0 +1,51 @@
+run:
+  # Default timeout is 1m, up to give more room
+  timeout: 4m
+
+linters:
+  enable:
+  - asciicheck
+  - bodyclose
+  - errorlint
+  - ginkgolinter
+  - gofmt
+  - goimports
+  - gosec
+  - importas
+  - misspell
+  - nestif
+  - nonamedreturns
+  - prealloc
+  - revive
+  - stylecheck
+  - tparallel
+  - unconvert
+  - unparam
+  - unused
+  - whitespace
+
+linters-settings:
+  errorlint:
+    errorf: false
+
+  importas:
+    alias:
+    - pkg: k8s.io/apimachinery/pkg/apis/meta/v1
+      alias: metav1
+    - pkg: k8s.io/apimachinery/pkg/api/errors
+      alias: apierrors
+    - pkg: k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1
+      alias: apiextensionsv1
+    - pkg: k8s.io/apimachinery/pkg/util/runtime
+      alias: utilruntime
+    - pkg: "^k8s\\.io/api/([^/]+)/(v[^/]+)$"
+      alias: $1$2
+    - pkg: sigs.k8s.io/controller-runtime
+      alias: ctrl
+    - pkg: github.com/operator-framework/rukpak/api/v1alpha1
+      alias: rukpakv1alpha1
+  goimports:
+    local-prefixes: github.com/operator-framework/catalogd
+
+output:
+  format: tab
diff --git a/Makefile b/Makefile
@@ -77,6 +77,10 @@ tidy: ## Update dependencies
 verify: tidy fmt vet generate ## Verify the current code generation and lint
 	git diff --exit-code
 
+.PHONY: lint
+lint: $(GOLANGCI_LINT) ## Run golangci linter.
+	$(GOLANGCI_LINT) run $(GOLANGCI_LINT_ARGS)
+
 ##@ Build
 
 BINARIES=manager

diff --git a/cmd/manager/main.go b/cmd/manager/main.go
@@ -32,12 +32,13 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
+	"github.com/spf13/pflag"
+
 	"github.com/operator-framework/catalogd/internal/source"
 	"github.com/operator-framework/catalogd/internal/version"
 	corecontrollers "github.com/operator-framework/catalogd/pkg/controllers/core"
 	"github.com/operator-framework/catalogd/pkg/features"
 	"github.com/operator-framework/catalogd/pkg/profile"
-	"github.com/spf13/pflag"
 
 	//+kubebuilder:scaffold:imports
 	"github.com/operator-framework/catalogd/api/core/v1alpha1"

diff --git a/pkg/controllers/core/catalog_controller.go b/pkg/controllers/core/catalog_controller.go
@@ -110,6 +110,14 @@ func (r *CatalogReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
+// Note: This function always returns ctrl.Result{}. The linter
+// fusses about this as we could instead just return error. This was
+// discussed in https://github.com/operator-framework/rukpak/pull/635#discussion_r1229859464
+// and the consensus was that it is better to keep the ctrl.Result return
+// type so that if we do end up needing to return something else we don't forget
+// to add the ctrl.Result type back as a return value. Adding a comment to ignore
+// linting from the linter that was fussing about this.
+// nolint:unparam
 func (r *CatalogReconciler) reconcile(ctx context.Context, catalog *v1alpha1.Catalog) (ctrl.Result, error) {
 	unpackResult, err := r.Unpacker.Unpack(ctx, catalog)
 	if err != nil {
@@ -146,7 +154,6 @@ func (r *CatalogReconciler) reconcile(ctx context.Context, catalog *v1alpha1.Cat
 	default:
 		return ctrl.Result{}, updateStatusUnpackFailing(&catalog.Status, fmt.Errorf("unknown unpack state %q: %v", unpackResult.State, err))
 	}
-
 }
 
 func updateStatusUnpackPending(status *v1alpha1.CatalogStatus, result *source.Result) {
@@ -254,7 +261,8 @@ func (r *CatalogReconciler) syncBundleMetadata(ctx context.Context, declCfg *dec
 	if err := r.List(ctx, &existingBundles); err != nil {
 		return fmt.Errorf("list existing bundle metadatas: %v", err)
 	}
-	for _, existingBundle := range existingBundles.Items {
+	for i := range existingBundles.Items {
+		existingBundle := existingBundles.Items[i]
 		if _, ok := newBundles[existingBundle.Name]; !ok {
 			if err := r.Delete(ctx, &existingBundle); err != nil {
 				return fmt.Errorf("delete existing bundle metadata %q: %v", existingBundle.Name, err)
@@ -340,7 +348,8 @@ func (r *CatalogReconciler) syncPackages(ctx context.Context, declCfg *declcfg.D
 	if err := r.List(ctx, &existingPkgs); err != nil {
 		return fmt.Errorf("list existing packages: %v", err)
 	}
-	for _, existingPkg := range existingPkgs.Items {
+	for i := range existingPkgs.Items {
+		existingPkg := existingPkgs.Items[i]
 		if _, ok := newPkgs[existingPkg.Name]; !ok {
 			// delete existing package
 			if err := r.Delete(ctx, &existingPkg); err != nil {

diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go
@@ -32,7 +32,7 @@ type MockSource struct {
 	shouldError bool
 }
 
-func (ms *MockSource) Unpack(ctx context.Context, catalog *v1alpha1.Catalog) (*source.Result, error) {
+func (ms *MockSource) Unpack(_ context.Context, _ *v1alpha1.Catalog) (*source.Result, error) {
 	if ms.shouldError {
 		return nil, errors.New("mocksource error")
 	}
@@ -314,7 +314,7 @@ var _ = Describe("Catalogd Controller Test", func() {
 					Expect(pack.Spec.Channels).To(HaveLen(1))
 					Expect(pack.Spec.Channels[0].Name).To(Equal(testChannelName))
 					Expect(pack.Spec.Channels[0].Entries).To(HaveLen(1))
-					Expect(Expect(pack.Spec.Channels[0].Entries[0].Name).To(Equal(testBundleName)))
+					Expect(pack.Spec.Channels[0].Entries[0].Name).To(Equal(testBundleName))
 				})
 			})
 		})

diff --git a/pkg/controllers/core/suite_test.go b/pkg/controllers/core/suite_test.go
@@ -22,14 +22,15 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	catalogdv1alpha1 "github.com/operator-framework/catalogd/api/core/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+
+	catalogdv1alpha1 "github.com/operator-framework/catalogd/api/core/v1alpha1"
 )
 
 // These tests use Ginkgo (BDD-style Go testing framework). Refer to

diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go
@@ -6,11 +6,12 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1"
 )
 
 var (

diff --git a/test/e2e/unpack_test.go b/test/e2e/unpack_test.go
@@ -7,12 +7,13 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1"
-	v1 "k8s.io/api/core/v1"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+
+	catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1"
 )
 
 const (
@@ -66,7 +67,7 @@ var _ = Describe("Catalog Unpacking", func() {
 			By("Ensuring the expected Package resource is created")
 			pack := &catalogd.Package{}
 			expectedPackSpec := catalogd.PackageSpec{
-				Catalog: v1.LocalObjectReference{
+				Catalog: corev1.LocalObjectReference{
 					Name: catalogName,
 				},
 				Channels: []catalogd.PackageChannel{
@@ -89,7 +90,7 @@ var _ = Describe("Catalog Unpacking", func() {
 			By("Ensuring the expected BundleMetadata resource is created")
 			bm := &catalogd.BundleMetadata{}
 			expectedBMSpec := catalogd.BundleMetadataSpec{
-				Catalog: v1.LocalObjectReference{
+				Catalog: corev1.LocalObjectReference{
 					Name: catalogName,
 				},
 				Package: pkg,