golangci-lint: suppress one issue, demote others to "hints" #118502

pohly · 2023-06-06T11:32:48Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The voting in #117288 led to one check that got rejected ("ifElseChain: rewrite if-else to switch statement") and several that are "nice to know".

golangci-lint's support for issue "severity" is too limited to identify "nice to know" issues in the output (filtering is only by linter without considering the issue text; not part of text output). Therefore a third configuration gets added which emits all issues (must fix and nits). The intention is to use the "strict" configuration in pull-kubernetes-verify and the "hints" configuration in a new non-blocking pull-kubernetes-linter-hints.

That way, "must fix" issues will block merging while issues that may be useful will show up in a failed optional job. However, that job then also contains "must fix" issues, partly because filtering out those would make the configuration a lot larger and is likely to be unreliably (all "must fix" issues would need to be identified and listed), partly because it may be useful to have all issues in one place.

Which issue(s) this PR fixes:

Fixes #117288

Special notes for your reviewer:

The previous approach of manually keeping two configs in sync with special comments didn't scale to three configs. Now a single golangci.yaml.in with text/template constructs contains the source for all three configs. A new simple CLI frontend for text/template (cmd/gotemplate) is used by hack/update-golangci-lint-config.sh to generate the three flavors.

Verification uses the same approach as hack/verify-codegen.sh (ensure working tree is clean, update, check for a diff). Because the logic is the same and only the update command is different, the common logic gets moved into hack/lib/verify-any.sh.

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2023-06-06T11:32:57Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pohly · 2023-06-06T12:00:47Z

/test pull-kubernetes-verify-strict-lint

pohly · 2023-06-06T14:02:24Z

/test pull-kubernetes-verify-strict-lint

We have consensus on how the different known issues should be handled. kubernetes/kubernetes#118502 implements that. Before go towards the final goal of making strict linting merge blocking by moving it into pull-kubernetes-verify, let's expose more developers to this new check by running it for all PRs. Hints are issues that may or may not need fixing. To make those visible to developers and reviewers, another, permanently non-blocking job is needed which runs checks for those -> pull-kubernetes-linter-hints.

cmd/gotemplate/OWNERS

pohly · 2023-06-06T14:24:10Z

cmd/gotemplate/gotemplate.go

+//
+// Besides the default functions (https://pkg.go.dev/text/template#hdr-Functions),
+// gotemplate also implements:
+// - include <filename>: returns the content of that file as string


I'll use that to include the content of logcheck.conf, once golangci-lint supports that. The advantage will be that manually invalidating the golangci-lint cache when changing logcheck.conf isn't going to be needed anymore.

See #119860 for that usage.

pohly · 2023-06-06T14:26:07Z

hack/golangci-strict.yaml

 issues:
+  max-issues-per-linter: 0


In theory, a lot more issues might get reported now, although it should still be rare. If it occurs, developers will probably expect to get a complete report.

I found it highly confusing in local testing when issues were truncated at the default of 50 issues per linter.

pohly · 2023-06-06T14:26:30Z

hack/golangci-strict.yaml


 run:
  timeout: 30m
  skip-files:
    - "^zz_generated.*"

+output:
+  sort-results: true


Makes it possible to compare before/after reports.... ordering was random before.

dims · 2023-06-06T14:34:20Z

/assign @BenTheElder

thockin

Having a whole new tool to run gotemplates, which are pretty universally hated, seems unfortunate. Do we have alternatives? We probably don't want to demand that people have jinja install (or something), right? Can we do this with sed or awk?

hack/lib/verify-any.sh

thockin · 2023-06-19T17:22:49Z

hack/lib/verify-any.sh

+# This sets up the environment, like GOCACHE, which keeps the worktree cleaner.
+kube::golang::setup_env
+
+_tmpdir="$(kube::realpath "$(mktemp -d -t "$(basename "$0").XXXXXX")")"


What is $0 here - is it "verify-any.sh"? That's not super useful. Maybe you should check that $1 is not empty and use that here instead?

In the new function, it is now "verify-generated-$(basename "$1" | sed -e 's/[^a-zA-Z][^a-zA-Z]*/-/g').XXXXXX")".

$1 cannot be empty, it must be a function or command that is to be executed.

thockin · 2023-06-19T17:30:02Z

hack/lib/verify-any.sh

I don't love the name of this file (or that it is a file, really). We have a ton of verify* files, this is not like them. Since this is really a utility, why not turn it into function (run_verify ?) in hack/lib and call it from places like codegen (and mocks, and internal-modules, and licenses, and yamlfmt...)

