diff --git a/hack/golangci-strict.yaml b/hack/golangci-strict.yaml new file mode 100644 index 00000000000..8c73016649e --- /dev/null +++ b/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", + ] diff --git a/hack/make-rules/verify.sh b/hack/make-rules/verify.sh index 41ab4adcb7f..1d5556ac8a2 100755 --- a/hack/make-rules/verify.sh +++ b/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 ) diff --git a/hack/verify-golangci-lint-pr.sh b/hack/verify-golangci-lint-pr.sh new file mode 100755 index 00000000000..a3491585c28 --- /dev/null +++ b/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 diff --git a/hack/verify-golangci-lint.sh b/hack/verify-golangci-lint.sh index d2b785da1a3..5d4925ea899 100755 --- a/hack/verify-golangci-lint.sh +++ b/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 -# Kubernetes package by golint. +# This script checks the coding style for the Go language files using +# 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 <&2 -Usage: $0 [-- ] [packages]" +Usage: $0 [-r |-a] [-s] [-c none|] [-- ] [packages]" + -r : 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 : 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 [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}")" - echo "running ( cd ${KUBE_ROOT}/${MODPATH}; ${golangci[*]} --path-prefix ${MODPATH} ./... )" - pushd "${KUBE_ROOT}/${MODPATH}" >/dev/null - "${golangci[@]}" --path-prefix "${MODPATH}" ./... >&2 || res=$? - popd >/dev/null + MODPATH="staging/src/k8s.io/$(basename "${d}")" + echo "running ( cd ${KUBE_ROOT}/${MODPATH}; ${golangci[*]} --path-prefix ${MODPATH} ./... )" + pushd "${KUBE_ROOT}/${MODPATH}" >/dev/null + "${golangci[@]}" --path-prefix "${MODPATH}" ./... >&2 || res=$? + 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/'