Merge pull request #231 from signalfx/enable_golang_linters

enabling linters that were suppose to be enabled with --enable-all flag

.circleci/config.yml

@@ -3,7 +3,8 @@ jobs:
   build:
     working_directory: ~/repo
     docker:
-      - image: circleci/golang:1.17.2
+      - image: circleci/golang:1.17.3
+    resource_class: xlarge # to avoid sfxclient timeouts lets use a bigger box
     steps:
       - checkout
       - restore_cache:
@@ -20,7 +21,7 @@ jobs:
           name: Run tests
           command: |
             set -x
-            go test -covermode atomic -race -timeout 120s -parallel 4 -coverprofile ./coverage.out $(go list ./...)
+            go test -covermode atomic -race -timeout 120s -coverprofile ./coverage.out $(go list ./...)
             cov="$(go tool cover -func coverage.out | grep -v 100.0% || true)"
             echo $cov
             test -z "$cov"

.github/workflows/golangci-lint.yml

@@ -16,7 +16,7 @@ jobs:
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v2
         with:
-          version: v1.41.1
+          version: v1.42.1
           # Optional: golangci-lint command line arguments.
           args: '--config=.golangci.yml -v'
 

.golangci.yml

@@ -1,9 +1,12 @@
 run:
-  timeout: 2m
+  timeout: 5m
   tests: true
   skip-dirs:
+    - 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
@@ -91,33 +94,131 @@ linters-settings:
   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" ]
+  dupl:
+    # tokens count to trigger issue, 150 by default
+    threshold: 100
 
 linters:
+  enable: # please use alphabetical order when enabling any linters
+    - asciicheck
+    - bodyclose
+    - cyclop
+    - deadcode
+    - depguard
+    - dupl
+    - durationcheck
+    - errorlint
+    - errname
+    - exportloopref
+    - forbidigo
+    - forcetypeassert
+    - gci
+    - gocognit
+    - goconst
+    - gocyclo
+    - godox
+    - gofmt
+    - gofumpt
+    - goheader
+    - goimports
+    - gomodguard
+    - goprintffuncname
+    - gosec
+    - gosimple
+    - govet
+    - ifshort
+    - ineffassign
+    - makezero
+    - megacheck
+    - misspell
+    - nakedret
+    - nilerr
+    - noctx
+    - prealloc
+    - predeclared
+    - revive
+    - staticcheck
+    - structcheck
+    - stylecheck
+    - testpackage
+    - thelper
+    - tparallel
+    - typecheck
+    - unconvert
+    - unparam
+    - unused
+    - wastedassign
+    - whitespace
   enable-all: true
   disable:
-    - ifshort
-    - forcetypeassert
-    # need to enable linters above this line
     - lll
-    - gochecknoinits
     - paralleltest # this has lots of test cases that needs to be added as parallel and some of them have race condition so disabling after some effort
     - gochecknoglobals
-    - errcheck
-    - unparam
-    - interfacer
-    - dupl
-    - exhaustive # not urgent this currently as its complaining mainly about switch statements in sharecache
+    - exhaustive
     # TODO: need to enable this two low priority for better coding guidelines in terms of space between condition
-    - whitespace
     - wsl
     - godot
-    - gomnd
-    - nestif
+    - gomnd # we cannot enable this as there are lots of magic numbers
+    - nestif # cannot enable this given the way the code structure is organized for unit testing
     - nlreturn
     - nolintlint
     - tagliatelle # cannot be enabled as it's asking to modify `json` which is not possible
     - promlinter # we do not care above prometheus linter for metrics
     - gomoddirectives # needs to be disabled as we need replacement for jaeger & redis
+    - scopelint # deprecated and hence disabled it
+    - interfacer # deprecated and hence disabled
+    - wrapcheck # Checks that errors returned from external packages are wrapped
   fast: true
 
 # output configuration options

datapoint/cast_test.go

@@ -159,6 +159,7 @@ func TestCastMetricValueWithBool(t *testing.T) {
 	}
 }
 
