gci linter no longer autofixing import orders

[killianmuldoon]

gci linter is no longer fixing import order with make lint-fix. This issue was noted in #6348. The errors from the linter are opaque making fixing import order a manual and time consuming process for maintainers - and very discouraging for all contributors.

It seems from here that the autofix was disabled in a recent version c/f golangci/golangci-lint#2604. I'm still researching why this occurred and will update with any information here.

We can either

• remove gci from our golangci-lint.
• or downgrade to an older version, i.e. roll back ✨Bump to golangci-lint v1.45.2 #6335

Until the linter's behaviour in autofixing comes back in line with expectations. I'm in favour of doing a downgrade - gci was really useful when make lint-fix was working.

/kind bug
[One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels]

[killianmuldoon]

/assign

[sbueringer]

Hm, could it be an alternative to call gci directly as part of lint-fix? I think they did something similar here: https://github.com/vmware-tanzu/cartographer/pull/691/files

(although I would still run gci via golangci-lint)

I think we need this only until they fixed it in golangci-lint/gci.

[vincepri]

Are folks maintaining golanci-lint aware of this issue?

[killianmuldoon]

There's an issue at golangci/golangci-lint#2604
And a PR with a fix at golangci/golangci-lint#2653

[fabriziopandini]

/milestone v1.2

[sbueringer]

I think we have a few options:

1. just wait until the fix is merged in golangci-lint and then upgrade (i.e. ignore the issue until then)
2. downgrade now and then upgrade again once it's fixed (we can't upgrade golangci-lint until then)
3. provide a way to run gci "fix" via make (e.g. as part of lint-fix)

Not sure what the best way forward is, given that we don't know how soon they'll fix it. Maybe 2 for now and 3 if they don't fix it soon?

[sbueringer]

Okay PR is merged, so we picked 2. Let's see what happens upstream in golangci-lint

[killianmuldoon]

golangci/golangci-lint#2892

This PR fixes the issue in golangci-lint. We should try to bump the version as soon as there's a new release.

[chrischdi]

Just because I did a lookup already the second time:

Hello, there are some minor features that have been merged, so we will not create a patch release. [0]

So we have to wait for a v1.47 release.

[schrej]

Yep. We've also asked for a release. One of the maintainers wants to get a few more fixes in before a release, but it seems like they can't find time to implement them unfortunately. See here

[sbueringer]

@schrej Your IPAM PR depends on Go 1.18 right? (i.e. using go 1.18 in go.mod files)

[schrej]

@schrej Your IPAM PR depends on Go 1.18 right? (i.e. using go 1.18 in go.mod files)

Yes. I'm using the shiny new net/netip package in the validation webhooks. I could also change it to https://pkg.go.dev/inet.af/netaddr, from which that std lib package has been derived and swap it out once we're on 1.18.

[killianmuldoon]

We could temporarily disable gci on the CI instead of refactoring that PR, assuming a release with the fix is inbound but just won't make it for 1.2.

[sbueringer]

My idea was:

• keep go 1.17 in go.mod for the v1.2.x release to avoid forcing all CAPI consumers to use Go 1.18
• bump to go 1.18 in go.mod files on main (we can do that today, golangci-lint still works)
  • bump golangci-lint later on. If it takes too long, bump golangci-lint and find another solution for gci

[schrej]

If we can get the IPAM PR into 1.2 that way, I can switch it to netaddr, and then switch back after the bump. Having it in a release makes it easier to consume for provider implementations I think.

[sbueringer]

Just to confirm. This package is only used in the validating webhook and not as part of the API types? (so changing the package wouldn't be a breaking change to the API types)

[daixiang0]

Sorry for this, golangci/golangci-lint#2892 has fixed it, let's wait for the next patch release.

[fabriziopandini]

/triage accepted

[daixiang0]

New release has come, please try :)

[chrischdi]

It's currently getting tracked via #6737 , the recent golangci lint disabled some linters in revive due to performance issues.

[killianmuldoon]

I think we should close this in favor of #6737. This no longer represents the issue with golangci-lint as of version 1.47.

This is also being tracked broadly as a part of #release task #5968

/close

[k8s-ci-robot]

@killianmuldoon: Closing this issue.

In response to this:

I think we should close this in favor of #6737. This no longer represents the issue with golangci-lint as of version 1.47.

This is also being tracked broadly as a part of #release task #5968

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

[sbueringer]

Fine for me to close this, but just to mention it. I think usually we shouldn't close issues in favor of PRs. But I guess good for now as we still have #5968.