That could be a commit of its own, just removing that duplication from those verify-* scripts.

A function should also work. I'll change it.

Done. Please take another look.

thockin · 2023-06-19T17:35:24Z

hack/update-golangci-lint-config.sh

+
+# Inform about differences.
+echo
+diff -c hack/golangci.yaml hack/golangci-strict.yaml || true


If we are going to diff, can we use -u ?

Changed. Here's what an invocation now looks like:

$ hack/update-golangci-lint-config.sh +++ [0621 08:45:09] Setting GOMAXPROCS: 36 Generating hack/golangci.yaml from hack/golangci.yaml.in with ./cmd/gotemplate Base=1 Generating hack/golangci-strict.yaml from hack/golangci.yaml.in with ./cmd/gotemplate Strict=1 Generating hack/golangci-hints.yaml from hack/golangci.yaml.in with ./cmd/gotemplate Hints=1 --- hack/golangci.yaml 2023-06-21 08:45:10.222186135 +0200 +++ hack/golangci-strict.yaml 2023-06-21 08:45:10.478185292 +0200 @@ -30,11 +30,6 @@ - path: conversion\.go linters: - ineffassign - # TODO(oscr) 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/*) - linters: - - gocritic # The Kubernetes naming convention for conversion functions uses underscores # and intentionally deviates from normal Go conventions to make those function @@ -88,7 +83,7 @@ text: "ST1023: should omit type .* from declaration; it will be inferred from the right-hand side" linters: - disable-all: true + disable-all: false enable: # please keep this alphabetized - ginkgolinter - gocritic @@ -98,6 +93,9 @@ - staticcheck - stylecheck - unused + disable: + # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507008359 + - errcheck linters-settings: # please keep this alphabetized custom: @@ -106,15 +104,6 @@ path: ../_output/local/bin/logcheck.so description: structured logging checker original-url: k8s.io/logtools/logcheck - gocritic: - enabled-checks: - - equalFold - - boolExprSimplify staticcheck: checks: - "all" - - "-SA1019" # TODO(fix) Using a deprecated function, variable, constant or field - - "-SA2002" # TODO(fix) Called testing.T.FailNow or SkipNow in a goroutine, which isn’t allowed - stylecheck: - checks: - - "ST1019" # Importing the same package multiple times --- hack/golangci-strict.yaml 2023-06-21 08:45:10.478185292 +0200 +++ hack/golangci-hints.yaml 2023-06-21 08:45:10.734184449 +0200 @@ -46,42 +46,6 @@ - gocritic text: "ifElseChain: rewrite if-else to switch statement" - # 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" - - # 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 @@ -93,9 +57,6 @@ - staticcheck - stylecheck - unused - disable: - # https://github.com/kubernetes/kubernetes/issues/117288#issuecomment-1507008359 - - errcheck linters-settings: # please keep this alphabetized custom:

Is that perhaps too verbose?

Diff is removed now.

pohly · 2023-06-19T17:55:31Z

Can we do this with sed or awk?

I doubt that it would be nice. How about a golangci-lint.in.sh shell script that is one giant

cat <<EOF
# 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.
...
$(if $base; then cat <<EOF2
# some code only for the base config
EOF2))
...
EOF

Would that be better?

thockin · 2023-06-19T18:06:32Z

I don't think multiple embedded heredocs are better, no :)

How about we run it through cpp

# 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.
...
#ifdef BASE
# some code only for the base config
#endif
...

I am kidding but only barely.

pohly · 2023-06-19T19:46:12Z

I don't think multiple embedded heredocs are better, no :)

They could get avoided by embedding calls to functions which then contain those heredocs, but that would break the flow of the config.

How about we run it through cpp

We can't assume that cpp is in the PATH, though, can we?

pohly · 2023-06-20T05:43:59Z

cpp is not capable enough: golangci-lint 1.16.x will support embedding the logcheck.conf inside the golangci-lint.yaml. But having that as a separate file is useful, so I want to include it in the template. That depends on indenting it, which cpp cannot do.

pohly · 2023-07-06T13:49:05Z

/sig testing

thockin

Comments on first commit. I noticed there's one more use of worktree in hack that you didn't capture, but it's not a "verify + update" pair.

We can do this as a followup (or not) but something like:

function run_in_worktree() {
    wtpfx="$1"
    shift

    (  # new sub-shell
        _tmpdir="$(mktemp -d -t "${wtpfx}-$(basename "$1").XXXXXX")"
        git worktree add -f -q "${_tmpdir}" HEAD
        cd "${_tmpdir}"
                                                                                                                                                                         
        "$@"
    )
}

function verify_generated() {
    run_in_worktree "verify_generated" "$@"
}

Then the hack/verify-licenses.sh script can call run_in_worktree against a function.

hack/lib/verify-generated.sh

thockin · 2023-07-14T16:49:35Z

hack/lib/verify-generated.sh

+    # This sets up the environment, like GOCACHE, which keeps the worktree cleaner.
+    kube::golang::setup_env
+
+    _tmpdir="$(kube::realpath "$(mktemp -d -t "verify-generated-$(basename "$1" | sed -e 's/[^a-zA-Z][^a-zA-Z]*/-/g').XXXXXX")")"


What is with this sed? What is it trying to accomplish? Was there a motivation for it?

No particular reason as far as I remember. I just wanted to ensure that the verify-generated- prefix is followed by something using the same convention (alpha, hyphen as separator). Probably not worth it or even confusing when trying to map the temp dir back to the original command - I've removed the sed.

thockin

I wonder if we should move tools like this under hack/tools or something? Not a problem for this PR.

cmd/gotemplate/OWNERS

thockin · 2023-07-14T17:14:42Z

cmd/gotemplate/gotemplate.go

