chore: lint code with golangci-lint (#1828)

* feat: add golangci-lint linting

* chore: fix linter issues

* feat: add linting into the workflow

* docs: update lint docs

* fix: cr suggestions

* fix: remove old formatting and vetting scripts

* fix: add docker make target

* fix: action go caching

* fix: depreciated actions checkout version

* fix: cr suggestion

* fix: cr suggestions

---------

Co-authored-by: Manu Gupta <[email protected]>

Author: nrwiersma

.github/workflows/ci.yml

@@ -3,6 +3,18 @@ on:
   - push
   - pull_request
 jobs:
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Set up Go
+        uses: actions/setup-go@v3
+        with:
+          go-version-file: 'go.mod'
+          cache: true
+      - name: Lint code
+        run: make lint-docker
+
   build:
     env:
       ATHENS_MONGO_STORAGE_URL: mongodb://localhost:27017
@@ -53,7 +65,7 @@ jobs:
         with:
           go-version-file: 'go.mod'
           cache: true
-      - name: Run linter
+      - name: Verify changes
         run: make verify
       - name: Unit tests
         run: go test -v -race ./...

.golangci.yml

@@ -0,0 +1,80 @@
+run:
+  tests: false
+  timeout: 5m
+
+linters-settings:
+  cyclop:
+    max-complexity: 12
+    skip-tests: true
+  gofumpt:
+    extra-rules: true
+
+linters:
+  enable-all: true
+  disable:
+    - interfacer # deprecated
+    - scopelint # deprecated
+    - maligned # deprecated
+    - golint # deprecated
+    - structcheck # deprecated
+    - deadcode # deprecated
+    - varcheck # deprecated
+    - nosnakecase # deprecated
+    - ifshort # deprecated
+    - errchkjson
+    - exhaustive
+    - exhaustivestruct
+    - exhaustruct
+    - forcetypeassert
+    - funlen
+    - gochecknoglobals
+    - gochecknoinits
+    - goconst
+    - godox
+    - goerr113
+    - gomnd
+    - ireturn
+    - lll
+    - musttag
+    - nilnil
+    - nlreturn
+    - nonamedreturns
+    - tagliatelle
+    - varnamelen
+    - wrapcheck
+    - wsl
+    - cyclop # TODO: turn this back on later
+    - gocognit # TODO: turn this back on later
+    - forbidigo # TODO: turn this back on later
+
+issues:
+  exclude-use-default: false
+  exclude:
+    - 'package-comments: should have a package comment'
+    - 'ST1000: at least one file in a package should have a package comment'
+    - 'G204: Subprocess launched with a potential tainted input or cmd arguments'
+    - 'G204: Subprocess launched with variable'
+    - 'G402: TLS MinVersion too low.'
+    - 'const `op` is unused'
+  exclude-rules:
+    - path: cmd/proxy/main.go
+      text: 'G108: Profiling endpoint is automatically exposed on /debug/pprof'
+    - path: pkg/stash/stasher.go
+      linters:
+        - contextcheck
+    - path: pkg/stash/with_azureblob.go # False positive
+      linters:
+        - bodyclose
+    - path: pkg/storage/azureblob/azureblob.go # False positive
+      linters:
+        - bodyclose
+    - path: pkg/storage/compliance/*
+      linters:
+        - thelper
+        - gosec
+        - errcheck
+    - path: pkg/index/compliance/*
+      linters:
+        - thelper
+        - gosec
+        - errcheck

DEVELOPMENT.md

@@ -234,10 +234,10 @@ Then open [http://localhost:1313](http://localhost:1313/).
 
 # Linting
 
-In our CI/CD pass, we use govet, so feel free to run it locally beforehand:
+In our CI/CD pass, we use golangci-lint, so feel free to run it locally beforehand:
 
 ```
-go vet ./...
+make lint
 ```
 
 # For People Doing a Release

Makefile

@@ -1,6 +1,8 @@
 VERSION = "unset"
 DATE=$(shell date -u +%Y-%m-%d-%H:%M:%S-%Z)
 
+GOLANGCI_LINT_VERSION=v1.51.2
+
 ifndef GOLANG_VERSION
 override GOLANG_VERSION = 1.19
 endif
@@ -43,10 +45,16 @@ docs: ## build the docs docker image
 setup-dev-env:
 	$(MAKE) dev
 
+.PHONY: lint
+lint:
+	@golangci-lint run ./...
+
+.PHONY: lint-docker
+lint-docker:
+	@docker run -t --rm -v $(CURDIR):/app -w /app golangci/golangci-lint:$(GOLANGCI_LINT_VERSION) golangci-lint run ./...
+
 .PHONY: verify
 verify: ## verify athens codebase
-	./scripts/check_gofmt.sh
-	./scripts/check_govet.sh
 	./scripts/check_deps.sh
 	./scripts/check_conflicts.sh
 

cmd/proxy/actions/app.go

@@ -16,7 +16,7 @@ import (
 	"go.opencensus.io/plugin/ochttp"
 )
 
-// Service is the name of the service that we want to tag our processes with
+// Service is the name of the service that we want to tag our processes with.
 const Service = "proxy"
 
 // App is where all routes and middleware for the proxy
@@ -126,7 +126,7 @@ func App(conf *config.Config) (http.Handler, error) {
 
 	store, err := GetStorage(conf.StorageType, conf.Storage, conf.TimeoutDuration(), client)
 	if err != nil {
-		err = fmt.Errorf("error getting storage configuration (%s)", err)
+		err = fmt.Errorf("error getting storage configuration: %w", err)
 		return nil, err
 	}
 
@@ -140,7 +140,7 @@ func App(conf *config.Config) (http.Handler, error) {
 		lggr,
 		conf,
 	); err != nil {
-		err = fmt.Errorf("error adding proxy routes (%s)", err)
+		err = fmt.Errorf("error adding proxy routes: %w", err)
 		return nil, err
 	}
 

cmd/proxy/actions/app_proxy.go

@@ -2,6 +2,7 @@ package actions
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -54,7 +55,7 @@ func addProxyRoutes(
 		}
 		supportPath := path.Join("/sumdb", sumdbURL.Host, "/supported")
 		r.HandleFunc(supportPath, func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(200)
+			w.WriteHeader(http.StatusOK)
 		})
 		sumHandler := sumdbProxy(sumdbURL, c.NoSumPatterns)
 		pathPrefix := "/sumdb/" + sumdbURL.Host
@@ -63,7 +64,7 @@ func addProxyRoutes(
 		)
 	}
 
-	// Download Protocol
+	// Download Protocol:
 	// the download.Protocol and the stash.Stasher interfaces are composable
 	// in a middleware fashion. Therefore you can separate concerns
 	// by the functionality: a download.Protocol that just takes care
@@ -142,13 +143,13 @@ func getSingleFlight(l *log.Logger, c *config.Config, checker storage.Checker) (
 		return stash.WithSingleflight, nil
 	case "etcd":
 		if c.SingleFlight == nil || c.SingleFlight.Etcd == nil {
-			return nil, fmt.Errorf("Etcd config must be present")
+			return nil, errors.New("etcd config must be present")
 		}
 		endpoints := strings.Split(c.SingleFlight.Etcd.Endpoints, ",")
 		return stash.WithEtcd(endpoints, checker)
 	case "redis":
 		if c.SingleFlight == nil || c.SingleFlight.Redis == nil {
-			return nil, fmt.Errorf("Redis config must be present")
+			return nil, errors.New("redis config must be present")
 		}
 		return stash.WithRedisLock(
 			&athensLoggerForRedis{logger: l},
@@ -158,7 +159,7 @@ func getSingleFlight(l *log.Logger, c *config.Config, checker storage.Checker) (
 			c.SingleFlight.Redis.LockConfig)
 	case "redis-sentinel":
 		if c.SingleFlight == nil || c.SingleFlight.RedisSentinel == nil {
-			return nil, fmt.Errorf("Redis config must be present")
+			return nil, errors.New("redis config must be present")
 		}
 		return stash.WithRedisSentinelLock(
 			&athensLoggerForRedis{logger: l},

cmd/proxy/actions/auth.go

@@ -19,7 +19,7 @@ func initializeAuthFile(path string) {
 		return
 	}
 
-	fileBts, err := os.ReadFile(path)
+	fileBts, err := os.ReadFile(filepath.Clean(path))
 	if err != nil {
 		log.Fatalf("could not read %s: %v", path, err)
 	}
@@ -31,7 +31,7 @@ func initializeAuthFile(path string) {
 
 	fileName := transformAuthFileName(filepath.Base(path))
 	rcp := filepath.Join(hdir, fileName)
-	if err := os.WriteFile(rcp, fileBts, 0600); err != nil {
+	if err := os.WriteFile(rcp, fileBts, 0o600); err != nil {
 		log.Fatalf("could not write to file: %v", err)
 	}
 }
@@ -45,7 +45,7 @@ func netrcFromToken(tok string) {
 		log.Fatalf("netrcFromToken: could not get homedir: %v", err)
 	}
 	rcp := filepath.Join(hdir, getNETRCFilename())
-	if err := os.WriteFile(rcp, []byte(fileContent), 0600); err != nil {
+	if err := os.WriteFile(rcp, []byte(fileContent), 0o600); err != nil {
 		log.Fatalf("netrcFromToken: could not write to file: %v", err)
 	}
 }

cmd/proxy/actions/basicauth.go

@@ -8,10 +8,8 @@ import (
 	"github.com/gorilla/mux"
 )
 
-var (
-	// basicAuthExcludedPaths is a regular expression that matches paths that should not be protected by HTTP basic authentication.
-	basicAuthExcludedPaths = regexp.MustCompile("^/(health|ready)z$")
-)
+// basicAuthExcludedPaths is a regular expression that matches paths that should not be protected by HTTP basic authentication.
+var basicAuthExcludedPaths = regexp.MustCompile("^/(health|ready)z$")
 
 func basicAuth(user, pass string) mux.MiddlewareFunc {
 	return func(h http.Handler) http.Handler {
@@ -40,9 +38,5 @@ func checkAuth(r *http.Request, user, pass string) bool {
 	}
 
 	isPass := subtle.ConstantTimeCompare([]byte(pass), []byte(givenPass))
-	if isPass != 1 {
-		return false
-	}
-
-	return true
+	return isPass == 1
 }

cmd/proxy/actions/catalog.go

@@ -18,7 +18,7 @@ type catalogRes struct {
 	NextPageToken   string                `json:"next,omitempty"`
 }
 
-// catalogHandler implements GET baseURL/catalog
+// catalogHandler implements GET baseURL/catalog.
 func catalogHandler(s storage.Backend) http.HandlerFunc {
 	const op errors.Op = "actions.CatalogHandler"
 	cs, isCataloger := s.(storage.Cataloger)
@@ -54,7 +54,7 @@ func catalogHandler(s storage.Backend) http.HandlerFunc {
 }
 
 // getLimitFromParam converts a URL query parameter into an int
-// otherwise converts defaultPageSize constant
+// otherwise converts defaultPageSize constant.
 func getLimitFromParam(param string) (int, error) {
 	if param == "" {
 		return defaultPageSize, nil

cmd/proxy/actions/home.go

@@ -5,5 +5,5 @@ import (
 )
 
 func proxyHomeHandler(w http.ResponseWriter, r *http.Request) {
-	w.Write([]byte(`"Welcome to The Athens Proxy"`))
+	_, _ = w.Write([]byte(`"Welcome to The Athens Proxy"`))
 }

cmd/proxy/actions/index.go

@@ -13,7 +13,7 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// indexHandler implements GET baseURL/index
+// indexHandler implements GET baseURL/index.
 func indexHandler(index index.Indexer) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		ctx := r.Context()

cmd/proxy/actions/robots.go

@@ -6,7 +6,7 @@ import (
 	"github.com/gomods/athens/pkg/config"
 )
 
-// robotsHandler implements GET baseURL/robots.txt
+// robotsHandler implements GET baseURL/robots.txt.
 func robotsHandler(c *config.Config) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		http.ServeFile(w, r, c.RobotsFile)

cmd/proxy/actions/storage.go

@@ -21,7 +21,7 @@ import (
 	"github.com/spf13/afero"
 )
 
-// GetStorage returns storage backend based on env configuration
+// GetStorage returns storage backend based on env configuration.
 func GetStorage(storageType string, storageConfig *config.Storage, timeout time.Duration, client *http.Client) (storage.Backend, error) {
 	const op errors.Op = "actions.GetStorage"
 	switch storageType {

cmd/proxy/actions/sumdb.go

@@ -17,12 +17,12 @@ func sumdbProxy(url *url.URL, nosumPatterns []string) http.Handler {
 		req.URL.Host = url.Host
 	}
 	if len(nosumPatterns) > 0 {
-		return noSumWrapper(rp, url.Host, nosumPatterns)
+		return noSumWrapper(rp, nosumPatterns)
 	}
 	return rp
 }
 
-func noSumWrapper(h http.Handler, host string, patterns []string) http.Handler {
+func noSumWrapper(h http.Handler, patterns []string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if strings.HasPrefix(r.URL.Path, "/lookup/") {
 			for _, p := range patterns {

cmd/proxy/actions/sumdb_test.go

@@ -97,7 +97,7 @@ func TestNoSumPatterns(t *testing.T) {
 	for _, tc := range noSumTestCases {
 		t.Run(tc.name, func(t *testing.T) {
 			w := httptest.NewRecorder()
-			skipHandler := noSumWrapper(http.HandlerFunc(emptyHandler), "sum.golang.org", tc.patterns)
+			skipHandler := noSumWrapper(http.HandlerFunc(emptyHandler), tc.patterns)
 			req := httptest.NewRequest("GET", "/lookup/"+tc.given, nil)
 			skipHandler.ServeHTTP(w, req)
 			if tc.status != w.Code {

cmd/proxy/actions/version.go

@@ -8,5 +8,5 @@ import (
 )
 
 func versionHandler(w http.ResponseWriter, r *http.Request) {
-	json.NewEncoder(w).Encode(build.Data())
+	_ = json.NewEncoder(w).Encode(build.Data())
 }