fix #1283: update and enable error reports from golangci (#1523)

.github/workflows/ci_golangci-lint.yml

@@ -19,14 +19,22 @@ jobs:
     name: lint
     runs-on: ubuntu-latest
     steps:
+      - uses: actions/setup-go@v3
       - uses: actions/checkout@v3
       - name: golangci-lint
-        uses: golangci/golangci-lint-action@v2
+        uses: golangci/golangci-lint-action@v3
         with:
-          # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
-          version: v1.45.2
+          # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
+          version: v1.45
           # Optional: golangci-lint command line arguments.
-          args: --issues-exit-code=0 --timeout 5m
-          only-new-issues: true
-
+          args: --issues-exit-code=1 --timeout 5m
+          # Optional: show only new issues if it's a pull request. The default value is `false`.
+          only-new-issues: false
+          # Optional: if set to true then the all caching functionality will be complete disabled,
+          #           takes precedence over all other caching options.
+          skip-cache: false
+          # Optional: if set to true then the action don't cache or restore ~/go/pkg.
+          skip-pkg-cache: false
+          # Optional: if set to true then the action don't cache or restore ~/.cache/go-build.
+          skip-build-cache: false
 

.golangci.yml

@@ -0,0 +1,247 @@
+# see https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml
+
+run:
+  skip-dirs:
+    - pkg/time/rate
+  skip-files:
+    - pkg/database/ent/generate.go
+
+linters-settings:
+  gocyclo:
+    min-complexity: 30
+
+  funlen:
+    # Checks the number of lines in a function.
+    # If lower than 0, disable the check.
+    # Default: 60
+    lines: -1
+    # Checks the number of statements in a function.
+    # If lower than 0, disable the check.
+    # Default: 40
+    statements: -1
+
+  govet:
+    check-shadowing: true
+  lll:
+    line-length: 140
+  misspell:
+    locale: US
+  nolintlint:
+    allow-leading-space: true # don't require machine-readable nolint directives (i.e. with no leading space)
+    allow-unused: false # report any unused nolint directives
+    require-explanation: false # don't require an explanation for nolint directives
+    require-specific: false # don't require nolint directives to be specific about which linter is being skipped
+
+linters:
+  enable-all: true
+  disable:
+    #
+    # DEPRECATED by golangi-lint
+    #
+    - golint          # [deprecated]: Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes
+    - interfacer      # [deprecated]: Linter that suggests narrower interface types
+    - maligned        # [deprecated]: Tool to detect Go structs that would take less memory if their fields were sorted
+    - scopelint       # [deprecated]: Scopelint checks for unpinned variables in go programs
+
+    #
+    # Enabled
+    #
+    # - asciicheck          # Simple linter to check that your code does not contain non-ASCII identifiers
+    # - bidichk             # Checks for dangerous unicode character sequences
+    # - decorder            # check declaration order and count of types, constants, variables and functions
+    # - depguard            # Go linter that checks if package imports are in a list of acceptable packages
+    # - durationcheck       # check for two durations multiplied together
+    # - exportloopref       # checks for pointers to enclosing loop variables
+    # - funlen              # Tool for detection of long functions
+    # - gochecknoinits      # Checks that no init functions are present in Go code
+    # - goheader            # Checks is file header matches to pattern
+    # - gomoddirectives     # Manage the use of 'replace', 'retract', and 'excludes' directives in go.mod.
+    # - gomodguard          # Allow and block list linter for direct Go module dependencies. This is different from depguard where there are different block types for example version constraints and module recommendations.
+    # - goprintffuncname    # Checks that printf-like functions are named with `f` at the end
+    # - grouper             # An analyzer to analyze expression groups.
+    # - importas            # Enforces consistent import aliases
+    # - makezero            # Finds slice declarations with non-zero initial length
+    # - nolintlint          # Reports ill-formed or insufficient nolint directives
+    # - rowserrcheck        # checks whether Err of rows is checked successfully
+    # - sqlclosecheck       # Checks that sql.Rows and sql.Stmt are closed.
+    # - tenv                # tenv is analyzer that detects using os.Setenv instead of t.Setenv since Go1.17
+    # - tparallel           # tparallel detects inappropriate usage of t.Parallel() method in your Go test codes
+    # - typecheck           # Like the front-end of a Go compiler, parses and type-checks Go code
+    # - varcheck            # Finds unused global variables and constants
+
+    #
+    # Enabled by default by golangci (but requires fixing current issues, see at the end of this file) There
+    # is some redundancy, but never 1 to 1 (staticcheck seems to find more
+    # cases than ineffassign, deadcore more than unused..).
+    #
+    # - deadcode              # Finds unused code
+    # - errcheck              # Errcheck is a program for checking for unchecked errors in go programs. These unchecked errors can be critical bugs in some cases
+    # - gosimple              # (megacheck): Linter for Go source code that specializes in simplifying a code
+    # - govet                 # (vet, vetshadow): Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string
+    # - ineffassign           # Detects when assignments to existing variables are not used
+    # - staticcheck           # (megacheck): Staticcheck is a go vet on steroids, applying a ton of static analysis checks
+    # - structcheck           # Finds unused struct fields
+    # - unused                # (megacheck): Checks Go code for unused constants, variables, functions and types
+
+    #
+    # Recommended? (easy)
+    #
+    - errchkjson            # Checks types passed to the json encoding functions. Reports unsupported types and optionally reports occations, where the check for the returned error can be omitted.
+    - errorlint             # errorlint is a linter for that can be used to find code that will cause problems with the error wrapping scheme introduced in Go 1.13.
+    - forcetypeassert       # finds forced type assertions
+    - gci                   # Gci control golang package import order and make it always deterministic.
+    - gofmt                 # Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification
+    - goimports             # In addition to fixing imports, goimports also formats your code in the same style as gofmt.
+    - gosec                 # (gas): Inspects source code for security problems
+    - misspell              # Finds commonly misspelled English words in comments
+    - nakedret              # Finds naked returns in functions greater than a specified function length
+    - nilerr                # Finds the code that returns nil even if it checks that the error is not nil.
+    - predeclared           # find code that shadows one of Go's predeclared identifiers
+    - promlinter            # Check Prometheus metrics naming via promlint
+    - revive                # Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint.
+    - unconvert             # Remove unnecessary type conversions
+    - wastedassign          # wastedassign finds wasted assignment statements.
+    - gocritic              # Provides diagnostics that check for bugs, performance and style issues.
+    - exhaustive            # check exhaustiveness of enum switch statements
+    - thelper               # thelper detects golang test helpers without t.Helper() call and checks the consistency of test helpers
+    - dogsled               # Checks assignments with too many blank identifiers (e.g. x, _, _, _, := f())
+    - wrapcheck             # Checks that errors returned from external packages are wrapped
+    - lll                   # Reports long lines
+    - ifshort               # Checks that your code uses short syntax for if-statements whenever possible
+    - godot                 # Check if comments end in a period
+
+    #
+    # Recommended? (requires some work)
+    #
+
+    - bodyclose             # checks whether HTTP response body is closed successfully
+    - containedctx          # containedctx is a linter that detects struct contained context.Context field
+    - contextcheck          # check the function whether use a non-inherited context
+    - nilnil                # Checks that there is no simultaneous return of `nil` error and an invalid value.
+    - noctx                 # noctx finds sending http request without context.Context
+    - unparam               # Reports unused function parameters
+    - errname               # Checks that sentinel errors are prefixed with the `Err` and error types are suffixed with the `Error`.
+    - gomnd                 # An analyzer to detect magic numbers.
+    - ireturn               # Accept Interfaces, Return Concrete Types
+
+    #
+    # Formatting only, useful in IDE but should not be forced on CI?
+    #
+
+    - gofumpt               # Gofumpt checks whether code was gofumpt-ed.
+    - nlreturn              # nlreturn checks for a new line before return and branch statements to increase code clarity
+    - whitespace            # Tool for detection of leading and trailing whitespace
+    - wsl                   # Whitespace Linter - Forces you to use empty lines!
+
+    #
+    # Well intended, but not ready for this
+    #
+    - paralleltest          # paralleltest detects missing usage of t.Parallel() method in your Go test
+    - cyclop                # checks function and package cyclomatic complexity
+    - gocognit              # Computes and checks the cognitive complexity of functions
+    - maintidx              # maintidx measures the maintainability index of each function.
+    - goerr113              # Golang linter to check the errors handling expressions
+    - nestif                # Reports deeply nested if statements
+    - gocyclo               # Computes and checks the cyclomatic complexity of functions
+    - godox                 # Tool for detection of FIXME, TODO and other comment keywords
+    - dupl                  # Tool for code clone detection
+    - testpackage           # linter that makes you use a separate _test package
+
+    #
+    # Too strict (for now?)
+    #
+    - forbidigo             # Forbids identifiers
+    - tagliatelle           # Checks the struct tags.
+    - varnamelen            # checks that the length of a variable's name matches its scope
+    - gochecknoglobals      # check that no global variables exist
+    - exhaustivestruct      # Checks if all struct's fields are initialized
+    - goconst               # Finds repeated strings that could be replaced by a constant
+    - stylecheck            # Stylecheck is a replacement for golint
+
+    #
+    # Under evaluation
+    #
+
+    - prealloc              # Finds slice declarations that could potentially be preallocated
+
+
+issues:
+  exclude-rules:
+    - path: go.mod
+      text: "replacement are not allowed: golang.org/x/time/rate"
+
+    # `err` is often shadowed, we may continue to do it
+    - linters:
+        - govet
+      text: "shadow: declaration of \"err\" shadows declaration"
+
+    #
+    # govet
+    #
+
+    - linters:
+        - govet
+      text: "shadow: declaration of .* shadows declaration"
+    - linters:
+        - govet
+      text: "copylocks: assignment copies lock value to newStream:"
+    - linters:
+        - govet
+      text: "composites: .* composite literal uses unkeyed fields"
+
+    #
+    # errcheck
+    #
+
+    - linters:
+        - errcheck
+      text: "Error return value of `.*` is not checked"
+
+    #
+    # staticcheck
+    #
+
+    - linters:
+        - staticcheck
+      text: "SA4009: argument .* is overwritten before first use"
+    - linters:
+        - staticcheck
+      text: "SA4009.related information.: assignment to .*"
+    - linters:
+        - staticcheck
+      text: "SA4006: this value of .* is never used"
+    - linters:
+        - staticcheck
+      text: "SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead"
+
+    #
+    # gosimple
+    #
+
+    - linters:
+        - gosimple
+      text: "S1023: redundant .* statement"
+    - linters:
+        - gosimple
+      text: "S1000: should use a simple channel send/receive instead of `select` with a single case"
+    - linters:
+        - gosimple
+      text: "S1028: should use .* instead of .*"
+
+    #
+    # deadcode
+    #
+
+    - linters:
+        - deadcode
+        - unused
+        - structcheck
+      text: ".* is unused"
+
+    #
+    # ineffassign
+    #
+
+    - linters:
+        - ineffassign
+      text: "ineffectual assignment to .*"

cmd/crowdsec-cli/hubtest.go

@@ -533,7 +533,7 @@ cscli hubtest create my-scenario-test --parsers crowdsecurity/nginx --scenarios
 				if err != nil {
 					log.Fatalf(err.Error())
 				}
-				fmt.Printf(output)
+				fmt.Print(output)
 			}
 		},
 	}

cmd/crowdsec/serve.go

@@ -18,7 +18,7 @@ import (
 	//"github.com/sevlyar/go-daemon"
 )
 
-//debugHandler is kept as a dev convenience : it shuts down and serialize internal state
+// debugHandler is kept as a dev convenience : it shuts down and serialize internal state
 func debugHandler(sig os.Signal, cConfig *csconfig.Config) error {
 	var tmpFile string
 	var err error

pkg/acquisition/acquisition_test.go

@@ -371,10 +371,8 @@ func (f *MockTail) StreamingAcquisition(out chan types.Event, t *tomb.Tomb) erro
 		evt.Line.Src = "test"
 		out <- evt
 	}
-	select {
-	case <-t.Dying():
-		return nil
-	}
+	<-t.Dying()
+	return nil
 }
 func (f *MockTail) CanRun() error                            { return nil }
 func (f *MockTail) GetMetrics() []prometheus.Collector       { return nil }