+
+func main() {
+	kvs := make(map[string]string)
+	flag.Parse()


do we need to call this if we have no flags?

Not really. It adds -h, but that doesn't print anything useful. I've removed flag.

thockin · 2023-07-14T17:31:12Z

hack/update-golangci-lint-config.sh

+generate hack/golangci-hints.yaml Hints=1
+
+# Inform about differences.
+echo


I'm not sure we need this output every run. Maybe just print the commands the user can run to see them?

My intention was to have that output in the Prow job output when someone reviews a PR which changes the template. Checking out the PR and running diff locally also works. Removed.

pohly · 2023-08-02T10:59:35Z

Then the hack/verify-licenses.sh script can call run_in_worktree against a function.

Is that really worth the churn? As far as I can tell at a glance, the indention of most of verify-licenses.sh would change when moving the current top-level code into a function.

pohly · 2023-08-02T11:55:44Z

I wonder if we should move tools like this under hack/tools or something?

That would make running them harder when inside the main Kubernetes repo:

$ git mv cmd/gotemplate hack/tools/
$ go run ./hack/tools/gotemplate
main module (k8s.io/kubernetes) does not contain package k8s.io/kubernetes/hack/tools/gotemplate

pohly · 2023-08-02T11:56:15Z

/test pull-kubernetes-verify-strict-lint

pohly · 2023-08-02T13:32:19Z

I pushed my updates.

However, I then had to rebase to include some config change from master in the template.

pohly · 2023-08-02T15:00:27Z

/test pull-kubernetes-verify-strict-lint

thockin

LGTM overall

thockin · 2023-08-22T16:40:40Z

hack/lib/verify-generated.sh

+# in the work tree.
+#
+# Example: kube::verify::generated "Mock files are out of date" "Please run 'hack/update-mocks.sh'" hack/update-mocks.sh
+kube::verify::generated() (


This is tricky syntax. It surprised even me. Can I arm-twist you into something more obvious:

kube::verify::generated() { ( # a subshell prevents environment changes from leaking out of this function local failure_header=$1 shift # etc. ) }

Sure, if that's preferred. Defining commands as a single sub-shell is how I learned it, but I guess it's not that common in Kubernetes.

Updated.

Several verify scripts used the same pattern of "check for clean working tree, generated files, check for diffs". The code for that is now in kube::verify::generated, defined in hack/lib/verify-generated.sh, and those scripts just source that.

The voting in kubernetes#117288 led to one check that got rejected ("ifElseChain: rewrite if-else to switch statement") and several that are "nice to know". golangci-lint's support for issue "severity" is too limited to identify "nice to know" issues in the output (filtering is only by linter without considering the issue text; not part of text output). Therefore a third configuration gets added which emits all issues (must fix and nits). The intention is to use the "strict" configuration in pull-kubernetes-verify and the "hints" configuration in a new non-blocking pull-kubernetes-linter-hints. That way, "must fix" issues will block merging while issues that may be useful will show up in a failed optional job. However, that job then also contains "must fix" issues, partly because filtering out those would make the configuration a lot larger and is likely to be unreliably (all "must fix" issues would need to be identified and listed), partly because it may be useful to have all issues in one place. The previous approach of manually keeping two configs in sync with special comments didn't scale to three configs. Now a single golangci.yaml.in with text/template constructs contains the source for all three configs. A new simple CLI frontend for text/template (cmd/gotemplate) is used by hack/update-golangci-lint-config.sh to generate the three flavors.

thockin · 2023-08-22T19:39:23Z

Thanks!

/lgtm
/approve
/retest

k8s-ci-robot · 2023-08-22T19:39:30Z

LGTM label has been added.

Git tree hash: 4cda3d4498b721c69167b4cf15b003279e29fe88

k8s-ci-robot · 2023-08-22T19:44:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/OWNERS~~ [thockin]
~~hack/OWNERS~~ [pohly,thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

We have consensus on how the different known issues should be handled. kubernetes/kubernetes#118502 implements that. Before go towards the final goal of making strict linting merge blocking by moving it into pull-kubernetes-verify, let's expose more developers to this new check by running it for all PRs. Hints are issues that may or may not need fixing. To make those visible to developers and reviewers, another, permanently non-blocking job is needed which runs checks for those -> pull-kubernetes-linter-hints.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 6, 2023

k8s-ci-robot requested review from justaugustus and wojtek-t June 6, 2023 11:34

pohly force-pushed the golangci-lint-hints branch from 88908b3 to 3a8762a Compare June 6, 2023 12:34

pohly changed the title ~~WIP: golangci-lint: suppress one issue, demote others to "hints"~~ golangci-lint: suppress one issue, demote others to "hints" Jun 6, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 6, 2023

pohly force-pushed the golangci-lint-hints branch from 3a8762a to 03c7a3a Compare June 6, 2023 14:10

pohly mentioned this pull request Jun 6, 2023

testing: always run pull-kubernetes-verify-strict-lint, add linter hints kubernetes/test-infra#29702

Merged

pohly commented Jun 6, 2023

View reviewed changes

k8s-ci-robot assigned BenTheElder Jun 6, 2023

thockin reviewed Jun 19, 2023

View reviewed changes

pohly force-pushed the golangci-lint-hints branch 2 times, most recently from a3ed4b1 to dba7fb1 Compare June 21, 2023 08:09

k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 6, 2023

thockin reviewed Jul 14, 2023

View reviewed changes

pohly mentioned this pull request Aug 1, 2023

logcheck: support new golangci-lint plugin interface kubernetes-sigs/logtools#22

Closed

pohly force-pushed the golangci-lint-hints branch from dba7fb1 to d6da30e Compare August 2, 2023 11:55

pohly force-pushed the golangci-lint-hints branch from d6da30e to 0be4998 Compare August 2, 2023 13:32

pohly mentioned this pull request Aug 9, 2023

golangci-lint update and support for Go 1.21 #119860

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2023

pohly force-pushed the golangci-lint-hints branch from 0be4998 to 400b6db Compare August 22, 2023 13:53

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2023

pohly force-pushed the golangci-lint-hints branch from 400b6db to 141885f Compare August 22, 2023 15:18

thockin reviewed Aug 22, 2023

View reviewed changes

pohly added 2 commits August 22, 2023 20:39

pohly force-pushed the golangci-lint-hints branch from 141885f to ce9e668 Compare August 22, 2023 18:40

k8s-ci-robot assigned thockin Aug 22, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2023

k8s-ci-robot merged commit a5ebc9d into kubernetes:master Aug 22, 2023
12 checks passed

k8s-ci-robot added this to the v1.29 milestone Aug 22, 2023

golangci-lint: suppress one issue, demote others to "hints" #118502

golangci-lint: suppress one issue, demote others to "hints" #118502

Conversation

pohly commented Jun 6, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Jun 6, 2023

pohly commented Jun 6, 2023

pohly commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Jun 6, 2023

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Jun 19, 2023

thockin commented Jun 19, 2023

pohly commented Jun 19, 2023

pohly commented Jun 20, 2023

pohly commented Jul 6, 2023

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Aug 2, 2023

pohly commented Aug 2, 2023

pohly commented Aug 2, 2023

pohly commented Aug 2, 2023

pohly commented Aug 2, 2023

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 22, 2023

k8s-ci-robot commented Aug 22, 2023

k8s-ci-robot commented Aug 22, 2023