+//nolint:dupl
 func TestCastFloatValue(t *testing.T) {
 	type args struct {
 		value interface{}
@@ -192,6 +193,7 @@ func TestCastFloatValue(t *testing.T) {
 	}
 }
 
+//nolint:dupl
 func TestCastUIntegerValue(t *testing.T) {
 	type args struct {
 		value interface{}
@@ -225,6 +227,7 @@ func TestCastUIntegerValue(t *testing.T) {
 	}
 }
 
+//nolint:dupl
 func TestCastIntegerValue(t *testing.T) {
 	type args struct {
 		value interface{}

datapoint/dpsink/counter.go

@@ -2,6 +2,7 @@ package dpsink
 
 import (
 	"context"
+	goerrors "errors"
 	"sync/atomic"
 	"time"
 
@@ -67,6 +68,7 @@ func (c *Counter) logErrMsg(ctx context.Context, err error, msg string) {
 }
 
 // AddDatapoints will send points to the next sink and track points send to the next sink
+//nolint:dupl
 func (c *Counter) AddDatapoints(ctx context.Context, points []*datapoint.Datapoint, next Sink) error {
 	atomic.AddInt64(&c.TotalDatapoints, int64(len(points)))
 	atomic.AddInt64(&c.TotalProcessCalls, 1)
@@ -91,6 +93,7 @@ func (c *Counter) logger() log.Logger {
 }
 
 // AddEvents will send events to the next sink and track events sent to the next sink
+//nolint:dupl
 func (c *Counter) AddEvents(ctx context.Context, events []*event.Event, next Sink) error {
 	atomic.AddInt64(&c.TotalEvents, int64(len(events)))
 	atomic.AddInt64(&c.TotalProcessCalls, 1)
@@ -108,6 +111,7 @@ func (c *Counter) AddEvents(ctx context.Context, events []*event.Event, next Sin
 }
 
 // AddSpans will send spans to the next sink and track spans sent to the next sink
+//nolint:dupl
 func (c *Counter) AddSpans(ctx context.Context, spans []*trace.Span, next trace.Sink) error {
 	atomic.AddInt64(&c.TotalSpans, int64(len(spans)))
 	atomic.AddInt64(&c.TotalProcessCalls, 1)
@@ -118,7 +122,8 @@ func (c *Counter) AddSpans(ctx context.Context, spans []*trace.Span, next trace.
 	atomic.AddInt64(&c.CallsInFlight, -1)
 	if err != nil && spanfilter.IsInvalid(err) {
 		atomic.AddInt64(&c.TotalProcessErrors, 1)
-		if m, ok := err.(*spanfilter.Map); ok {
+		var m *spanfilter.Map
+		if goerrors.As(err, &m) {
 			atomic.AddInt64(&c.ProcessErrorSpans, int64(len(m.Invalid)))
 		} else {
 			atomic.AddInt64(&c.ProcessErrorSpans, int64(len(spans)))

datapoint/dptest/basicsink_test.go

@@ -87,6 +87,7 @@ func TestContextErrorTrace(t *testing.T) {
 	<-progressChan
 }
 
+//nolint:dupl
 func TestNext(t *testing.T) {
 	ctx := context.Background()
 	b := NewBasicSink()
@@ -105,6 +106,7 @@ func TestNext(t *testing.T) {
 	})
 }
 
+//nolint:dupl
 func TestNextEvent(t *testing.T) {
 	ctx := context.Background()
 	b := NewBasicSink()
@@ -123,6 +125,7 @@ func TestNextEvent(t *testing.T) {
 	})
 }
 
+//nolint:dupl
 func TestNextSpan(t *testing.T) {
 	ctx := context.Background()
 	b := NewBasicSink()

datapoint/dptest/generator.go

@@ -25,6 +25,7 @@ type DatapointSource struct {
 
 var globalSource DatapointSource
 
+//nolint:gochecknoinits
 func init() {
 	globalSource.Metric = "random"
 	globalSource.Dims = map[string]string{"source": "randtest"}