Upgrade and fix golangci-lint errors

This also makes the repo use github actions for linting
signalfx · May 24, 2022 · 6189725 · 6189725
1 parent 1de9e2a
commit 6189725
Show file tree

Hide file tree

Showing 59 changed files with 464 additions and 380 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -7,5 +7,5 @@ jobs:
     steps:
       - checkout
       - run:
-          name: Verify
-          command: make verify
+          name: Test
+          command: make test
diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml
@@ -0,0 +1,33 @@
+name: golangci-lint
+on:
+  push:
+    tags:
+      - v*
+    branches:
+      - master
+      - main
+  pull_request:
+jobs:
+  golangci:
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v2
+        with:
+          version: v1.46.1
+          # Optional: golangci-lint command line arguments.
+          args: '--config=.golangci.yml -v'
+
+          # Optional: show only new issues if it's a pull request. The default value is `false`.
+          # only-new-issues: true
+
+          # Optional: if set to true then the action will use pre-installed Go.
+          # skip-go-installation: true
+
+          # Optional: if set to true then the action don't cache or restore ~/go/pkg.
+          skip-pkg-cache: true
+
+          # Optional: if set to true then the action don't cache or restore ~/.cache/go-build.
+          skip-build-cache: true
diff --git a/.golangci.yml b/.golangci.yml
@@ -2,19 +2,19 @@ run:
   timeout: 5m
   tests: true
   skip-dirs:
-    - cmd/backfill_test
-    - lib/sfxthrift/datamodelservice
     - src/external_libs
+    - gocat
     - genfiles$
     - vendor$
+    - ketama
   # which files to skip: they will be analyzed, but issues from them
   # won't be reported. Default value is empty list, but there is
   # no need to include all autogenerated files, we confidently recognize
   # autogenerated files. If it's not please let us know.
   skip-files:
     - ".*\\.pb\\.go"
     - ".*\\.gen\\.go"
-    - ".*\\.java"
+    - ".*_easyjson\\.go"
   # by default isn't set. If set we pass it to "go list -mod={option}". From "go help modules":
   # If invoked with -mod=readonly, the go command is disallowed from the implicit
   # automatic updating of go.mod described above. Instead, it fails when any changes
@@ -41,6 +41,8 @@ linters-settings:
     min-confidence: 0.3
   gocyclo:
     min-complexity: 10
+  gocognit:
+    min-complexity: 30
   maligned:
     suggest-new: true
   gofmt:
@@ -50,10 +52,10 @@ linters-settings:
   settings:
     printf: # analyzer name, run `go tool vet help` to see all analyzers
       funcs: # run `go tool vet help printf` to see available settings for `printf` analyzer
-      - (github.com/golangci/golangci-lint/pkg/logutils.Log).Infof
-      - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf
-      - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf
-      - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf
+        - (github.com/golangci/golangci-lint/pkg/logutils.Log).Infof
+        - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf
+        - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf
+        - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf
   enabled-tags:
     - performance
   gocritic:
@@ -88,25 +90,118 @@ linters-settings:
     # report any comments starting with keywords, this is useful for TODO or FIXME comments that
     # might be left in the code accidentally and should be resolved before merging
     keywords: # default keywords are TODO, BUG, and FIXME, these can be overwritten by this setting
-    - OPTIMIZE # marks code that should be optimized before merging
-    - HACK # marks hack-arounds that should be removed before merging
+      - OPTIMIZE # marks code that should be optimized before merging
+      - HACK # marks hack-arounds that should be removed before merging
   funlen:
     lines: 500 # TODO: need to set this to 150 statements and work on it
     statements: 150
+  ifshort:
+    # Maximum length of variable declaration measured in number of lines, after which linter won't suggest using short syntax.
+    # Has higher priority than max-decl-chars.
+    max-decl-lines: 1
+    # Maximum length of variable declaration measured in number of characters, after which linter won't suggest using short syntax.
+    max-decl-chars: 30
+  cyclop:
+    # the maximal code complexity to report
+    max-complexity: 15
+    # should ignore tests (default false)
+    skip-tests: false
+  gosec:
+    # To select a subset of rules to run.
+    # Available rules: https://github.com/securego/gosec#available-rules
+    includes:
+      - G101 # Look for hard coded credentials
+      - G102 # Bind to all interfaces
+      - G106 # Audit the use of ssh.InsecureIgnoreHostKey
+      - G107 # Url provided to HTTP request as taint input
+      - G108 # Profiling endpoint automatically exposed on /debug/pprof
+      - G109 # Potential Integer overflow made by strconv.Atoi result conversion to int16/32
+      - G110 # Potential DoS vulnerability via decompression bomb
+      - G306 # Poor file permissions used when writing to a new file
+      - G401 # Detect the usage of DES, RC4, MD5 or SHA1
+      - G501 # Import blocklist: crypto/md5
+      - G502 # Import blocklist: crypto/des
+      - G503 # Import blocklist: crypto/rc4
+      - G504 # Import blocklist: net/http/cgi
+      - G505 # Import blocklist: crypto/sha1
+      - G601 # Implicit memory aliasing of items from a range statement
+    # To specify a set of rules to explicitly exclude.
+    # Available rules: https://github.com/securego/gosec#available-rules
+    excludes:
+      - G204 # Audit use of command execution
+    # To specify the configuration of rules.
+    # The configuration of rules is not fully documented by gosec:
+    # https://github.com/securego/gosec#configuration
+    # https://github.com/securego/gosec/blob/569328eade2ccbad4ce2d0f21ee158ab5356a5cf/rules/rulelist.go#L60-L102
+    config:
+      G306: "0600"
+      G101:
+        pattern: "(?i)pwd|password|private_key|secret"
+        ignore_entropy: false
+        entropy_threshold: "80.0"
+        per_char_threshold: "3.0"
+        truncate: "32"
+  staticcheck:
+    # Select the Go version to target. The default is '1.13'.
+    go: "1.17"
+    # https://staticcheck.io/docs/options#checks
+    checks:
+      - "all"
+      - "-SA1029"  # disable the check for using string in context value keys 
+      - "-SA1019"  # disable the check for net.Temporary as we use it still
+  dupl:
+    # tokens count to trigger issue, 150 by default
+    threshold: 100
 
 linters:
