verify-golangci-lint.sh: support stricter checking in new code #109728
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
release-note-none
|
Please note that we're already in Test Freeze for the |
k8s-ci-robot
added
size/L
|
It is instructive to look at issues reported for recently added code, for example everything since v1.24.0-alpha.1. Code quality definitely could be better, and IMHO golangci-lint does a good job with pointing out relevant issues: |
pohly
commented
Apr 29, 2022
hack/verify-golangci-lint.sh
Outdated
| 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 .golangci-strict.yaml as part of your PR.' |
There was a problem hiding this comment.
This is up for debate. I think it will be important to document the escalation path here, but it doesn't have to be this way.
The elephant in the room is: who has the authority to decide about the checks that new code has to pass?
There was a problem hiding this comment.
hack/ OWNERS, who will also be approving the script itself and other similar lints?
There was a problem hiding this comment.
That looks like a set of individuals, not any particular SIG. That's fine with me, I was really just wondering whether there is a SIG which is responsible for code quality questions.
Should .golangci-strict.yaml then be under hack, too? .golangci.yaml was placed in the root because that is where golangci-lint found it when walking up the stack, but the script doesn't rely on that anymore.
There was a problem hiding this comment.
Oh I see! I think code organization is the closest thing at the moment as a specific subset of architecture, but typically tooling for code quality would be sig testing or more narrow like the logging linter.
I am a proponent of not dumping everything in the repo root both for ease of approvals and for making things less cluttered to discover ... The only counterpoint I can think of is if people try to run outside of our scripts, but that already doesn't make much sense for the non-default config. (It also probably doesn't work well anyhow for the other one)
There was a problem hiding this comment.
Fully agreed. It also doesn't need to be a dot file anymore... will change it.
There was a problem hiding this comment.
IIRC we had to add it to the top because there was to be additional executions of the golangci-lint process so we can lint the modules too
There was a problem hiding this comment.
When using the --config parameter, the file can be anywhere.
So perhaps I should also move .golangci.yaml, unless someone can think of a reason why it needs to be in the root?
There was a problem hiding this comment.
I don't see any problem if it works for the vendored modules, @thockin played with it a bit more
There was a problem hiding this comment.
The file gets moved as part of #116166.
|
/sig testing I'm +1 on make more use of the linters /assign @wojtek-t @thockin @dims @BenTheElder |
k8s-ci-robot
added
the
sig/testing
k8s-ci-robot
added
sig/architecture
|
Are there reasons for or against running this linting in a GitHub action? A GitHub action might be simplest because it already has permission to publish comments. I don't know whether Prow jobs can do that. But it would be different from the rest of the tests and we'd rely on compute resources provided by GitHub. |
|
I have added a Kubernetes specific linter plugin for #109600. It's just a proot-of-concept (needs more tests and additional work) and should be merged separately, but I wanted to demonstrate that regardless of what we think about enabling the default golangci-lint checks, this approach would also be useful for other checks. New |
k8s-ci-robot
added
size/XL
GitHub actions don't work as well for blocking merge. In particular we cannot guarantee that we've tested the merged state prior to merge. Having multiple CI in a repo may add additional confusion for other reasons. But we've done it elsewhere (e.g. in KIND).
We have some jobs with github tokens for commenting (think the triage robot), so it's very possible. In either case securing the token and limiting its usage is problematic. |
I've looked at how the golangci-lint GitHub action works and my conclusion is that we probably would have to fork it if we want it to work with current Kubernetes. It might work better once we use a workspace to build all of Kubernetes, but even then it would be different. My preference is to enable annotations from a Prow job - see also my question on Slack: https://kubernetes.slack.com/archives/C01672LSZL0/p1651495487499899 |
pohly
force-pushed
the
lint-pull-requests
branch
2 times, most recently
from
704ed2d
to
4639f11
Compare
k8s-ci-robot
added
the
needs-rebase
It is useful to check new code with a stricter configuration because we want it to be of higher quality. Code reviews also become easier when reviewers don't need to point out inefficient code manually. What exactly should be enabled is up for debate. The current config uses the golangci-lint defaults plus everything that is enabled explicitly by the normal golangci.yaml, just to be on the safe side.
The long-term goal is that when "make verify" is invoked in pull job, it will also run golangci-lint with the strict configuration and write an $ARTIFACTS/golangci-lint-githubactions.log file with GitHub actions annotations. How to get those published for the GitHub PR is open. When "make verify" is invoked manually or in any other job, the stricter check will be skipped. That works because "PR_NUMBER" is only set for pre-merge jobs (https://github.com/kubernetes/test-infra/blob/master/prow/jobs.md#job-environment-variables). Because strict linting is still new and might not be useful without a better UI (= GitHub annotations), this PR does not fully enable the integration into "make verify". Instead, the new verify-golangci-lint-pr.sh is excluded from it and needs to be run in a separate job.
pohly
force-pushed
the
lint-pull-requests
branch
from
5d3b378
to
04700e9
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Let me also call out that the acfa78b commit which failed pull-kubernetes-verify-strict-lint passed pull-kubernetes-verify, as intended: https://prow.k8s.io/pr-history/?org=kubernetes&repo=kubernetes&pr=109728 |
|
Next steps once this is merged (roughly in this order):
|
BenTheElder
reviewed
Mar 7, 2023
There was a problem hiding this comment.
patrick brought this to the sig testing meeting again today and I think we have a good path forward here:
- test a bit in non-blocking job
- move this into
pull-kubernetes-verifyso we can reuse the golangci-lint cache and keep costs down, while only failing additional linters on net-new code - if we manage to cleanup all old code, we can move linters between the two config files. we don't have a mechanism to enforce that, but since new diffs will have the linter enforced, we can just perform this by first adding the now-always-passing linter to the "non strict" lint and then rotate it out of the stricter lint, when we manually identify that we've reached this point. We can consider filing tracking issues to ask people to work on this.
/lgtm
/approve
Thank you for persevering here.
|
|
||
| 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]" |
There was a problem hiding this comment.
If this gets much more complex I'm going to suggest a go binary but this repo is already full of complicated bash :-)
| - ineffassign | ||
|
|
||
| linters: | ||
| enable: # please keep this alphabetized |
There was a problem hiding this comment.
maybe add a comment about how this should only contain linters not already in the standard yaml?
There was a problem hiding this comment.
(this should not block merging, but I think it might be helpful for future review context)
There was a problem hiding this comment.
Actually, it absolutely should contain those 😅
That way, developers can be sure that verify-golangci-lint.sh -a -s catches all linter issues in their code and won't be surprised later when the "normal" config gets applied to it.
There was a problem hiding this comment.
And when the time comes to integrate it into pull-kubernetes-verify, it'll be enough to run verify-golangci-lint.sh only once, with the strict config.
I had a comment about that in the file at some point, but must have lost it when syncing anew with a modified golangci.yaml - I'll bring that comment back.
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: dca7a342299803ac455668cf9268d9f79f3588e3
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, pohly 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
pohly
added a commit
to pohly/kubernetes
that referenced
this pull request
Mar 8, 2023
kubernetes#109728 added a golangci-strict.yaml where gingkolinter and stylecheck (some recent additions to golangci.yaml) were missing. To prevent such mistakes in the future, lines that are intentionally different get annotated with a comment about golangci-strict.yaml or golangci.yaml. Then a suitable diff command in the new verify-golangci-lint-config.sh checks that only such lines, comments and blank lines are different.
pohly
added a commit
to pohly/kubernetes
that referenced
this pull request
Mar 8, 2023
kubernetes#109728 added a golangci-strict.yaml where gingkolinter and stylecheck (some recent additions to golangci.yaml) were missing. To prevent such mistakes in the future, lines that are intentionally different get annotated with a comment about golangci-strict.yaml or golangci.yaml. Then a suitable diff command in the new verify-golangci-lint-config.sh checks that only such lines, comments and blank lines are different.
rayowang
pushed a commit
to rayowang/kubernetes
that referenced
this pull request
Feb 9, 2024
All wrappers except for ExpectNoError are identical to their gomega
counterparts. The only advantage that they have is that their invocations are
shorter.
That advantage does not outweigh their disadvantages:
- cannot be used in combination with gomega.Eventually/Consistently
- not a full replacement for gomega, so we just end up using both
- don't support passing a stack offset and thus cannot be used in helper
functions
- ginkgolinter does not work for them, so sub-optimal calls like this one
are not reported:
framework.ExpectEqual(len(items), 0)
->
gomega.Expect(items).To(gomega.BeEmpty())
- developers try to make do with what's available in the framework, leading
to sub-optimal checks like this:
framework.ExpectEqual(true, strings.Contains(event.Message, expectedEventError), "Event error should indicate non-root policy caused container to not start")
->
gomega.Expect(event.Message).To(gomega.ContainSubstring(expectedEventError), "Event error should indicate non-root policy caused container to not start")
So let's remove these wrappers. As a first step they get marked as deprecated.
This enables stricter
linting (kubernetes#109728), once enabled,
to report new code which uses them.
rayowang
pushed a commit
to rayowang/kubernetes
that referenced
this pull request
Feb 9, 2024
kubernetes#109728 added a golangci-strict.yaml where gingkolinter and stylecheck (some recent additions to golangci.yaml) were missing. To prevent such mistakes in the future, lines that are intentionally different get annotated with a comment about golangci-strict.yaml or golangci.yaml. Then a suitable diff command in the new verify-golangci-lint-config.sh checks that only such lines, comments and blank lines are different.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it:
It is useful to check new code with a stricter configuration because we want it
to be of higher quality. Code reviews also become easier when reviewers don't
need to point out inefficient code manually.
What exactly should be enabled is up for debate. The current config uses the
golangci-lint defaults plus everything that is enabled explicitly by the normal
.golangci.yaml, just to be on the safe side.
Special notes for your reviewer:
As a first step we could add a non-blocking pull job which runs with the stricter configuration for the new code, using
-r PULL_BASE_REF.kubernetes/test-infra#17056 would have been nice, but hasn't made much progress. Let's move forward by enabling strict linting in an optional job.
Does this PR introduce a user-facing change?