Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI] Update golangci to v2 #4048

Merged
merged 1 commit into from
Mar 30, 2025
Merged

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Mar 27, 2025

This updates golangci to their newly released v2.

  1. convert the configuration file to the new format
  2. update the makefile install task (also bump version for other tools we use)
  3. remove go-imports-reviser - we can now use gci (bundled with golangci) with 99% compatibility (save a handful of changes required in code)
  4. remove fix-imports, lint-imports (see ^)

I opted to configure the new .golangci.yml for the minimal set of changes to the source code.
However, that meant disabling a bunch of checks from staticcheck and a couple of formatters, that we may want to investigate / enable.

# - gomodguard
# - goprintffuncname
# - gosec (gas)
- gosimple # (megacheck)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now part of staticcheck.

enable:
- depguard
- gofmt
- goimports
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are now "formatters".
gofmt is down there - goimports is not necessary as we use gci instead.

run:
concurrency: 6
timeout: 5m
modules-download-mode: readonly
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concurrency does not help - we should let it pick whatever matches max proc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeout here does not help either - we have timeouts everywhere, and it is hard to figure out which ones kicks when.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeout here does not help either - we have timeouts everywhere, and it is hard to figure out which ones kicks when.

The idea was to limit the CI to avoid overflow and optimize.

- govet
- ineffassign
- misspell
- nakedret
- prealloc
- typecheck
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer a linter and cannot be disabled.

.golangci.yml Outdated
- "-ST1021"
- "-ST1022"
# FIXME: this below this point is disabled for now, but we should investigate
- "-QF1008"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are new checks in staticcheck that we do not pass. Some of them are just annoying, but others are more interesting and we should look into this.

settings:
gci:
sections:
- standard
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gci is what replaces our import reviser and that configuration defines how we want our import sectioned.

@apostasie apostasie marked this pull request as ready for review March 27, 2025 22:40
@apostasie
Copy link
Contributor Author

Note: this is independent from the ongoing work on Tigron, though I would prefer to get it merged first before the upcoming changes in test.command.

# golangci: v2.0.2 (2024-03-26)
# git-validation: main (2025-02-25)
# ltag: main (2025-03-04)
# go-licenses: v2.0.0-alpha.1 (2024-06-27)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These irrelevant lines will probably disappear after rebasing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm...
Not completely sure what you mean - but these have been updated on mod/tigron/Makefile - not yet on nerdctl/Makefile, so they are part of this changeset, right?

.golangci.yml Outdated
- "-ST1021"
- "-ST1022"
# FIXME: this below this point is disabled for now, but we should investigate
- "-QF1008"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Googled this, only got flight information about Qantas Airways 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HAHAHA

@AkihiroSuda AkihiroSuda added this to the v2.0.5 milestone Mar 28, 2025
@AkihiroSuda AkihiroSuda added the area/ci e.g., CI failure label Mar 28, 2025
@fahedouch
Copy link
Member

Hi @apostasie, thanks for pushing the v2 version. Did you use the migration tools to make the changes?

Signed-off-by: apostasie <[email protected]>
@@ -30,6 +30,7 @@ import (

"github.com/containerd/nerdctl/mod/tigron/expect"
"github.com/containerd/nerdctl/mod/tigron/test"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the differences between gci and go-imports-reviser.

@apostasie
Copy link
Contributor Author

Hi @apostasie, thanks for pushing the v2 version. Did you use the migration tools to make the changes?

hey @fahedouch !

Yes, I started from the output of the migration tool and guide.
In our case though, it required more changes after the migration to get it working.

@apostasie
Copy link
Contributor Author

@AkihiroSuda @fahedouch just added a few changes:

  • added comments to staticcheck rules as requested
  • re-enabled revive with the full list of rules we need disabled right now

Also working on a follow-on PR to fix a bunch of issues and enable some rules.

@apostasie
Copy link
Contributor Author

CI failure looks like GHA cache garbage collection tripping us.

@apostasie
Copy link
Contributor Author

Rootless failure is #4046

&& go install gotest.tools/gotestsum@ac6dad9c7d87b969004f7749d1942938526c9716
@echo "Remember to add GOROOT/bin to your path"
@echo "Remember to add \$$HOME/go/bin to your path"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path depends on GOPATH, GOBIN

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.
Will amend in a follow-on "cleanup" PR.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@AkihiroSuda AkihiroSuda merged commit 1837b2f into containerd:main Mar 30, 2025
54 of 55 checks passed
@fahedouch
Copy link
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci e.g., CI failure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants