Investigate if we should enable additional linters (through golangci-lint)

[sbueringer]

User Story

As a reviewer I would like to focus on reviewing the most relevant changes in PRs and not too much on nitpicking.

Detailed Description

The amount of cleanup PRs we get (although they are great!) suggests that we have a few gaps in our linting.
I think we should investigate if there are some additional linters we could easily enable via golangci-lint (https://golangci-lint.run/usage/linters/).

Anything else you would like to add:

As matters of code style can be very contentious I would suggest the following process:

• Identify linters we could potentially use to automate frequent review findings
• Discuss here on this issue which ones we want to enable
• Open PRs to fix the findings and enable the linters

xref: #4607 (comment)

Tasks:

• fix exclude bug: ⚠️ Refactor golangci-lint config to remove false negatives #4657
• move excludes to exclude-rules: ⚠️ Refactor golangci-lint config to remove false negatives #4657 (comment) will be fixed by 🌱 deduplicate linters, move exclude to exclude-rules #4804
• fix linter findings and enable new linters (xref: 🌱 Enable excluded lint checks #4624 (comment))
  • enable exportloopref, ifshort, nilerr 🌱 Enable exportloopref, ifshort, and nilerr linters #4649
  • enable importas Investigate if we should enable additional linters (through golangci-lint) #4622 (comment)
• fix linter findings and remove issue excludes (xref: https://github.com/stmcginnis/cluster-api/blob/6a24e15c523d083739e2d605249457c212f92a33/.golangci.yml#L49-L55)
  • Subprocess launch(ed with variable|ing should be audited): 🌱 explicitly ignore command injection (in tests) #5017
  • (Expect directory permissions to be 0750 or less|Expect
  • G104: 🐛 fix unhandled errors #4895
  • G307: will be handled via Failures in deferred calls not captured #4586
  • exported (method|function|type|const) (.+) should have comment or be unexported

/kind feature

[sbueringer]

@enxebre @fabriziopandini @vincepri If this sounds reasonable to you, I would take a look if I can find some useful (and non-contentious) linters.

[stmcginnis]

Here's a list of golangci-lint based linters I've used on other projects. For the most part I think they wouldn't be too contentious, though there may be a couple (certain gocritic cases) where we may want to tweak some of the specific linter settings.

linters:
  disable-all: true
  enable:
    - bodyclose
    - deadcode
    - depguard
    - dogsled
    - dupl
    - errcheck
    - funlen
    - goconst
    - gocritic
    - gocyclo
    - gofmt
    - goheader
    - goimports
    - golint
    - gomnd
    - goprintffuncname
    - gosec
    - gosimple
    - govet
    - ineffassign
    - maligned
    - misspell
    - nakedret
    - noctx
    - nolintlint
    - rowserrcheck
    - staticcheck
    - structcheck
    - stylecheck
    - typecheck
    - unconvert
    - unparam
    - unused
    - varcheck
    - whitespace

Happy to help in this effort too, if you need it.

[sbueringer]

@stmcginnis You can just take it over if you want :). Depending on how much open linter findings we have to fix then we should/could probably create a list and flag the issue with help wanted? (once we have a list with non-contentious ones)

[stmcginnis]

Sounds good @sbueringer! I'll do some local checking and see how badly that list complains, then share that here.

/assign

[stmcginnis]

To get the discussion started I've submitted #4624

That has a list of other checks we can enable that others can comment on if they feel they are contentious or not.

Biggest impact from the ones not included in that change are two related to docstrings: whether packages have godocs (maybe contentious) and whether doc string matches function names (which triggered this discussion in the first place, so hopefully not contentious).

[sbueringer]

@stmcginnis @fabriziopandini @vincepri I adjusted the issue description above to reflect the open tasks. I would suggest adding the help wanted label, wdyt?

[stmcginnis]

Sounds good. Working on the next step now, but would be good to capture more that we want to do under this issue and see if we want to crowdsource knocking those out.

[sbueringer]

@stmcginnis Ah, alright. Let's just try to make clear which parts are already in progress and which might be available for crowdsourcing (in case there are any - your choice). Let me know if I should adjust the issue description in any way.

[sbueringer]

Another suggestion. There's also an importas linter. This can be used to enforce that certain packages are always imported with the "correct" name. Imho this makes the code easier readable (e.g. because you know that you always use corev1 to import a certain package etc...) and it also avoids duplicate imports of those packages.

If we want to use it, I would restrict it to certain very frequently imported packages. Here is an example config:

linters-settings:
  importas:
    apierrors: k8s.io/apimachinery/pkg/api/errors
    appsv1: k8s.io/api/apps/v1
    authorizationv1: k8s.io/api/authorization/v1
    batchv1: k8s.io/api/batch/v1
    corev1: k8s.io/api/core/v1
    metav1: k8s.io/apimachinery/pkg/apis/meta/v1
    rbacv1: k8s.io/api/rbac/v1

I think this would be probably the list I would use, plus a few CAPI specific packages.

[sbueringer]

Short update. Looks like we're making good progress, very nice!

After #4804 we only have to fix the remaining excludes and we're done:

• G204 Subprocess launch(ed with variable|ing should be audited)
• (Expect directory permissions to be 0750 or less|Expect
• G104: 🐛 fix unhandled errors #4895
• G307: will be handled via Failures in deferred calls not captured #4586

[vincepri]

/milestone v0.4

[vincepri]

/help

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

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.

[sbueringer]

Regarding: G204: Subprocess launched with variable (gosec)
We currently have 3 findings and I don't think we shouldn't change our code. I think we have the following options:

• Add nolint to our code
• Permanently ignore the finding via exclude-rules (for linter gosec)
• Permanently ignore the finding via exclude-rules (for linter gosec and only in test/framework)

I would prefer the first option.

(@vincepri)

[fabriziopandini]

As per #4928 (comment) it seems that we are not linting comments on the APIs

[sbueringer]

As per #4928 (comment) it seems that we are not linting comments on the APIs

@fabriziopandini Looks like revive does not enforce comments on exported fields (but on types and funcs): https://github.com/mgechev/revive/blob/master/rule/exported.go

I think the linter is compliant to CodeReviewComments:

All top-level, exported names should have doc comments, as should non-trivial unexported type or function declarations. See https://golang.org/doc/effective_go.html#commentary for more information about commentary conventions.
https://github.com/golang/go/wiki/CodeReviewComments#doc-comments

In this case it wasn't a top-level name as it's a field.

I also couldn't find another linter (supported by golangci-lint) which does this. So short of writing a linter ourselves I don't see a good solution here (and that's probably (?) not worth it).

[sbueringer]

As per #4928 (comment) it seems that we are not linting comments on the APIs

@fabriziopandini Looks like revive does not enforce comments on exported fields (but on types and funcs): https://github.com/mgechev/revive/blob/master/rule/exported.go

I think the linter is compliant to CodeReviewComments:

All top-level, exported names should have doc comments, as should non-trivial unexported type or function declarations. See https://golang.org/doc/effective_go.html#commentary for more information about commentary conventions.
https://github.com/golang/go/wiki/CodeReviewComments#doc-comments

In this case it wasn't a top-level name as it's a field.

I also couldn't find another linter (supported by golangci-lint) which does this. So short of writing a linter ourselves I don't see a good solution here (and that's probably (?) not worth it).

@fabriziopandini In case you want to follow-up on the comment linting, I think we should create a new issue for that. (just because the by far most part of the current issue has been implemented and I don't see a straightforward way to implement comment-linting on fields)