pkg/apiclient/alerts_service_test.go

@@ -399,7 +399,7 @@ func TestAlertsGetAsMachine(t *testing.T) {
 	}
 
 	//fail
-	alerts, resp, err = client.Alerts.GetByID(context.Background(), 2)
+	_, resp, err = client.Alerts.GetByID(context.Background(), 2)
 	assert.Contains(t, fmt.Sprintf("%s", err), "API error: object not found")
 
 }

pkg/apiserver/apiserver.go

@@ -190,7 +190,6 @@ func NewServer(config *csconfig.LocalApiServerCfg) (*APIServer, error) {
 
 	router.NoRoute(func(c *gin.Context) {
 		c.JSON(http.StatusNotFound, gin.H{"message": "Page or Method not found"})
-		return
 	})
 	router.Use(CustomRecoveryWithWriter())
 

pkg/apiserver/controllers/v1/decisions.go

@@ -82,7 +82,6 @@ func (c *Controller) DeleteDecisionById(gctx *gin.Context) {
 	}
 
 	gctx.JSON(http.StatusOK, deleteDecisionResp)
-	return
 }
 
 func (c *Controller) DeleteDecisions(gctx *gin.Context) {
@@ -98,7 +97,6 @@ func (c *Controller) DeleteDecisions(gctx *gin.Context) {
 	}
 
 	gctx.JSON(http.StatusOK, deleteDecisionResp)
-	return
 }
 
 func (c *Controller) StreamDecision(gctx *gin.Context) {
@@ -210,5 +208,4 @@ func (c *Controller) StreamDecision(gctx *gin.Context) {
 	}
 
 	gctx.JSON(http.StatusOK, ret)
-	return
 }

