From ce9e668a93263786cadb3ba86ea8c96f8832a598 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 6 Jun 2023 13:15:11 +0200 Subject: [PATCH] golangci-lint: suppress one issue, demote others to "hints" The voting in https://github.com/kubernetes/kubernetes/issues/117288 led to one check that got rejected ("ifElseChain: rewrite if-else to switch statement") and several that are "nice to know". golangci-lint's support for issue "severity" is too limited to identify "nice to know" issues in the output (filtering is only by linter without considering the issue text; not part of text output). Therefore a third configuration gets added which emits all issues (must fix and nits). The intention is to use the "strict" configuration in pull-kubernetes-verify and the "hints" configuration in a new non-blocking pull-kubernetes-linter-hints. That way, "must fix" issues will block merging while issues that may be useful will show up in a failed optional job. However, that job then also contains "must fix" issues, partly because filtering out those would make the configuration a lot larger and is likely to be unreliably (all "must fix" issues would need to be identified and listed), partly because it may be useful to have all issues in one place. The previous approach of manually keeping two configs in sync with special comments didn't scale to three configs. Now a single golangci.yaml.in with text/template constructs contains the source for all three configs. A new simple CLI frontend for text/template (cmd/gotemplate) is used by hack/update-golangci-lint-config.sh to generate the three flavors. --- cmd/gotemplate/OWNERS | 11 ++ cmd/gotemplate/gotemplate.go | 97 +++++++++++++++++ cmd/gotemplate/gotemplate_test.go | 108 +++++++++++++++++++ hack/golangci-hints.yaml | 91 ++++++++++++++++ hack/golangci-strict.yaml | 85 +++++++++++++-- hack/golangci.yaml | 99 ++++++++++++++--- hack/golangci.yaml.in | 159 ++++++++++++++++++++++++++++ hack/logcheck.yaml | 6 ++ hack/update-golangci-lint-config.sh | 44 ++++++++ hack/verify-golangci-lint-config.sh | 24 +---- hack/verify-golangci-lint.sh | 23 +++- 11 files changed, 696 insertions(+), 51 deletions(-) create mode 100644 cmd/gotemplate/OWNERS create mode 100644 cmd/gotemplate/gotemplate.go create mode 100644 cmd/gotemplate/gotemplate_test.go create mode 100644 hack/golangci-hints.yaml create mode 100644 hack/golangci.yaml.in create mode 100644 hack/logcheck.yaml create mode 100755 hack/update-golangci-lint-config.sh diff --git a/cmd/gotemplate/OWNERS b/cmd/gotemplate/OWNERS new file mode 100644 index 00000000000..e634ba5e022 --- /dev/null +++ b/cmd/gotemplate/OWNERS @@ -0,0 +1,11 @@ +# See the OWNERS docs at https://go.k8s.io/owners + +reviewers: + - bentheelder + - dims + - pohly +approvers: + - dims + - pohly +labels: + - sig/testing diff --git a/cmd/gotemplate/gotemplate.go b/cmd/gotemplate/gotemplate.go new file mode 100644 index 00000000000..afd033c203c --- /dev/null +++ b/cmd/gotemplate/gotemplate.go @@ -0,0 +1,97 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "bytes" + "fmt" + "io" + "os" + "strings" + "text/template" +) + +// gotemplate is a simple CLI for text/template. It reads from stdin and writes to stdout. +// Optional arguments are = pairs which can be used as {{.}} to inject +// the for that key. +// +// Besides the default functions (https://pkg.go.dev/text/template#hdr-Functions), +// gotemplate also implements: +// - include : returns the content of that file as string +// - indent : replace each newline with "newline + spaces", indent the newline at the end +// - trim : strip leading and trailing whitespace + +func main() { + kvs := make(map[string]string) + + for _, keyValue := range os.Args[1:] { + index := strings.Index(keyValue, "=") + if index <= 0 { + fmt.Fprintf(os.Stderr, "optional arguments must be of the form =, got instead: %q\n", keyValue) + os.Exit(1) + } + kvs[keyValue[0:index]] = keyValue[index+1:] + } + + if err := generate(os.Stdin, os.Stdout, kvs); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } +} + +func generate(in io.Reader, out io.Writer, data interface{}) error { + var buf bytes.Buffer + if _, err := buf.ReadFrom(in); err != nil { + return fmt.Errorf("reading input: %v", err) + } + + funcMap := template.FuncMap{ + "include": include, + "indent": indent, + "trim": trim, + } + + tmpl, err := template.New("").Funcs(funcMap).Parse(buf.String()) + if err != nil { + return fmt.Errorf("parsing input as text template: %v", err) + } + + if err := tmpl.Execute(out, data); err != nil { + return fmt.Errorf("generating result: %v", err) + } + return nil +} + +func include(filename string) (string, error) { + content, err := os.ReadFile(filename) + if err != nil { + return "", err + } + return string(content), nil +} + +func indent(numSpaces int, content string) string { + if content == "" { + return "" + } + prefix := strings.Repeat(" ", numSpaces) + return strings.ReplaceAll(content, "\n", "\n"+prefix) +} + +func trim(content string) string { + return strings.TrimSpace(content) +} diff --git a/cmd/gotemplate/gotemplate_test.go b/cmd/gotemplate/gotemplate_test.go new file mode 100644 index 00000000000..3322702ad96 --- /dev/null +++ b/cmd/gotemplate/gotemplate_test.go @@ -0,0 +1,108 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "bytes" + "os" + "path" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGenerate(t *testing.T) { + for name, tt := range map[string]struct { + in string + data map[string]string + files map[string]string + expected string + expectedErr string + }{ + "missing-file": { + in: `{{include "no-such-file.txt"}}`, + expectedErr: "open no-such-file.txt: no such file or directory", + }, + "data": { + in: `{{.Hello}} {{.World}}`, + data: map[string]string{"Hello": "world", "World": "hello"}, + expected: "world hello", + }, + "include": { + in: `{{include "test.txt" | indent 2}}`, + files: map[string]string{"test.txt": "hello\nworld"}, + expected: "hello\n world", + }, + } { + cwd, err := os.Getwd() + require.NoError(t, err) + + t.Run(name, func(t *testing.T) { + tmp := t.TempDir() + for fileName, fileContent := range tt.files { + err := os.WriteFile(path.Join(tmp, fileName), []byte(fileContent), 0666) + require.NoError(t, err, "create input file") + } + defer os.Chdir(cwd) + require.NoError(t, os.Chdir(tmp), "change into tmp directory") + in := strings.NewReader(tt.in) + var out bytes.Buffer + err := generate(in, &out, tt.data) + if tt.expectedErr == "" { + require.NoError(t, err, "expand template") + require.Equal(t, tt.expected, out.String()) + } else { + require.Contains(t, err.Error(), tt.expectedErr) + } + }) + } +} + +func TestIndent(t *testing.T) { + for name, tt := range map[string]struct { + numSpaces int + content string + expected string + }{ + "empty": { + numSpaces: 10, + content: "", + expected: "", + }, + "trailing-newline": { + numSpaces: 2, + content: "hello\nworld\n", + expected: "hello\n world\n ", + }, + "no-trailing-newline": { + numSpaces: 1, + content: "hello\nworld", + expected: "hello\n world", + }, + "zero-indent": { + numSpaces: 0, + content: "hello\nworld", + expected: "hello\nworld", + }, + } { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tt.expected, indent(tt.numSpaces, tt.content)) + }) + } +} diff --git a/hack/golangci-hints.yaml b/hack/golangci-hints.yaml new file mode 100644 index 00000000000..527f267bff1 --- /dev/null +++ b/hack/golangci-hints.yaml @@ -0,0 +1,91 @@ +# golangci-lint is used in Kubernetes with different configurations that +# enable an increasing amount of checks: +# - golangci.yaml is the most permissive configuration. All existing code +# passed. +# - golangci-strict.yaml adds checks that all new code in pull requests +# must pass. +# - golangci-hints.yaml adds checks for code patterns where developer +# and reviewer may decide whether findings should get addressed before +# merging. Beware that the golangci-lint output includes also the +# issues that must be fixed and doesn't indicate how severe each issue +# is (https://gophers.slack.com/archives/CS0TBRKPC/p1685721815275349). +# +# All three flavors are generated from golangci.yaml.in with +# hack/update-golangci-lint-config.sh. + +run: + timeout: 30m + skip-files: + - "^zz_generated.*" + +output: + sort-results: true + +issues: + max-issues-per-linter: 0 + max-same-issues: 0 + # Excluding configuration per-path, per-linter, per-text and per-source + exclude-rules: + # exclude ineffassign linter for generated files for conversion + - path: conversion\.go + linters: + - ineffassign + + # SSA Extract calls are allowed in tests. + - linters: + - forbidigo + text: should not be used because managedFields was removed + path: _test.go$ + + # The Kubernetes naming convention for conversion functions uses underscores + # and intentionally deviates from normal Go conventions to make those function + # names more readable. Same for SetDefaults_*. + # + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507028627 + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1514201592 + - linters: + - stylecheck + text: "ST1003: should not use underscores in Go names; func (Convert_.*_To_.*|SetDefaults_)" + + # This check currently has some false positives (https://github.com/nunnatsa/ginkgolinter/issues/91). + - linters: + - ginkgolinter + text: use a function call in (Eventually|Consistently) + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507012435 + - linters: + - gocritic + text: "ifElseChain: rewrite if-else to switch statement" + +linters: + disable-all: false + enable: # please keep this alphabetized + - forbidigo + - ginkgolinter + - gocritic + - govet + - ineffassign + - logcheck + - staticcheck + - stylecheck + - unused + +linters-settings: # please keep this alphabetized + custom: + logcheck: + # Installed there by hack/verify-golangci-lint.sh. + path: ../_output/local/bin/logcheck.so + description: structured logging checker + original-url: k8s.io/logtools/logcheck + forbidigo: + analyze-types: true + forbid: + - p: ^managedfields\.ExtractInto$ + pkg: ^k8s\.io/apimachinery/pkg/util/managedfields$ + msg: should not be used because managedFields was removed + - p: \.Extract + pkg: ^k8s\.io/client-go/applyconfigurations/ + msg: should not be used because managedFields was removed + staticcheck: + checks: + - "all" diff --git a/hack/golangci-strict.yaml b/hack/golangci-strict.yaml index 2515eb81ec8..57880634914 100644 --- a/hack/golangci-strict.yaml +++ b/hack/golangci-strict.yaml @@ -1,13 +1,28 @@ -# This file configures checks that all new code for Kubernetes is meant to -# pass, in contrast to .golangci.yaml which defines checks that also the -# existing code passes. +# golangci-lint is used in Kubernetes with different configurations that +# enable an increasing amount of checks: +# - golangci.yaml is the most permissive configuration. All existing code +# passed. +# - golangci-strict.yaml adds checks that all new code in pull requests +# must pass. +# - golangci-hints.yaml adds checks for code patterns where developer +# and reviewer may decide whether findings should get addressed before +# merging. Beware that the golangci-lint output includes also the +# issues that must be fixed and doesn't indicate how severe each issue +# is (https://gophers.slack.com/archives/CS0TBRKPC/p1685721815275349). +# +# All three flavors are generated from golangci.yaml.in with +# hack/update-golangci-lint-config.sh. run: timeout: 30m skip-files: - "^zz_generated.*" +output: + sort-results: true + issues: + max-issues-per-linter: 0 max-same-issues: 0 # Excluding configuration per-path, per-linter, per-text and per-source exclude-rules: @@ -15,25 +30,72 @@ issues: - path: conversion\.go linters: - ineffassign + + # SSA Extract calls are allowed in tests. + - linters: + - forbidigo + text: should not be used because managedFields was removed + path: _test.go$ + # The Kubernetes naming convention for conversion functions uses underscores # and intentionally deviates from normal Go conventions to make those function # names more readable. Same for SetDefaults_*. + # + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507028627 + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1514201592 - linters: - stylecheck text: "ST1003: should not use underscores in Go names; func (Convert_.*_To_.*|SetDefaults_)" + # This check currently has some false positives (https://github.com/nunnatsa/ginkgolinter/issues/91). - linters: - ginkgolinter text: use a function call in (Eventually|Consistently) - # SSA Extract calls are allowed in tests. + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507012435 - linters: - - forbidigo - text: should not be used because managedFields was removed - path: _test.go$ + - gocritic + text: "ifElseChain: rewrite if-else to switch statement" + + # The following issues were deemed "might be worth fixing, needs to be + # decided on a case-by-case basis". This was initially decided by a + # majority of the developers who voted in + # https://github.com/kubernetes/kubernetes/issues/117288 and may evolve + # over time. + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507008918 + - linters: + - gocritic + text: "assignOp:" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507016854 + - linters: + - gosimple + text: "S1002: should omit comparison to bool constant" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507023980 + - linters: + - gosimple + text: "S1016: should convert opts .* instead of using struct literal" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507026758 + - linters: + - gosimple + text: "S1033: unnecessary guard around call to delete" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507030071 + - linters: + - stylecheck + text: "ST1012: error var .* should have name of the form ErrFoo" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507031224 + - linters: + - stylecheck + text: "ST1023: should omit type .* from declaration; it will be inferred from the right-hand side" linters: - disable-all: false # in contrast to golangci.yaml, the default set of linters remains enabled - enable: # please keep this alphabetized and in sync with golangci.yaml + disable-all: false + enable: # please keep this alphabetized - forbidigo - ginkgolinter - gocritic @@ -43,6 +105,9 @@ linters: - staticcheck - stylecheck - unused + disable: + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507008359 + - errcheck linters-settings: # please keep this alphabetized custom: @@ -60,8 +125,6 @@ linters-settings: # please keep this alphabetized - p: \.Extract pkg: ^k8s\.io/client-go/applyconfigurations/ msg: should not be used because managedFields was removed - gocritic: staticcheck: checks: - "all" - stylecheck: diff --git a/hack/golangci.yaml b/hack/golangci.yaml index 448374acf09..78b5f4b8326 100644 --- a/hack/golangci.yaml +++ b/hack/golangci.yaml @@ -1,9 +1,28 @@ +# golangci-lint is used in Kubernetes with different configurations that +# enable an increasing amount of checks: +# - golangci.yaml is the most permissive configuration. All existing code +# passed. +# - golangci-strict.yaml adds checks that all new code in pull requests +# must pass. +# - golangci-hints.yaml adds checks for code patterns where developer +# and reviewer may decide whether findings should get addressed before +# merging. Beware that the golangci-lint output includes also the +# issues that must be fixed and doesn't indicate how severe each issue +# is (https://gophers.slack.com/archives/CS0TBRKPC/p1685721815275349). +# +# All three flavors are generated from golangci.yaml.in with +# hack/update-golangci-lint-config.sh. + run: timeout: 30m skip-files: - "^zz_generated.*" +output: + sort-results: true + issues: + max-issues-per-linter: 0 max-same-issues: 0 # Excluding configuration per-path, per-linter, per-text and per-source exclude-rules: @@ -11,30 +30,78 @@ issues: - path: conversion\.go linters: - ineffassign + + # SSA Extract calls are allowed in tests. + - linters: + - forbidigo + text: should not be used because managedFields was removed + path: _test.go$ + # TODO(oscr) Remove these excluded directories and fix findings. Due to large amount of findings in different components # with different owners it's hard to fix everything in a single pr. This will therefore be done in multiple prs. - - path: (pkg/volume/*|test/*|azure/*|pkg/cmd/wait*|request/bearertoken/*|metrics/*|filters/*) # not in golangci-strict.yaml - linters: # not in golangci-strict.yaml - - gocritic # not in golangci-strict.yaml + - path: (pkg/volume/*|test/*|azure/*|pkg/cmd/wait*|request/bearertoken/*|metrics/*|filters/*) + linters: + - gocritic + # The Kubernetes naming convention for conversion functions uses underscores # and intentionally deviates from normal Go conventions to make those function # names more readable. Same for SetDefaults_*. + # + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507028627 + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1514201592 - linters: - stylecheck text: "ST1003: should not use underscores in Go names; func (Convert_.*_To_.*|SetDefaults_)" + # This check currently has some false positives (https://github.com/nunnatsa/ginkgolinter/issues/91). - linters: - ginkgolinter text: use a function call in (Eventually|Consistently) - # SSA Extract calls are allowed in tests. + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507012435 - linters: - - forbidigo - text: should not be used because managedFields was removed - path: _test.go$ + - gocritic + text: "ifElseChain: rewrite if-else to switch statement" + + # The following issues were deemed "might be worth fixing, needs to be + # decided on a case-by-case basis". This was initially decided by a + # majority of the developers who voted in + # https://github.com/kubernetes/kubernetes/issues/117288 and may evolve + # over time. + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507008918 + - linters: + - gocritic + text: "assignOp:" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507016854 + - linters: + - gosimple + text: "S1002: should omit comparison to bool constant" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507023980 + - linters: + - gosimple + text: "S1016: should convert opts .* instead of using struct literal" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507026758 + - linters: + - gosimple + text: "S1033: unnecessary guard around call to delete" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507030071 + - linters: + - stylecheck + text: "ST1012: error var .* should have name of the form ErrFoo" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507031224 + - linters: + - stylecheck + text: "ST1023: should omit type .* from declaration; it will be inferred from the right-hand side" linters: - disable-all: true # not disabled in golangci-strict.yaml - enable: # please keep this alphabetized and in sync with golangci-strict.yaml + disable-all: true + enable: # please keep this alphabetized - forbidigo - ginkgolinter - gocritic @@ -62,14 +129,14 @@ linters-settings: # please keep this alphabetized pkg: ^k8s\.io/client-go/applyconfigurations/ msg: should not be used because managedFields was removed gocritic: - enabled-checks: # not limited in golangci-strict.yaml - - equalFold # not limited in golangci-strict.yaml - - boolExprSimplify # not limited in golangci-strict.yaml + enabled-checks: + - equalFold + - boolExprSimplify staticcheck: checks: - "all" - - "-SA1019" # TODO(fix) Using a deprecated function, variable, constant or field - enabled in golangci-strict.yaml - - "-SA2002" # TODO(fix) Called testing.T.FailNow or SkipNow in a goroutine, which isn’t allowed - enabled in golangci-strict.yaml + - "-SA1019" # TODO(fix) Using a deprecated function, variable, constant or field + - "-SA2002" # TODO(fix) Called testing.T.FailNow or SkipNow in a goroutine, which isn’t allowed stylecheck: - checks: # golangci-strict.yaml uses the default checks. - - "ST1019" # Importing the same package multiple times - enabled in golangci-strict.yaml. + checks: + - "ST1019" # Importing the same package multiple times diff --git a/hack/golangci.yaml.in b/hack/golangci.yaml.in new file mode 100644 index 00000000000..ebdcf426c3a --- /dev/null +++ b/hack/golangci.yaml.in @@ -0,0 +1,159 @@ +# golangci-lint is used in Kubernetes with different configurations that +# enable an increasing amount of checks: +# - golangci.yaml is the most permissive configuration. All existing code +# passed. +# - golangci-strict.yaml adds checks that all new code in pull requests +# must pass. +# - golangci-hints.yaml adds checks for code patterns where developer +# and reviewer may decide whether findings should get addressed before +# merging. Beware that the golangci-lint output includes also the +# issues that must be fixed and doesn't indicate how severe each issue +# is (https://gophers.slack.com/archives/CS0TBRKPC/p1685721815275349). +# +# All three flavors are generated from golangci.yaml.in with +# hack/update-golangci-lint-config.sh. + +run: + timeout: 30m + skip-files: + - "^zz_generated.*" + +output: + sort-results: true + +issues: + max-issues-per-linter: 0 + max-same-issues: 0 + # Excluding configuration per-path, per-linter, per-text and per-source + exclude-rules: + # exclude ineffassign linter for generated files for conversion + - path: conversion\.go + linters: + - ineffassign + + # SSA Extract calls are allowed in tests. + - linters: + - forbidigo + text: should not be used because managedFields was removed + path: _test.go$ + + {{- if .Base}} + + # TODO(oscr) Remove these excluded directories and fix findings. Due to large amount of findings in different components + # with different owners it's hard to fix everything in a single pr. This will therefore be done in multiple prs. + - path: (pkg/volume/*|test/*|azure/*|pkg/cmd/wait*|request/bearertoken/*|metrics/*|filters/*) + linters: + - gocritic + {{- end}} + + # The Kubernetes naming convention for conversion functions uses underscores + # and intentionally deviates from normal Go conventions to make those function + # names more readable. Same for SetDefaults_*. + # + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507028627 + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1514201592 + - linters: + - stylecheck + text: "ST1003: should not use underscores in Go names; func (Convert_.*_To_.*|SetDefaults_)" + + # This check currently has some false positives (https://github.com/nunnatsa/ginkgolinter/issues/91). + - linters: + - ginkgolinter + text: use a function call in (Eventually|Consistently) + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507012435 + - linters: + - gocritic + text: "ifElseChain: rewrite if-else to switch statement" + + {{- if not .Hints}} + + # The following issues were deemed "might be worth fixing, needs to be + # decided on a case-by-case basis". This was initially decided by a + # majority of the developers who voted in + # https://github.com/kubernetes/kubernetes/issues/117288 and may evolve + # over time. + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507008918 + - linters: + - gocritic + text: "assignOp:" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507016854 + - linters: + - gosimple + text: "S1002: should omit comparison to bool constant" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507023980 + - linters: + - gosimple + text: "S1016: should convert opts .* instead of using struct literal" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507026758 + - linters: + - gosimple + text: "S1033: unnecessary guard around call to delete" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507030071 + - linters: + - stylecheck + text: "ST1012: error var .* should have name of the form ErrFoo" + + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507031224 + - linters: + - stylecheck + text: "ST1023: should omit type .* from declaration; it will be inferred from the right-hand side" + {{- end}} + +linters: + disable-all: {{if .Base -}} true {{- else -}} false {{- end}} + enable: # please keep this alphabetized + - forbidigo + - ginkgolinter + - gocritic + - govet + - ineffassign + - logcheck + - staticcheck + - stylecheck + - unused + {{- if .Strict}} + disable: + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507008359 + - errcheck + {{- end}} + +linters-settings: # please keep this alphabetized + custom: + logcheck: + # Installed there by hack/verify-golangci-lint.sh. + path: ../_output/local/bin/logcheck.so + description: structured logging checker + original-url: k8s.io/logtools/logcheck + forbidigo: + analyze-types: true + forbid: + - p: ^managedfields\.ExtractInto$ + pkg: ^k8s\.io/apimachinery/pkg/util/managedfields$ + msg: should not be used because managedFields was removed + - p: \.Extract + pkg: ^k8s\.io/client-go/applyconfigurations/ + msg: should not be used because managedFields was removed + {{- if .Base }} + gocritic: + enabled-checks: + - equalFold + - boolExprSimplify + {{- end}} + staticcheck: + checks: + - "all" + {{- if .Base }} + - "-SA1019" # TODO(fix) Using a deprecated function, variable, constant or field + - "-SA2002" # TODO(fix) Called testing.T.FailNow or SkipNow in a goroutine, which isn’t allowed + {{- end}} + {{- if .Base }} + stylecheck: + checks: + - "ST1019" # Importing the same package multiple times + {{- end}} diff --git a/hack/logcheck.yaml b/hack/logcheck.yaml new file mode 100644 index 00000000000..66047a693c1 --- /dev/null +++ b/hack/logcheck.yaml @@ -0,0 +1,6 @@ +# Installed there by hack/verify-golangci-lint.sh. +path: ../_output/local/bin/logcheck.so +description: structured logging checker +original-url: k8s.io/logtools/logcheck + settings: + config: /nvme/gopath/src/k8s.io/kubernetes/hack/logcheck.conf diff --git a/hack/update-golangci-lint-config.sh b/hack/update-golangci-lint-config.sh new file mode 100755 index 00000000000..172ab070018 --- /dev/null +++ b/hack/update-golangci-lint-config.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash + +# Copyright 2023 The Kubernetes Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# This script checks that golangci-strict.yaml and golangci.yaml remain in +# sync. Lines that are intentionally different must have a comment which +# mentions golangci.yaml or golangci-lint.yaml. + +set -o errexit +set -o nounset +set -o pipefail + +KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. +source "${KUBE_ROOT}/hack/lib/init.sh" + +# This sets up the environment, like GOCACHE, which keeps the worktree cleaner. +kube::golang::setup_env + +# Remove all files, some of them might be obsolete. +rm -f hack/golangci*.yaml + +generate () { + out="$1" + shift + echo "Generating $out from hack/golangci.yaml.in with ./cmd/gotemplate $*" + go run ./cmd/gotemplate "${out}" "$@" +} + +# Regenerate. +generate hack/golangci.yaml Base=1 +generate hack/golangci-strict.yaml Strict=1 +generate hack/golangci-hints.yaml Hints=1 diff --git a/hack/verify-golangci-lint-config.sh b/hack/verify-golangci-lint-config.sh index 93954551fad..236d3e589e5 100755 --- a/hack/verify-golangci-lint-config.sh +++ b/hack/verify-golangci-lint-config.sh @@ -14,30 +14,14 @@ # See the License for the specific language governing permissions and # limitations under the License. -# This script checks that golangci-strict.yaml and golangci.yaml remain in -# sync. Lines that are intentionally different must have a comment which -# mentions golangci.yaml or golangci-lint.yaml. +# This script checks that all generated golangci-lint configurations +# are up-to-date. set -o errexit set -o nounset set -o pipefail KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. +source "${KUBE_ROOT}/hack/lib/verify-generated.sh" -if differences=$(diff --context --ignore-blank-lines \ - --ignore-matching-lines='^ *#' \ - --ignore-matching-lines='#.*golangci\(-strict\)*.yaml' \ - "${KUBE_ROOT}/hack/golangci.yaml" "${KUBE_ROOT}/hack/golangci-strict.yaml" ); then - echo "hack/golangci.yaml and hack/golangci-strict.yaml are synchronized." -else - cat >&2 <|-a] [-s] [-c none|] [-- : also write results with --out-format=github-actions to a separate file -c : use the specified configuration or none instead of the default hack/golangci.yaml @@ -65,8 +67,9 @@ golangci=(env LOGCHECK_CONFIG="${KUBE_ROOT}/hack/logcheck.conf" "${GOBIN}/golang golangci_config="${KUBE_ROOT}/hack/golangci.yaml" base= strict= +hints= githubactions= -while getopts "ar:sg:c:" o; do +while getopts "ar:sng:c:" o; do case "${o}" in a) base="$(git merge-base origin/master HEAD)" @@ -83,6 +86,10 @@ while getopts "ar:sg:c:" o; do golangci_config="${KUBE_ROOT}/hack/golangci-strict.yaml" strict=1 ;; + n) + golangci_config="${KUBE_ROOT}/hack/golangci-hints.yaml" + hints=1 + ;; g) githubactions="${OPTARG}" ;; @@ -192,9 +199,17 @@ else echo 'If the above warnings do not make sense, you can exempt this warning with a comment' echo ' (if your reviewer is okay with it).' if [ "$strict" ]; then - echo 'The more strict golangci-strict.yaml was used. If you feel that this warns about issues' - echo 'that should be ignored by default, then please discuss with your reviewer and propose' - echo 'a change for hack/golangci-strict.yaml as part of your PR.' + echo 'The more strict golangci-strict.yaml was used.' + elif [ "$hints" ]; then + echo 'The golangci-hints.yaml was used. Some of the reported issues may have to be fixed' + echo 'while others can be ignored, depending on the circumstances and/or personal' + echo 'preferences. To determine which issues have to be fixed, check the report that' + echo 'uses golangci-strict.yaml.' + fi + if [ "$strict" ] || [ "$hints" ]; then + echo 'If you feel that this warns about issues that should be ignored by default,' + echo 'then please discuss with your reviewer and propose' + echo 'a change for hack/golangci.yaml.in as part of your PR.' fi echo 'In general please prefer to fix the error, we have already disabled specific lints' echo ' that the project chooses to ignore.'