-  enable-all: true
-  disable:
-    - lll
-    - gochecknoglobals
-    - errcheck
-    - unparam
+  disable-all: true
+  enable: # please use alphabetical order when enabling any linters
+    - asciicheck
+    - bodyclose
+    - cyclop
+    - deadcode
+    - depguard
+    - durationcheck
+    - errorlint
+    - errname
+    - exportloopref
+    - forbidigo
+    - gci
+    - gocognit
+    - goconst
+    - gocyclo
+    - godox
+    - gofmt
+    - gofumpt
+    - goheader
+    - goimports
+    - gomodguard
+    - goprintffuncname
     - gosec
-    - interfacer
-    - dupl
-    # TODO: need to enable this two for better coding guidelines in terms of space between condition
+    - gosimple
+    - govet
+    - ifshort
+    - ineffassign
+    - makezero
+    - megacheck
+    - misspell
+    - nakedret
+    - nilerr
+    - noctx
+    - prealloc
+    - predeclared
+    - revive
+    - staticcheck
+    - structcheck
+    - stylecheck
+    - testpackage
+    - thelper
+    - tparallel
+    - unconvert
+    - unparam
+    - unused
+    - wastedassign
     - whitespace
-    - wsl
   fast: true
 
 # output configuration options

diff --git a/config/globbing/glob_test.go b/config/globbing/glob_test.go
@@ -7,7 +7,7 @@ import (
 )
 
 func TestEscapeMetaCharacters(t *testing.T) {
-	var cases = []struct {
+	cases := []struct {
 		desc    string
 		pattern string
 		match   []string

diff --git a/dp/dpbuffered/bufferedforwarder.go b/dp/dpbuffered/bufferedforwarder.go
@@ -1,13 +1,12 @@
 package dpbuffered
 
 import (
-	"sync"
-	"sync/atomic"
-
 	"context"
 	"fmt"
 	"net/http"
 	"runtime"
+	"sync"
+	"sync/atomic"
 
 	"github.com/signalfx/golib/v3/datapoint"
 	"github.com/signalfx/golib/v3/datapoint/dpsink"
@@ -103,9 +102,9 @@ func (forwarder *BufferedForwarder) DebugEndpoints() map[string]http.Handler {
 
 var _ dpsink.Sink = &BufferedForwarder{}
 
-type errDPBufferFull string
+type dpBufferFullError string
 
-func (e errDPBufferFull) Error() string {
+func (e dpBufferFullError) Error() string {
 	return "Forwarder " + string(e) + " unable to send more datapoints.  Buffer full"
 }
 
@@ -117,7 +116,7 @@ func (forwarder *BufferedForwarder) AddDatapoints(ctx context.Context, points []
 	atomic.AddInt64(&forwarder.stats.totalDatapointsBuffered, int64(len(points)))
 	if *forwarder.config.MaxTotalDatapoints <= atomic.LoadInt64(&forwarder.stats.totalDatapointsBuffered) {
 		atomic.AddInt64(&forwarder.stats.totalDatapointsBuffered, int64(-len(points)))
-		return errDPBufferFull(forwarder.identifier)
+		return dpBufferFullError(forwarder.identifier)
 	}
 	select {
 	case forwarder.dpChan <- points:
@@ -129,9 +128,9 @@ func (forwarder *BufferedForwarder) AddDatapoints(ctx context.Context, points []
 	}
 }
 
-type errEBufferFull string
+type eBufferFullError string
 
-func (e errEBufferFull) Error() string {
+func (e eBufferFullError) Error() string {
 	return "Forwarder " + string(e) + " unable to send more events.  Buffer full"
 }
 
@@ -143,7 +142,7 @@ func (forwarder *BufferedForwarder) AddEvents(ctx context.Context, events []*eve
 	atomic.AddInt64(&forwarder.stats.totalEventsBuffered, int64(len(events)))
 	if *forwarder.config.MaxTotalEvents <= atomic.LoadInt64(&forwarder.stats.totalEventsBuffered) {
 		atomic.AddInt64(&forwarder.stats.totalEventsBuffered, int64(-len(events)))
-		return errEBufferFull(forwarder.identifier)
+		return eBufferFullError(forwarder.identifier)
 	}
 	select {
 	case forwarder.eChan <- events:
@@ -155,9 +154,9 @@ func (forwarder *BufferedForwarder) AddEvents(ctx context.Context, events []*eve
 	}
 }
 
-type errTBufferFull string
+type tBufferFullError string
 
-func (e errTBufferFull) Error() string {
+func (e tBufferFullError) Error() string {
 	return "Forwarder " + string(e) + " unable to send more traces.  Buffer full"
 }
 
@@ -166,7 +165,7 @@ func (forwarder *BufferedForwarder) AddSpans(ctx context.Context, traces []*trac
 	atomic.AddInt64(&forwarder.stats.totalTracesBuffered, int64(len(traces)))
 	if *forwarder.config.MaxTotalSpans <= atomic.LoadInt64(&forwarder.stats.totalTracesBuffered) {
 		atomic.AddInt64(&forwarder.stats.totalTracesBuffered, int64(-len(traces)))
-		return errTBufferFull(forwarder.identifier)
+		return tBufferFullError(forwarder.identifier)
 	}
 	select {
 	case forwarder.tChan <- traces:

diff --git a/dp/dpbuffered/bufferedforwarder_test.go b/dp/dpbuffered/bufferedforwarder_test.go
@@ -1,15 +1,14 @@
 package dpbuffered
 
 import (
-	"testing"
-	"time"
-
 	"bytes"
 	"context"
+	"errors"
 	"io"
-	"sync"
-
 	"net/http"
+	"sync"
+	"testing"
+	"time"
 
 	"github.com/signalfx/golib/v3/datapoint"
 	"github.com/signalfx/golib/v3/datapoint/dpsink"
@@ -336,7 +335,7 @@ func TestBufferedForwarderMaxTotalDatapoints(t *testing.T) {
 	}
 	found := false
 	for i := 0; i < 100; i++ {
-		if err := bf.AddDatapoints(ctx, datas); err == errDPBufferFull(*config.Name) {
+		if err := bf.AddDatapoints(ctx, datas); errors.Is(err, dpBufferFullError(*config.Name)) {
 			assert.NotEmpty(t, err.Error())
 			found = true
 			break
@@ -369,7 +368,7 @@ func TestBufferedForwarderMaxTotalEvents(t *testing.T) {
 	spans := []*trace.Span{{}, {}}
 	found := false
 	for i := 0; i < 100; i++ {
-		if err := bf.AddEvents(ctx, events); err == errEBufferFull(*config.Name) {
+		if err := bf.AddEvents(ctx, events); errors.Is(err, eBufferFullError(*config.Name)) {
 			assert.NotEmpty(t, err.Error())
 			found = true
 			break
@@ -378,7 +377,7 @@ func TestBufferedForwarderMaxTotalEvents(t *testing.T) {
 	assert.True(t, found, "With small buffer size, I should error out with a full buffer")
 	found = false
 	for i := 0; i < 100; i++ {
-		if err := bf.AddSpans(ctx, spans); err == errTBufferFull(*config.Name) {
+		if err := bf.AddSpans(ctx, spans); errors.Is(err, tBufferFullError(*config.Name)) {
 			assert.NotEmpty(t, err.Error())
 			found = true
 			break

diff --git a/grpc/auth.go b/grpc/auth.go
@@ -10,7 +10,7 @@ import (
 // SignalFxTokenAuth is a credentials.PerRPCCredentials object that sets an auth token on each gRPC request
 // as expected by our ingest service.
 type SignalFxTokenAuth struct {
-	Token string
+	Token                    string
 	DisableTransportSecurity bool
 }
 

diff --git a/grpc/auth_test.go b/grpc/auth_test.go
@@ -11,7 +11,7 @@ import (
 func TestGRPCAuth(t *testing.T) {
 	Convey("grpc auth", t, func() {
 		a := &SignalFxTokenAuth{
-			Token: "test",
+			Token:                    "test",
 			DisableTransportSecurity: false,
 		}
 

diff --git a/protocol/carbon/carbon.go b/protocol/carbon/carbon.go
@@ -1,6 +1,7 @@
 package carbon
 
 import (
+	stderrors "errors"
 	"strconv"
 	"strings"
 	"time"
@@ -38,7 +39,7 @@ func NewCarbonDatapoint(line string, metricDeconstructor metricdeconstructor.Met
 	originalMetricName := parts[0]
 	metricName, mtype, dimensions, err := metricDeconstructor.Parse(originalMetricName)
 
-	if err == metricdeconstructor.ErrSkipMetric {
+	if stderrors.Is(err, metricdeconstructor.ErrSkipMetric) {
 		return nil, nil
 	}
 	if err != nil {