feat(linter): introduces linter to check for problematic code (#41)

This way we don not have to spend time on it during code reviews and can focus on features instead. golangci-lint is set of linters which is highly customizable. This PR enables it both as a make target and GitHub action.

I fixed all the problems so we can start fresh.

.github/workflows/lint.yml

@@ -0,0 +1,36 @@
+name: linters
+on:
+ push:
+ branches:
+ - master
+ pull_request:
+jobs:
+ go-lint:
+ name: golangci-lint
+ runs-on: ubuntu-20.04
+ env:
+ REPOSITORY: ${{ github.repository }}
+ steps:
+ - name: Set up Go env
+ uses: actions/setup-go@v4
+ with:
+ go-version: 1.19
+ - uses: actions/checkout@v3
+ with:
+ fetch-depth: 1
+ path: go/src/github.com/${{ env.REPOSITORY }}
+ - name: Set $GOPATH
+ run: |
+ echo "GOPATH=${{ github.workspace }}/go" >> $GITHUB_ENV
+ echo "${{ github.workspace }}/go/bin" >> $GITHUB_PATH
+ shell: bash
+ - name: Prepare codebase for linter (generates deps, stubs)
+ run: |
+ cd go/src/github.com/${{ env.REPOSITORY }}
+ make deps generate
+ shell: bash
+ - name: golangci-lint
+ uses: golangci/golangci-lint-action@v3
+ with:
+ version: v1.53
+ working-directory: go/src/github.com/${{ env.REPOSITORY }}

.golangci.yml

@@ -0,0 +1,112 @@
+linters-settings:
+ govet:
+ check-shadowing: true
+ golint:
+ min-confidence: 0
+ gocyclo:
+ min-complexity: 16
+ cyclop:
+ max-complexity: 16
+ dupl:
+ threshold: 128
+ funlen:
+ lines: 128
+ statements: 64
+ goconst:
+ min-len: 4
+ min-occurrences: 3
+ depguard:
+ list-type: blacklist
+ packages:
+ - github.com/sirupsen/logrus
+ - sigs.k8s.io/controller-runtime/pkg/log
+ - sigs.k8s.io/controller-runtime/pkg/log/zap
+ - sigs.k8s.io/controller-runtime/pkg/runtime/log
+ misspell:
+ locale: US
+ ignore-words:
+ - istio
+ - k8s
+ lll:
+ line-length: 180
+ goimports:
+ local-prefixes: github.com/maistra/odh-project-controller
+ gocritic:
+ enabled-tags:
+ - performance
+ - style
+ - experimental
+ disabled-checks:
+ - wrapperFunc
+ - commentFormatting # https://github.com/go-critic/go-critic/issues/755
+ - hugeParam # seems to be premature optimization based on https://github.com/Maistra/istio-workspace/pull/378#discussion_r392208906
+ nestif:
+ min-complexity: 8
+ unused:
+ check-exported: true
+ gocognit:
+ min-complexity: 20
+ wrapcheck:
+ ignoreSigs:
+ - .Errorf(
+ - errors.New(
+ - errors.Unwrap(
+ - .Wrap(
+ - .Wrapf(
+ - .WithMessage(
+ - errors.WrapIfWithDetails
+ - errors.WithDetails(
+ - errors.WrapWithDetails(
+ - errors.WrapIf(
+ - errors.NewWithDetails(
+
+linters:
+ enable-all: true
+ disable:
+ - depguard
+ - exhaustruct
+ - exhaustivestruct
+ - forbidigo
+ - gofmt # We use goimports and when using them both leads to contradicting errors
+ - gofumpt
+ - gomnd
+ - paralleltest
+ - prealloc
+
+run:
+ deadline: 10m
+
+output:
+ format: colored-line-number
+ print-issued-lines: true
+ print-linter-name: true
+
+issues:
+ max-issues-per-linter: 0
+ max-same-issues: 0
+ exclude-rules:
+ - path: test/
+ linters:
+ - goconst
+ - gocyclo
+ - golint
+ - errcheck
+ - dupl
+ - gosec
+ - revive
+ - stylecheck
+ - wrapcheck
+ # Exclude particular linters for tests files.
+ - path: _test\.go
+ linters:
+ - gochecknoglobals
+ - gocyclo
+ - errcheck
+ - dupl
+ - gosec
+ - revive
+ - wrapcheck
+ - path: _suite_test\.go
+ linters:
+ - revive
+ - unused

Makefile

@@ -9,7 +9,7 @@ PACKAGE_NAME:=github.com/opendatahub-io/$(PROJECT_NAME)
 SHELL = /usr/bin/env bash -o pipefail
 
 .PHONY: all
-all: tools test build
+all: tools lint test build
 
 ##@ General
 .PHONY: help
@@ -26,16 +26,19 @@ generate: tools ## Generates required resources for the controller to work prope
 
 SRC_DIRS:=./controllers ./test
 SRCS:=$(shell find ${SRC_DIRS} -name "*.go")
-.PHONY: fmt
-fmt: $(SRCS) ## Formats the code.
- $(LOCALBIN)/goimports -l -w -e $(SRC_DIRS)
 
-.PHONY: vet
-vet: ## Run go vet against code.
- go vet ./...
+.PHONY: format
+format: $(SRCS) ## Removes unneeded imports and formats source code
+ $(call header,"Formatting code")
+ $(LOCALBIN)/goimports -l -w -e $(SRC_DIRS) $(TEST_DIRS)
+
+.PHONY: lint
+lint: tools ## Concurrently runs a whole bunch of static analysis tools
+ $(call header,"Running a whole bunch of static analysis tools")
+ $(LOCALBIN)/golangci-lint run --fix --sort-results
 
 .PHONY: test
-test: generate fmt vet
+test: generate
 test: test-unit+kube-envtest ## Run all tests. You can also select a category by running e.g. make test-unit or make test-kube-envtest
 
 ENVTEST_K8S_VERSION = 1.26 # refers to the version of kubebuilder assets to be downloaded by envtest binary.
@@ -73,14 +76,14 @@ deps:
  go mod download && go mod tidy
 
 .PHONY: build
-build: deps generate fmt vet go-build ## Build manager binary.
+build: deps format generate go-build ## Build manager binary.
 
 .PHONY: go-build
 go-build:
  ${GOBUILD} go build -ldflags "${LDFLAGS}" -o bin/manager main.go
 
 .PHONY: run
-run: generate fmt vet ## Run a controller from your host.
+run: format generate ## Run a controller from your host.
  go run ./main.go
 
 ##@ Container images
@@ -122,7 +125,7 @@ $(shell mkdir -p $(LOCALBIN))
 tools: deps
 tools: $(LOCALBIN)/controller-gen $(LOCALBIN)/kustomize ## Installs required tools in local ./bin folder
 tools: $(LOCALBIN)/setup-envtest $(LOCALBIN)/ginkgo
-tools: $(LOCALBIN)/goimports
+tools: $(LOCALBIN)/goimports $(LOCALBIN)/golangci-lint
 
 KUSTOMIZE_VERSION ?= v5.0.1
 $(LOCALBIN)/kustomize:
@@ -145,5 +148,10 @@ $(LOCALBIN)/ginkgo:
  GOBIN=$(LOCALBIN) go install -mod=readonly github.com/onsi/ginkgo/v2/ginkgo
 
 $(LOCALBIN)/goimports:
- $(call header,"Installing goimports")
+ $(call header,"Installing $(notdir $@)")
  GOBIN=$(LOCALBIN) go install -mod=readonly golang.org/x/tools/cmd/goimports
+
+LINT_VERSION=v1.53.3
+$(LOCALBIN)/golangci-lint:
+ $(call header,"Installing $(notdir $@)")
+ curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(LOCALBIN) $(LINT_VERSION)

controllers/controller_suite_test.go

@@ -6,19 +6,16 @@ import (
  "testing"
  "time"
 
+ . "github.com/onsi/ginkgo/v2"
+ . "github.com/onsi/gomega"
  "github.com/opendatahub-io/odh-project-controller/controllers"
  "github.com/opendatahub-io/odh-project-controller/test/labels"
+ "go.uber.org/zap/zapcore"
  v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
  "k8s.io/apimachinery/pkg/runtime"
  utilruntime "k8s.io/apimachinery/pkg/util/runtime"
  maistramanifests "maistra.io/api/manifests"
-
- "go.uber.org/zap/zapcore"
  ctrl "sigs.k8s.io/controller-runtime"
-
- . "github.com/onsi/ginkgo/v2"
- . "github.com/onsi/gomega"
-
  "sigs.k8s.io/controller-runtime/pkg/client"
  "sigs.k8s.io/controller-runtime/pkg/envtest"
  logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -39,7 +36,7 @@ func TestController(t *testing.T) {
  RunSpecs(t, "Controller & Webhook Suite")
 }
 
-var _ = BeforeSuite(func() {
+var _ = SynchronizedBeforeSuite(func() {
  if !Label(labels.EvnTest).MatchesLabelFilter(GinkgoLabelFilter()) {
  return
  }
@@ -91,21 +88,23 @@ var _ = BeforeSuite(func() {
  defer GinkgoRecover()
  Expect(mgr.Start(ctx)).To(Succeed(), "Failed to start manager")
  }()
-})
+}, func() {})
 
-var _ = AfterSuite(func() {
+var _ = SynchronizedAfterSuite(func() {
  if !Label(labels.EvnTest).MatchesLabelFilter(GinkgoLabelFilter()) {
  return
  }
  By("Tearing down the test environment")
  cancel()
  Expect(envTest.Stop()).To(Succeed())
-})
+}, func() {})
 
 func loadCRDs() []*v1.CustomResourceDefinition {
  smmYaml, err := maistramanifests.ReadManifest("maistra.io_servicemeshmembers.yaml")
  Expect(err).NotTo(HaveOccurred())
+
  crd := &v1.CustomResourceDefinition{}
+
  err = controllers.ConvertToStructuredResource(smmYaml, crd)
  Expect(err).NotTo(HaveOccurred())
 

controllers/converter.go

@@ -4,21 +4,24 @@ import (
  "bytes"
 
  "github.com/manifestival/manifestival"
+ "github.com/pkg/errors"
  "k8s.io/client-go/kubernetes/scheme"
 )
 
 func ConvertToStructuredResource(yamlContent []byte, out interface{}, opts ...manifestival.Option) error {
  reader := bytes.NewReader(yamlContent)
- m, err := manifestival.ManifestFrom(manifestival.Reader(reader), opts...)
+
+ manifest, err := manifestival.ManifestFrom(manifestival.Reader(reader), opts...)
  if err != nil {
- return err
+ return errors.Wrap(err, "failed reading manifest")
  }
 
  s := scheme.Scheme
  RegisterSchemes(s)
- err = s.Convert(&m.Resources()[0], out, nil)
- if err != nil {
- return err
+
+ if err := s.Convert(&manifest.Resources()[0], out, nil); err != nil {
+ return errors.Wrap(err, "failed converting manifest")
  }
+
  return nil
 }