pkg/apiserver/controllers/v1/machines.go

@@ -27,5 +27,4 @@ func (c *Controller) CreateMachine(gctx *gin.Context) {
 	}
 
 	gctx.Status(http.StatusCreated)
-	return
 }

pkg/apiserver/jwt_test.go

@@ -46,7 +46,7 @@ func TestLogin(t *testing.T) {
 	router.ServeHTTP(w, req)
 
 	assert.Equal(t, 401, w.Code)
-	assert.Equal(t, "{\"code\":401,\"message\":\"missing : invalid character 'e' in literal true (expecting 'r')\"}", w.Body.String())
+	assert.Equal(t, "{\"code\":401,\"message\":\"missing: invalid character 'e' in literal true (expecting 'r')\"}", w.Body.String())
 
 	// Login with invalid format
 	w = httptest.NewRecorder()

pkg/apiserver/middlewares/v1/jwt.go

@@ -7,14 +7,13 @@ import (
 	"strings"
 	"time"
 
-	"errors"
-
 	jwt "github.com/appleboy/gin-jwt/v2"
 	"github.com/crowdsecurity/crowdsec/pkg/database"
 	"github.com/crowdsecurity/crowdsec/pkg/database/ent/machine"
 	"github.com/crowdsecurity/crowdsec/pkg/models"
 	"github.com/gin-gonic/gin"
 	"github.com/go-openapi/strfmt"
+	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/crypto/bcrypt"
 )
@@ -48,7 +47,7 @@ func (j *JWT) Authenticator(c *gin.Context) (interface{}, error) {
 	var scenarios string
 	var err error
 	if err := c.ShouldBindJSON(&loginInput); err != nil {
-		return "", errors.New(fmt.Sprintf("missing : %v", err.Error()))
+		return "", errors.Wrap(err, "missing")
 	}
 	if err := loginInput.Validate(strfmt.Default); err != nil {
 		return "", errors.New("input format error")

pkg/metabase/database.go

@@ -89,7 +89,7 @@ func (d *Database) Update() error {
 		return errors.Wrap(err, "update sqlite db response (unmarshal)")
 	}
 	model.Details = d.Details
-	success, errormsg, err = d.Client.Do("PUT", routes[databaseEndpoint], model)
+	_, errormsg, err = d.Client.Do("PUT", routes[databaseEndpoint], model)
 	if err != nil {
 		return err
 	}