Merge pull request #129875 from pohly/golangci-lint-strict-removal

golangci-lint: remove "strict" checking

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

hack/golangci-strict.yaml

@@ -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 <pkg>/<file>,
-          # 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

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

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:

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

hack/verify-golangci-lint.sh

@@ -25,7 +25,6 @@ Usage: $0 [-r <revision>|-a] [-s] [-c none|<config>] [-- <golangci-lint run flag
    -r <revision>: 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 <github action file>: 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
+    if [ "$hints" ]; then
-        echo
-        echo 'golangci-strict.yaml was used as configuration. Warnings must be fixed in'
-        echo 'new or modified code.'
-    elif [ "$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).'
+        echo 'the report that uses golangci.yaml (= pull-kubernetes-verify).'
-    fi
-    if [ "$strict" ] || [ "$hints" ]; then
         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