diff --git a/hack/golangci-hints.yaml b/hack/golangci-hints.yaml index b411e7abeb0..660600b28cd 100644 --- a/hack/golangci-hints.yaml +++ b/hack/golangci-hints.yaml @@ -2,8 +2,6 @@ # 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 diff --git a/hack/golangci-strict.yaml b/hack/golangci-strict.yaml deleted file mode 100644 index 6c55b51e8ca..00000000000 --- a/hack/golangci-strict.yaml +++ /dev/null @@ -1,269 +0,0 @@ -# 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 - - # The default excludes disable the "should have comment or be unexported" check from revive. - # We want that to be enabled, therefore we have to disable all default excludes and - # add those back one-by-one that we want. See https://github.com/golangci/golangci-lint/issues/456#issuecomment-617470264 - exclude-use-default: false - exclude: - # staticcheck: Developers tend to write in C-style with an explicit 'break' in a 'switch', so it's ok to ignore - - ineffective break statement. Did you mean to break out of the outer loop - - # 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$ - - # Adding unversioned feature gates is allowed in tests - - linters: - - forbidigo - text: should not use MutableFeatureGate.Add, use AddVersioned instead - 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 - - revive - text: "(ST1003: should not use underscores in Go names; func (Convert_.*_To_.*|SetDefaults_)|exported: exported function (Convert|SetDefaults)_.* should be of the form)" - - # 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" - - # Only packages listed here opt into the strict "exported symbols must be documented". - # - # Exclude texts from https://github.com/golangci/golangci-lint/blob/ab3c3cd69e602ff53bb4c3e2c188f0caeb80305d/pkg/config/issues.go#L11-L103 - - linters: - - golint - - revive - - stylecheck - text: comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form|comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form|exported (.+) should have comment( \(or a comment on this block\))? or be unexported|package comment should be of the form "(.+)...|comment on exported (.+) should be of the form "(.+)...|should have a package comment - path-except: cmd/kubeadm - - # The unused linter that comes from staticcheck currently does not handle types which implement - # a generic interface. The linter incorrectly reports the implementations of unexported - # interface methods as unused. See https://github.com/dominikh/go-tools/issues/1294. - # Rather than exporting the interface methods, which makes the error go away but changes the - # semantics of the code, we ignore this error for affected files. - # This can be removed when the staticcheck implementation of this rule is fixed, which may - # depend on https://github.com/golang/go/issues/63982. - - linters: - - unused - path: staging/src/k8s.io/client-go/util/workqueue/metrics.go - - # 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" - - # Didn't make it into https://github.com/kubernetes/kubernetes/issues/117288. - # Discussion on Slack concluded that "it's hard to have a universal policy for all - # functions marked deprecated" and thus this can only be a hint which must - # be considered on a case-by-case basis. - - linters: - - staticcheck - text: "SA1019: .*is deprecated" - - # 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 - enable: # please keep this alphabetized - - depguard - - forbidigo - - ginkgolinter - - gocritic - - govet - - ineffassign - - logcheck - - revive - - staticcheck - - stylecheck - - testifylint - - unused - - usestdlibvars - disable: - # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507008359 - - errcheck - -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 - settings: - config: | - # hack/logcheck.conf contains regular expressions that are matched against /, - # for example k8s.io/cmd/kube-scheduler/app/config/config.go. - # - # By default, structured logging call parameters are checked, but usage of - # those calls is not required. That is changed on a per-file basis. - # - # Remember to clean the golangci-lint cache when changing the configuration and - # running the verify-golangci-lint.sh script multiple times, otherwise - # golangci-lint will report stale results: - # _output/local/bin/golangci-lint cache clean - - # At this point we don't enforce the usage structured logging calls except in - # those packages that were migrated. This disables the check for other files. - -structured .* - - # Now enable it again for migrated packages. - structured k8s.io/kubernetes/cmd/kubelet/.* - structured k8s.io/kubernetes/pkg/kubelet/.* - structured k8s.io/kubernetes/pkg/proxy/.* - structured k8s.io/kms/.* - structured k8s.io/apiserver/pkg/storage/value/.* - structured k8s.io/apiserver/pkg/server/options/encryptionconfig/.* - - # The following packages have been migrated to contextual logging. - # Packages matched here do not have to be listed above because - # "contextual" implies "structured". - contextual k8s.io/api/.* - contextual k8s.io/apimachinery/pkg/util/runtime/.* - contextual k8s.io/client-go/metadata/.* - contextual k8s.io/client-go/rest/.* - contextual k8s.io/client-go/tools/cache/.* - contextual k8s.io/client-go/tools/events/.* - contextual k8s.io/client-go/tools/record/.* - contextual k8s.io/component-helpers/.* - contextual k8s.io/cri-api/.* - contextual k8s.io/cri-client/.* - contextual k8s.io/csi-translation-lib/.* - contextual k8s.io/dynamic-resource-allocation/.* - contextual k8s.io/endpointslice/.* - contextual k8s.io/kms/.* - contextual k8s.io/kube-controller-manager/.* - contextual k8s.io/kube-proxy/.* - contextual k8s.io/kube-scheduler/.* - contextual k8s.io/sample-apiserver/.* - contextual k8s.io/sample-cli-plugin/.* - contextual k8s.io/sample-controller/.* - contextual k8s.io/kubernetes/cmd/kube-proxy/.* - contextual k8s.io/kubernetes/cmd/kube-scheduler/.* - contextual k8s.io/kubernetes/pkg/controller/.* - contextual k8s.io/kubernetes/pkg/scheduler/.* - contextual k8s.io/kubernetes/test/e2e/dra/.* - contextual k8s.io/kubernetes/pkg/kubelet/cm/dra/.* - contextual k8s.io/kubernetes/pkg/kubelet/pleg/.* - contextual k8s.io/kubernetes/pkg/kubelet/clustertrustbundle/.* - contextual k8s.io/kubernetes/pkg/kubelet/token/.* - contextual k8s.io/kubernetes/pkg/kubelet/cadvisor/.* - contextual k8s.io/kubernetes/pkg/kubelet/oom/.* - contextual k8s.io/kubernetes/pkg/kubelet/sysctl/.* - - # As long as contextual logging is alpha or beta, all WithName, WithValues, - # NewContext calls have to go through klog. Once it is GA, we can lift - # this restriction. Whether we then do a global search/replace remains - # to be decided. - with-helpers .* - depguard: - rules: - main: - files: - - $all - - "!$test" - - "!**/test/**" - deny: - - pkg: "github.com/google/go-cmp/cmp" - desc: "cmp is allowed only in test files" - 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 - - p: \.Add$ - pkg: ^k8s\.io/component-base/featuregate$ - type: ^MutableFeatureGate$ - msg: should not use MutableFeatureGate.Add, use AddVersioned instead - revive: - # Only these rules are enabled. - rules: - - name: exported - arguments: - - disableStutteringCheck - staticcheck: - checks: - - "all" - testifylint: - enable-all: true - disable: # TODO: remove each disabled rule and fix it - - require-error diff --git a/hack/golangci.yaml b/hack/golangci.yaml index ba824f48363..ff24e6d6d18 100644 --- a/hack/golangci.yaml +++ b/hack/golangci.yaml @@ -2,8 +2,6 @@ # 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 diff --git a/hack/golangci.yaml.in b/hack/golangci.yaml.in index 94c6bd52c1c..c567600e841 100644 --- a/hack/golangci.yaml.in +++ b/hack/golangci.yaml.in @@ -2,8 +2,6 @@ # 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 @@ -171,11 +169,6 @@ linters: {{- if not .Base }} - usestdlibvars {{- end}} - {{- if .Strict}} - disable: - # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507008359 - - errcheck - {{- end}} linters-settings: # please keep this alphabetized custom: diff --git a/hack/update-golangci-lint-config.sh b/hack/update-golangci-lint-config.sh index 172ab070018..d85ef6eb372 100755 --- a/hack/update-golangci-lint-config.sh +++ b/hack/update-golangci-lint-config.sh @@ -40,5 +40,4 @@ generate () { # 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.sh b/hack/verify-golangci-lint.sh index 8bf4843cd61..939abca3ac3 100755 --- a/hack/verify-golangci-lint.sh +++ b/hack/verify-golangci-lint.sh @@ -25,7 +25,6 @@ Usage: $0 [-r |-a] [-s] [-c none|] [-- : only report issues in code added since that revision -a: automatically select the common base of origin/master and HEAD as revision - -s: select a strict configuration for new code -n: in addition to strict checking, also enable hints (aka nits) that may or may not be useful -g : also write results with --out-format=github-actions @@ -53,7 +52,6 @@ invocation=(./hack/verify-golangci-lint.sh "$@") golangci=("${GOBIN}/golangci-lint" run) golangci_config="${KUBE_ROOT}/hack/golangci.yaml" base= -strict= hints= githubactions= while getopts "ar:sng:c:" o; do @@ -69,10 +67,6 @@ while getopts "ar:sng:c:" o; do usage fi ;; - s) - golangci_config="${KUBE_ROOT}/hack/golangci-strict.yaml" - strict=1 - ;; n) golangci_config="${KUBE_ROOT}/hack/golangci-hints.yaml" hints=1 @@ -185,18 +179,12 @@ else echo "Please review the above warnings. You can test via \"${invocation[*]}\"" 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 - echo 'golangci-strict.yaml was used as configuration. Warnings must be fixed in' - echo 'new or modified code.' - elif [ "$hints" ]; then + if [ "$hints" ]; then echo echo 'golangci-hints.yaml was used as configuration. Some of the reported issues may' echo 'have to be fixed while others can be ignored, depending on the circumstances' echo 'and/or personal preferences. To determine which issues have to be fixed, check' - echo 'the report that uses golangci-strict.yaml (= pull-kubernetes-verify-lint).' - fi - if [ "$strict" ] || [ "$hints" ]; then + echo 'the report that uses golangci.yaml (= pull-kubernetes-verify).' echo echo 'If you feel that this warns about issues that should be ignored by default,' echo 'then please discuss with your reviewer and propose' @@ -208,12 +196,13 @@ else echo 'Instead, propose to fix certain linter issues in an issue first and' echo 'discuss there with maintainers. PRs are welcome if they address a real' echo 'problem, which then needs to be explained in the PR.' + else + echo + echo 'In general please prefer to fix the error, we have already disabled specific lints' + echo ' that the project chooses to ignore.' + echo 'See: https://golangci-lint.run/usage/false-positives/' fi echo - echo 'In general please prefer to fix the error, we have already disabled specific lints' - echo ' that the project chooses to ignore.' - echo 'See: https://golangci-lint.run/usage/false-positives/' - echo } >&2 exit 1 fi