Golangci-linters - what we should have enabled?

[tuminoid]

In Metal3, golangci-linter works by disabling all linters, and then white-listing the ones that pass.

We have couple issues about enabling more golangci-linters. We have 40+ in CAPM3 and BMO, 30+ in IPAM and 30+ now in IRSO.

• Increase number of enabled golangci-lint linters ip-address-manager#752
• Increase number of enabled golangci-lint linters  cluster-api-provider-metal3#2106
• Increase number of enabled golangci-lint linters  #2072
• IRSO TBD

There are 50 golangci-linters available white enable-all: true, with 100+ in total. We should have a discussion which ones we aim to enable, and then achieve that in all repositories under Metal3. We should also look at the exceptions we've accumulated over the years, and decide if they should be dropped, and if not, document them better.

Based on the results of this discussion, I will adjust the linked issues and create new ones as needed.

[lentzi90]

If we are anyway at ~40 % enabled, how about we switch to enable all and then disable those that fail? Then it will be more obvious what we have disabled and we can create specific issues for fixing and enabling those that we care about.

[tuminoid]

I tried this, and actually it says there are 50 enabled when using enable-all: true in the config. 100+ is mentioned in golangci-lint, so the rest must be manually white-listed to enable. I revised the expectation to the 50 linters, not 100+.

Gist with output from enable-all: true: https://gist.github.com/tuminoid/734f11b6ffc7912e729458e2e6acbf86

In CAPM3, we fail:

• dupl
• funlen
• gochecknoinits
• godox
• gofumpt
• gomoddirectives
• lll
• nestif
• depguard
• forcetypeassert
• gomnd
• inamedparam
• interfacebloat
• intrange
• nlreturn
• testpackage
• wsl

I think we fail all that we have not enabled. I agree we should make it explicit in the config. Some of the errors seem fixable, some are quite intrusive and not productive.

[elfosardo]

@lentzi90 I actually went for the opposite in IrSO in metal3-io/ironic-standalone-operator#94 but I agree with that, I'm still in time to change it in case we want to experiment there as the lint test is not active yet

[tuminoid]

IRSO implementation was OK as is. We just need to enable the rest of the linters, and then add some more based on what we agree here. We did take a look at the failure list, and many of them are fixable, but some of them like wsl go way overboard.

We're not in hurry here, so I'll take time at some point to dig into each failing one, and suggest enabling/keeping disabled for each of them, with preferred fix suggestions, to decide.

[tuminoid]

@dtantsur did some checking in metal3-io/ironic-standalone-operator#184 and came up with a list of 68 linters that are worthy in his opinion. We could use this as a base what we want to include in each repo and use the same approach as the PR does - list all the linters we want to enable, and just comment out the ones that require further fixing in the code.

I don't want to have this discussion forever open, so can you @adilGhaffarDev @kashifest @lentzi90 @elfosardo PTAL the list in the PR, and either agree or disagree on the list.

[lentzi90]

The list looks good to me! I expect that we will have some issues enabling all of them in CAPM3/BMO but at least we can strive to do it 🙂

[kashifest]

Lgtm, I would be happy to help enabling them.

[adilGhaffarDev]

LGTM. But can we do it in the next release cycle? It might need some work in capm3, and I don't want to delay this release.

[tuminoid]

We don't need to enable them right now, all we need to achieve here is to agree on the list.

I will create PRs to add the agreed configuration to linter config, and leave any linter that is not clean right now commented out, just like we have in IRSO.

This way we can address them one by one. We already have an issue per repo about this, so I can make subtasks per linter and mark them for help-wanted. This way we also get bunch of issues that are small in size to lure in contributors.

[tuminoid]

After confirming in community meeting that this is fine with everybody, I have created PRs to sync the configs and created issues for fixing the rest of the missing linters in all of the four repos this touches.

Thank you for the discussion, I will close this thread as completed.