🌱 Update golangci-lint and Go version for golangci-lint workflow

[oscr]

What this PR does / why we need it:

Updates the GitHub workflow to use Go 1.18 and bumps golangci-lint 1.44.0 -> 1.46.2 (latest). Support for Go 1.18 was added in golangci-lint in v1.45.0. This pr also fixes 4 findings due to the upgraded lint version.

Reference sources: https://github.com/golangci/golangci-lint/releases https://github.com/golangci/golangci-lint/pull/2438

Part of #5968 Fixes #6350

[k8s-ci-robot]

Hi @oscr. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[chrischdi]

We need a version which includes https://github.com/kubernetes-sigs/cluster-api/issues/6350#issuecomment-1141383233

/hold

[sbueringer]

@oscr Please add the following to the PR description

Part of #5968 Fixes #6350

(2nd one is true after the next golangci-lint release)

[oscr]

@oscr Please add the following to the PR description

Part of #5968 Fixes #6350

(2nd one is true after the next golangci-lint release)

Sure! I will bump the golangci-lint version as soon as it's available.

[sbueringer]

Perfect. Thank you!

lgtm so far

/ok-to-test

[sbueringer]

@oscr You can rebase once #6943 is merged

[killianmuldoon]

@oscr It would be good to dedupe this with #6943 - would you mind picking up tracking the linter update here as we move toward finally being able to update? 😆

[killianmuldoon]

Summary of learnings from #6943:

v1.47.1 does not work due to performance issues in the revive linter. v1.47.2 disables the problematic linters allowing that version of golangci-lint to be used on CAPI at the cost of some linter coverage.

At this point we could:

• Delay update for now and see if the bug can be fixed in the next couple of weeks
• Update but keep the exclusions in the config so we can roll them back once a fix is out @fabriziopandini WDYT?

[chrischdi]

Relevant for discussing if to delay or not: https://github.com/golangci/golangci-lint/issues/2997#issuecomment-1190681977

List of disabled linters:

• var-declaration (default): https://revive.run/r#var-declaration
• errorf (default): https://revive.run/r#errorf
• unexported-return (default): https://revive.run/r#unexported-return
• time-naming (default): https://revive.run/r#time-naming
• context-keys-type (default): https://revive.run/r#context-keys-type
• modifies-value-receiver: https://revive.run/r#modifies-value-receiver
• range-val-address: https://revive.run/r#range-val-address
• string-of-int: https://revive.run/r#string-of-int
• time-equal: no documentation
• unhandled-error: https://revive.run/r#unhandled-error

[oscr]

I just want to highlight a change. When I cherry picked the changes from #6943 I got some nolintlint findings:

util/defaulting/defaulting.go:28:38: directive `//nolint:revive` is unused for linter "revive" (nolintlint)
type DefaultingValidator interface { //nolint:revive
                                     ^
cmd/clusterctl/client/alpha/rollout_pauser.go:39:92: directive `//nolint:revive // MachineDeployment is intentionally capitalized.` is unused for linter "revive" (nolintlint)
			return errors.Errorf("MachineDeploymet is already paused: %v/%v\n", ref.Kind, ref.Name) //nolint:revive // MachineDeployment is intentionally capitalized.
			                                                                                        ^
cmd/clusterctl/client/alpha/rollout_resumer.go:39:99: directive `//nolint:revive // MachineDeployment is intentionally capitalized.` is unused for linter "revive" (nolintlint)
			return errors.Errorf("MachineDeployment is not currently paused: %v/%v\n", ref.Kind, ref.Name) //nolint:revive // MachineDeployment is intentionally capitalized.
			                                                                                               ^
controlplane/kubeadm/internal/etcd/fake/client.go:26:30: directive `//nolint:revive` is unused for linter "revive" (nolintlint)
type FakeEtcdClient struct { //nolint:revive
                             ^
cmd/clusterctl/internal/test/fake_proxy.go:50:35: directive `//nolint:revive` is unused for linter "revive" (nolintlint)
	FakeScheme = runtime.NewScheme() //nolint:revive

So I temporary allowed unused in nolintlint until revive is fully enabled again.

nolintlint:
    allow-unused: true # TODO(oscr) Set to false when revive linter is fully enabled again.` 

I this ok?

[killianmuldoon]

So I temporary allowed unused in nolintlint until revive is fully enabled again. I this ok?

Yeah - that's perfectly fine I think.

[oscr]

/test pull-cluster-api-e2e-informing-main

[killianmuldoon]

Part of #5968

[oscr]

There are now two new versions of golangci-lint: https://github.com/golangci/golangci-lint/releases/tag/v1.47.2 https://github.com/golangci/golangci-lint/releases/tag/v1.47.3

1.47.2 which just disables the slow rules and 1.47.3 with some upgrades and fixes.

What would we like to do here? I can upgrade, stay or wait whichever works best.

[stmcginnis]

What would we like to do here? I can upgrade, stay or wait whichever works best.

Probably good to update to 1.47.3 at this point. Looks like it picks up some good fixes.

[stmcginnis]

/lgtm

[oscr]

/remove-hold

[killianmuldoon]

golangci-lint 1.48.0 is now released, and it looks like there's not going to be another release for a little while.

I think we should update CAPI to golangci-lint 1.48.0. I ran it locally and there's no big issues IMO - there's a bunch of new fixes, but they're minor formatting things for the most part. We can also bump Go to 1.18 in the golangci-lint job and in the .golangci.yml

We should track the disabled linters to make sure we're not causing too much drift for when we bring the full range of revive linters back with the release of 1.49.

[sbueringer]

Sounds good to me. Additional 1.48 also supports Go 1.19 which unblocks upcoming work for Kubernetes v1.25.0.

[oscr]

I upgraded the golangci-lint 1.48 but there is a problem with the license headers. Unfortunately the new version reports the header as a finding. For example:

cmd/clusterctl/api/v1alpha3/metadata_type_test.go:8: File is not `gofmt`-ed with `-s` (gofmt)
    http://www.apache.org/licenses/LICENSE-2.0

Running make lint-fix changes them and then the verify-boilerplate.sh script fails. Looking at it.

[killianmuldoon]

Running make lint-fix changes them and then the verify-boilerplate.sh script fails. Looking at it.

I've also been trying to pin this down on https://github.com/kubernetes-sigs/cluster-api/pull/7066/files#diff-c5a9f9e91fea8f113dbe8cc4feece40bed422499e3372094ae037f119b94ff78

I'll share anything I find - it's down to tabs vs spaces on some files. I'm not sure yet if there's a clean way to fix it. You can run the verify-boilerplate.py script directly (with -v) which might help see what the errors are

[killianmuldoon]

I managed to resolve the header issues over at: https://github.com/killianmuldoon/cluster-api/pull/31

I didn't need to change the boilerplate there's just a small number of the files that have a special character in the header (which some mix of gofmt and verify-boilerplate.sh is catching now).

I just copied the header from one of the files that's passing verify-boilerplate and it's all passing (at least locally 😆 )

[oscr]

It seems like it needs a space between the header and the package. That makes all the difference.

diff --git a/main.go b/main.go
index ba81125db..2445aef8d 100644
--- a/main.go
+++ b/main.go
@@ -13,6 +13,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 */
+
 package main
 
 import (

I've added it and now lint and boilerplate pass locally at least

[oscr]

Sorry for the spam.

[sbueringer]

No worries :)

/lgtm

cc @killianmuldoon for a final look

[sbueringer]

Yup thank you very much!

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

• ~~OWNERS~~ [sbueringer]

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