From 15d9a93d7c847b69bd9d8824e83ef81c9c2aaaea Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 5 May 2025 12:39:45 +0200 Subject: [PATCH 1/2] golangci-lint: redirecting stderr raced with termination of script Redirecting stderr via a pipe raced with handling the result of the golangci-lint invocation. We need to block the execution until sed has fully processed all output. Because of this race, pull-kubernetes-linter-hints jobs failed with "Please review the above warnings" without actually showing any. --- hack/verify-golangci-lint.sh | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/hack/verify-golangci-lint.sh b/hack/verify-golangci-lint.sh index 565da63c519..8ae58eb8c00 100755 --- a/hack/verify-golangci-lint.sh +++ b/hack/verify-golangci-lint.sh @@ -184,7 +184,13 @@ run () { # Only output on stderr indicates a real error and gets the "ERROR: " prefix for # highlighting in Spyglass. Stdout contains statistics and shouldn't get that prefix. # To avoid interleaving, it gets collected and dumped separately at the end. - "${golangci[@]}" "${targets[@]}" 2> >(sed -e 's;^;ERROR: ;') >"${KUBE_TEMP}/golangci-stdout.log" || res=$? + # + # This is done with some bash magic: + # - save original stdout in FD 3 + # - redirect stdout to file + # - redirect stderr to original stdout in FD 3 + # - pipe stderr via stdout into sed for on-the-fly processing, writing to stderr again + "${golangci[@]}" "${targets[@]}" 3>&1 >"${KUBE_TEMP}/golangci-stdout.log" 2>&3 | sed -e 's;^;ERROR: ;' >&2 || res=$? cat "${KUBE_TEMP}/golangci-stdout.log" } # First run with normal output. From 2ea4b1aba60ebc0133272e87a221b05894bca77f Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 5 May 2025 12:57:13 +0200 Subject: [PATCH 2/2] golangci-lint: don't warn about conversion and defaulting functions The linter suppression got removed in the migration to golangci-lint v2 because it wasn't triggered for existing code, but as seen in a PR which adds a conversion package the suppression is still needed. Furthermore, we also want to suppress for unexported, manually written conversion and defaulting functions because those follow the same Kubernetes naming convention. --- hack/golangci-hints.yaml | 11 +++++++++++ hack/golangci.yaml | 11 +++++++++++ hack/golangci.yaml.in | 11 +++++++++++ 3 files changed, 33 insertions(+) diff --git a/hack/golangci-hints.yaml b/hack/golangci-hints.yaml index 54ffefcddbc..8306b7be5a1 100644 --- a/hack/golangci-hints.yaml +++ b/hack/golangci-hints.yaml @@ -91,6 +91,17 @@ linters: text: should not use 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: + - staticcheck + - revive + text: "(ST1003: should not use underscores in Go names; func ([cC]onvert_.*_To_.*|[sS]etDefaults_)|exported: exported function (Convert|SetDefaults)_.* should be of the form)" + - path: (.+)\.go$ # staticcheck: Developers tend to write in C-style with an explicit 'break' in a 'switch', so it's ok to ignore text: ineffective break statement. Did you mean to break out of the outer loop diff --git a/hack/golangci.yaml b/hack/golangci.yaml index ecdb29b5800..bc9c4bf9e72 100644 --- a/hack/golangci.yaml +++ b/hack/golangci.yaml @@ -91,6 +91,17 @@ linters: text: should not use 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: + - staticcheck + - revive + text: "(ST1003: should not use underscores in Go names; func ([cC]onvert_.*_To_.*|[sS]etDefaults_)|exported: exported function (Convert|SetDefaults)_.* should be of the form)" + # TODO(https://github.com/kubernetes/kubernetes/issues/131475): 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/*) diff --git a/hack/golangci.yaml.in b/hack/golangci.yaml.in index 30490288ff8..570aafdc239 100644 --- a/hack/golangci.yaml.in +++ b/hack/golangci.yaml.in @@ -91,6 +91,17 @@ linters: text: should not use 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: + - staticcheck + - revive + text: "(ST1003: should not use underscores in Go names; func ([cC]onvert_.*_To_.*|[sS]etDefaults_)|exported: exported function (Convert|SetDefaults)_.* should be of the form)" + {{- if .Hints}} - path: (.+)\.go$