Audit golangci-lint config

[sbueringer]

Let's compare the golangci-lint config with the one from core CAPI and see if we should add more linters / config etc. (also potentially in core CAPI).

Some observations:

• I saw a few places where we didn't handle returned errors in CAPV. IIRC one of the linters enabled in core CAPI is catching that (not 100% sure). (we probably also need a corresponding exclude)
• We should configure the importas linter for at least the most important imports to get some consistency

Tasks

Beta Give feedback

1. enable configuration for importas (@chrischdi ) 🌱 lint: fix import aliases via importas linter #2231
2. enable linter containedctx (@zhanggbj) 🌱 lint: enable containedctx lint #2297
3. enable linter dupword (@MrDXY) 🌱 lint: fix duplicated words via dupword linter #2260
4. enable linter errchkjson (@MrDXY) 🌱 lint: fix the JSON encoding check error via errchkjson linter #2273
5. enable linter ginkgolinter (@nunnatsa) 🌱 lint: enable ginkgolinter and fix findings #2282
6. enable linter nolintlint (@MrDXY) 🌱 lint: enable nolintlint and fix findings #2294
7. enable configuration for gocritic (@MrDXY) 🌱 lint: enable gocritic configuration and fix findings #2315
8. remove apis/v1alpha3 from skip-files: (@adityabhatia) 🌱 lint: remove skip-files for api/v1alpha3 #2378
9. remove all additional exclusions below FIXME: All below excludes should get removed over time. 🌱 Remove lint ignore for comments and unhandled errors #2383
10. do a final diff

[chrischdi]

/assign

[chrischdi]

Let's tackle this in multiple steps:

1. One PR which sync's the .golangci.yml file to the core CAPI version.

   • When doing this, let's keep linters turned off (commented out) and enable them in follow-up PRs
   • Same applies for linter configuration
   • 🌱 Sync golangci-lint configuration to core CAPI #2173

2. Enable new Linters and enable configuration in multiple follow-up PRs. Doing this in a single PR would require too many code changes and would make it very hard for reviewers.

EDIT: moved this list to the top / initial comment as task list.

[lubronzhan]

I was debugging my PR and found out that it doesn't print any lines and code for specific lint error.

Found out here it could be fixed by --out-format=colored-line-number

Verified here https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/actions/runs/5791075333/job/15695267905?pr=2171
Gonna add another PR for that

[sbueringer]

@chrischdi Let's add a final step to diff against the core CAPI file again (just to make sure we didn't miss anything that we would want to have sync)

[chrischdi]

Update the above list and the initial MR

[chrischdi]

I already have a PR prepared as follow-up to enable the importas configuration.

[sbueringer]

Feel free to move the sub-task list in the issue description itself in case you think it's better (fine either way for me)

[chrischdi]

@zhanggbj @laozc : the first two MRs got merged 🎉

Feel free to pick up work from the leftover tasks above :-)

[zhanggbj]

I'll start with this one :)

• containedctx 🌱 lint: enable containedctx lint #2297

[sbueringer]

/assign @zhanggbj

[sbueringer]

We can assign multiple people, just pick a linter and let us know :)

/help

[k8s-ci-robot]

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

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

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

In response to this:

We can assign multiple people, just pick a linter and let us know :)

/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.

[MrDXY]

I'll go with this one: 😊

• dupword

[zhanggbj]

/assign @MrDXY

[MrDXY]

Hi, folks, I created another pr to track the following task. #2273

• enable linter errchkjson

[nunnatsa]

Hi.

Just created a new PR to enable ginkgolinter; See #2282

[MrDXY]

Hi team, I will continue with nolintlint. 😄

[MrDXY]

Hi folks, I will proceed with enabling the configuration for gocritic. 😄

[adityabhatia]

/assign

[adityabhatia]

I will take this - remove apis/v1alpha3 from skip-files:

[chrischdi]

Took up some parts of

remove all additional exclusions below FIXME: All below excludes should get removed over time. (Multiple PRs

in:

• 🌱 lint: remove linter ignores from .golangci-lint for gosec #2381

[killianmuldoon]

I'm going to have a go at adding comments across the code under FIXME

[killianmuldoon]

I ran a diff between CAPV and CAPI golanglint-ci.yaml config files and there's nothing unexpected there. There's a tiny update to formatting in the CAPI file that will make diffing slightly easier - I'll raise that one in a PR.

Thanks everyone for your contributions!

@chrischdi I think we should close this if your happy with it - there's some follow-ups in #2384.

[killianmuldoon]

/close

[k8s-ci-robot]

@killianmuldoon: Closing this issue.

In response to this:

/close

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.