Merge pull request #109728 from pohly/lint-pull-requests

verify-golangci-lint.sh: support stricter checking in new code

hack/golangci-strict.yaml

@@ -0,0 +1,38 @@
+# 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.
+
+run:
+  timeout: 30m
+  skip-files:
+    - "^zz_generated.*"
+
+issues:
+  max-same-issues: 0
+  # Excluding configuration per-path, per-linter, per-text and per-source
+  exclude-rules:
+    # exclude ineffassing linter for generated files for conversion
+    - path: conversion\.go
+      linters:
+        - ineffassign
+
+linters:
+  enable: # please keep this alphabetized
+    - gocritic
+    - govet
+    - ineffassign
+    - logcheck
+    - staticcheck
+    - 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
+  staticcheck:
+    checks: [
+      "all",
+    ]

hack/make-rules/verify.sh

@@ -33,6 +33,7 @@ source "${KUBE_ROOT}/third_party/forked/shell2junit/sh2ju.sh"
 EXCLUDED_PATTERNS=(
   "verify-all.sh"                # this script calls the make rule and would cause a loop
   "verify-*-dockerized.sh"       # Don't run any scripts that intended to be run dockerized
+  "verify-golangci-lint-pr.sh"   # Don't run this as part of the block pull-kubernetes-verify yet. TODO(pohly): try this in a non-blocking job and then reconsider this.
   "verify-licenses.sh"           # runs in a separate job to monitor availability of the dependencies periodically
   )
 

hack/verify-golangci-lint-pr.sh

@@ -0,0 +1,37 @@
+#!/usr/bin/env bash
+
+# Copyright 2022 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 a PR for the coding style for the Go language files using
+# golangci-lint. It does nothing when invoked as part of a normal "make
+# verify".
+
+set -o errexit
+set -o nounset
+set -o pipefail
+
+if [ ! "${PULL_NUMBER:-}" ]; then
+  echo 'Not testing anything because this is not a pull request.'
+  exit 0
+fi
+
+KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/..
+
+# TODO (https://github.com/kubernetes/test-infra/issues/17056):
+# take this additional artifact and convert it to GitHub annotations
+# to make it easier to see these problems during a PR review.
+#
+# -g "${ARTIFACTS}/golangci-lint-githubactions.log"
+"${KUBE_ROOT}/hack/verify-golangci-lint.sh" -r "${PULL_BASE_SHA}" -s

hack/verify-golangci-lint.sh

@@ -14,19 +14,26 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-# This script checks coding style for go language files in each
+# This script checks the coding style for the Go language files using
-# Kubernetes package by golint.
+# golangci-lint. Which checks are enabled depends on command line flags. The
+# default is a minimal set of checks that all existing code passes without
+# issues.
 
 usage () {
   cat <<EOF >&2
-Usage: $0 [-- <golangci-lint run flags>] [packages]"
+Usage: $0 [-r <revision>|-a] [-s] [-c none|<config>] [-- <golangci-lint run flags>] [packages]"
+   -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
+   -g <github action file>: also write results with --out-format=github-actions
+       to a separate file
    -c <config|"none">: use the specified configuration or none instead of the default hack/golangci.yaml
    [packages]: check specific packages or directories instead of everything
 EOF
   exit 1
 }
 
-
 set -o errexit
 set -o nounset
 set -o pipefail
@@ -50,8 +57,29 @@ invocation=(./hack/verify-golangci-lint.sh "$@")
 # _output/local/bin/golangci-lint cache clean
 golangci=(env LOGCHECK_CONFIG="${KUBE_ROOT}/hack/logcheck.conf" "${GOBIN}/golangci-lint" run)
 golangci_config="${KUBE_ROOT}/hack/golangci.yaml"
-while getopts "c:" o; do
+base=
+strict=
+githubactions=
+while getopts "ar:sg:c:" o; do
   case "${o}" in
+    a)
+      base="$(git merge-base origin/master HEAD)"
+      ;;
+    r)
+      base="${OPTARG}"
+      if [ ! "$base" ]; then
+        echo "ERROR: -c needs a non-empty parameter" >&2
+        echo >&2
+        usage
+      fi
+      ;;
+    s)
+      golangci_config="${KUBE_ROOT}/hack/golangci-strict.yaml"
+      strict=1
+      ;;
+    g)
+      githubactions="${OPTARG}"
+      ;;
     c)
       if [ "${OPTARG}" = "none" ]; then
         golangci_config=""
@@ -69,6 +97,16 @@ if [ "${golangci_config}" ]; then
     golangci+=(--config="${golangci_config}")
 fi
 
+if [ "$base" ]; then
+    # Must be a something that git can resolve to a commit.
+    # "git rev-parse --verify" checks that and prints a detailed
+    # error.
+    base="$(git rev-parse --verify "$base")"
+    golangci+=(--new --new-from-rev="$base")
+fi
+
+kube::golang::verify_go_version
+
 # Filter out arguments that start with "-" and move them to the run flags.
 shift $((OPTIND-1))
 targets=()
@@ -98,19 +136,31 @@ popd >/dev/null
 cd "${KUBE_ROOT}"
 
 res=0
-if [[ "${#targets[@]}" -gt 0 ]]; then
+run () {
+  if [[ "${#targets[@]}" -gt 0 ]]; then
     echo "running ${golangci[*]} ${targets[*]}" >&2
     "${golangci[@]}" "${targets[@]}" >&2 || res=$?
-else
+  else
     echo "running ${golangci[*]} ./..." >&2
     "${golangci[@]}" ./... >&2 || res=$?
     for d in staging/src/k8s.io/*; do
-        MODPATH="staging/src/k8s.io/$(basename "${d}")"
+      MODPATH="staging/src/k8s.io/$(basename "${d}")"
-        echo "running ( cd ${KUBE_ROOT}/${MODPATH}; ${golangci[*]} --path-prefix ${MODPATH} ./... )"
+      echo "running ( cd ${KUBE_ROOT}/${MODPATH}; ${golangci[*]} --path-prefix ${MODPATH} ./... )"
-        pushd "${KUBE_ROOT}/${MODPATH}" >/dev/null
+      pushd "${KUBE_ROOT}/${MODPATH}" >/dev/null
-            "${golangci[@]}" --path-prefix "${MODPATH}" ./... >&2 || res=$?
+        "${golangci[@]}" --path-prefix "${MODPATH}" ./... >&2 || res=$?
-        popd >/dev/null
+      popd >/dev/null
     done
+  fi
+}
+# First run with normal output.
+run "${targets[@]}"
+
+# Then optionally do it again with github-actions as format.
+# Because golangci-lint caches results, this is faster than the
+# initial invocation.
+if [ "$githubactions" ]; then
+  golangci+=(--out-format=github-actions)
+  run "$${targets[@]}" >"$githubactions" 2>&1
 fi
 
 # print a message based on the result
@@ -122,6 +172,11 @@ 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 '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.'
+    fi
     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/'