Merge pull request #10 from rdner/add-linter

Create linter configuration and address found issues.
Also added magefile targets so it's running on the CI.

Author: rdner

.ci/Jenkinsfile

@@ -37,12 +37,7 @@ pipeline {
         withGithubNotify(context: "Lint") {
           dir("${BASE_DIR}"){
             withMageEnv(){
-              sh(label: 'lint', script: '''
-                go mod tidy && git diff --exit-code
-                gofmt -l . | read && echo "Code differs from gofmt's style. Run 'gofmt -w .'" 1>&2 && exit 1 || true
-              ''')
               sh(label: 'Mage check', script: 'mage check')
-              sh(label: 'Go vet', script: 'go vet ./...')
             }
           }
         }

.golangci.yml

@@ -0,0 +1,143 @@
+# options for analysis running
+run:
+  # timeout for analysis, e.g. 30s, 5m, default is 1m
+  timeout: 1m
+
+output:
+  sort-results: true
+
+# Uncomment and add a path if needed to exclude
+# skip-dirs:
+#   - some/path
+# skip-files:
+#   - ".*\\.my\\.go$"
+#   - lib/bad.go
+
+# Find the whole list here https://golangci-lint.run/usage/linters/
+linters:
+  disable-all: true
+  enable:
+    - deadcode # finds unused code
+    - errcheck # checking for unchecked errors in go programs
+    - 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.
+    - goconst # finds repeated strings that could be replaced by a constant
+    - dupl # tool for code clone detection
+    - forbidigo # forbids identifiers	matched by reg exps
+    - gomoddirectives # manage the use of 'replace', 'retract', and 'excludes' directives in go.mod.
+    - gosimple # linter for Go source code that specializes in simplifying a code
+    - misspell # finds commonly misspelled English words in comments
+    - nakedret # finds naked returns in functions greater than a specified function length
+    - prealloc # finds slice declarations that could potentially be preallocated
+    - nolintlint # reports ill-formed or insufficient nolint directives
+    - staticcheck # Staticcheck is a go vet on steroids, applying a ton of static analysis checks
+    - stylecheck # a replacement for golint
+    - unparam # reports unused function parameters
+    - unused # checks Go code for unused constants, variables, functions and types
+
+    - govet # 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
+    - structcheck # finds unused struct fields
+    - typecheck # Like the front-end of a Go compiler, parses and type-checks Go code
+    - varcheck # Finds unused global variables and constants
+    - asciicheck # simple linter to check that your code does not contain non-ASCII identifiers
+    - bodyclose # checks whether HTTP response body is closed successfully
+    - durationcheck # check for two durations multiplied together
+    - exportloopref # checks for pointers to enclosing loop variables
+    - goimports # Goimports does everything that gofmt does. Additionally it checks unused imports
+    - gosec # inspects source code for security problems
+    - importas # enforces consistent import aliases
+    - nilerr # finds the code that returns nil even if it checks that the error is not nil.
+    - noctx # noctx finds sending http request without context.Context
+    - unconvert # Remove unnecessary type conversions
+    - wastedassign # wastedassign finds wasted assignment statements.
+    - godox # tool for detection of FIXME, TODO and other comment keywords
+
+# all available settings of specific linters
+linters-settings:
+  errcheck:
+    # report about not checking of errors in type assertions: `a := b.(MyStruct)`;
+    # default is false: such cases aren't reported by default.
+    check-type-assertions: true
+
+  errorlint:
+    # Check whether fmt.Errorf uses the %w verb for formatting errors. See the readme for caveats
+    errorf: true
+    # Check for plain type assertions and type switches
+    asserts: true
+    # Check for plain error comparisons
+    comparison: true
+
+  goconst:
+    # minimal length of string constant, 3 by default
+    min-len: 3
+    # minimal occurrences count to trigger, 3 by default
+    min-occurrences: 2
+
+  dupl:
+    # tokens count to trigger issue, 150 by default
+    threshold: 100
+
+  forbidigo:
+    # Forbid the following identifiers
+    forbid:
+      - fmt.Print.* # too much log noise
+    # Exclude godoc examples from forbidigo checks.  Default is true.
+    exclude_godoc_examples: true
+
+  gomoddirectives:
+    # Allow local `replace` directives. Default is false.
+    replace-local: false
+
+  gosimple:
+    # Select the Go version to target. The default is '1.13'.
+    go: "1.17"
+
+  misspell:
+    # Correct spellings using locale preferences for US or UK.
+    # Default is to use a neutral variety of English.
+    # Setting locale to US will correct the British spelling of 'colour' to 'color'.
+    # locale: US
+    # ignore-words:
+    #   - IdP
+
+  nakedret:
+    # make an issue if func has more lines of code than this setting and it has naked returns; default is 30
+    max-func-lines: 0
+
+  prealloc:
+    # Report preallocation suggestions only on simple loops that have no returns/breaks/continues/gotos in them.
+    # True by default.
+    simple: true
+    range-loops: true # Report preallocation suggestions on range loops, true by default
+    for-loops: false # Report preallocation suggestions on for loops, false by default
+
+  nolintlint:
+    # Enable to ensure that nolint directives are all used. Default is true.
+    allow-unused: false
+    # Disable to ensure that nolint directives don't have a leading space. Default is true.
+    allow-leading-space: true
+    # Exclude following linters from requiring an explanation.  Default is [].
+    allow-no-explanation: []
+    # Enable to require an explanation of nonzero length after each nolint directive. Default is false.
+    require-explanation: true
+    # Enable to require nolint directives to mention the specific linter being suppressed. Default is false.
+    require-specific: true
+
+  staticcheck:
+    # Select the Go version to target. The default is '1.13'.
+    go: "1.17"
+
+  stylecheck:
+    # Select the Go version to target. The default is '1.13'.
+    go: "1.17"
+
+  unparam:
+    # Inspect exported functions, default is false. Set to true if no external program/library imports your code.
+    # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors:
+    # if it's called for subdir of a project it can't find external interfaces. All text editor integrations
+    # with golangci-lint call it on a directory with the changed file.
+    check-exported: false
+
+  unused:
+    # Select the Go version to target. The default is '1.13'.
+    go: "1.17"

config/config.go

@@ -38,6 +38,8 @@ type Namespace struct {
 	config *C
 }
 
+const mask = "xxxxx"
+
 var (
 	configOpts = []ucfg.Option{
 		ucfg.PathSep("."),
@@ -60,11 +62,6 @@ var (
 	)
 )
 
-const (
-	selectorConfig             = "config"
-	selectorConfigWithPassword = "config-with-passwords"
-)
-
 func NewConfig() *C {
 	return fromConfig(ucfg.New())
 }
@@ -369,10 +366,10 @@ func applyLoggingMask(c interface{}) {
 			if maskList.Has(strings.ToLower(k)) {
 				if arr, ok := v.([]interface{}); ok {
 					for i := range arr {
-						arr[i] = "xxxxx"
+						arr[i] = mask
 					}
 				} else {
-					cfg[k] = "xxxxx"
+					cfg[k] = mask
 				}
 			} else {
 				applyLoggingMask(v)

config/flags.go

@@ -25,6 +25,8 @@ import (
 	cfgflag "github.com/elastic/go-ucfg/flag"
 )
 
+const typeString = "string"
+
 // StringsFlag collects multiple usages of the same flag into an array of strings.
 // Duplicate values will be ignored.
 type StringsFlag struct {
@@ -157,7 +159,7 @@ func (f *StringsFlag) List() []string {
 // Type reports the type of contents (string) expected to be parsed by Set.
 // It is used to build the CLI usage string.
 func (f *StringsFlag) Type() string {
-	return "string"
+	return typeString
 }
 
 // SettingFlag defines a setting flag, name and it's usage. The return value is
@@ -214,7 +216,7 @@ func (f *SettingsFlag) Get() interface{} {
 	return f.Config()
 }
 
-// String always returns an empty string. It is required to fulfil
+// String always returns an empty string. It is required to fulfill
 // the flag.Value interface.
 func (f *SettingsFlag) String() string {
 	return ""
@@ -286,5 +288,5 @@ func (f *flagOverwrite) Get() interface{} {
 }
 
 func (f *flagOverwrite) Type() string {
-	return "string"
+	return typeString
 }

config/flags_test.go

@@ -28,6 +28,7 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestStringArrFlag(t *testing.T) {
@@ -158,7 +159,8 @@ func TestOverwriteFlag(t *testing.T) {
 	cobraExpectedUsage := "  -a, --a string   message\n"
 	assert.Equal(t, cobraExpectedUsage, cobraUsage)
 
-	fs.Set("a", "overwrite")
+	err = fs.Set("a", "overwrite")
+	require.NoError(t, err)
 	final, err := config.String("a", -1)
 	assert.NoError(t, err)
 	assert.Equal(t, "overwrite", final)

file/helper_other.go

@@ -37,8 +37,10 @@ func SafeFileRotate(path, tempfile string) error {
 	// filesystems, so to update the parents directory metadata to actually
 	// contain the new file being rotated in.
 	f, err := os.Open(parent)
+
+	// nolint: nilerr // ignore error
 	if err != nil {
-		return nil // ignore error
+		return nil
 	}
 	defer f.Close()
 

file/helper_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+// nolint: gosec // file permissions are valid for test purposes
 func TestSafeFileRotateExistingFile(t *testing.T) {
 	tempdir, err := ioutil.TempDir("", "")
 	assert.NoError(t, err)