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: bump golangci-lint to v1.44, golangci-lint-action to v3 #3370

Merged
merged 3 commits into from
Mar 8, 2022

Conversation

kolyshkin
Copy link
Contributor

and format sources using gofumpt 0.2.1 (which is uncluded to that golangci-lint version).

thaJeztah
thaJeztah previously approved these changes Feb 9, 2022
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda
Copy link
Member

gofumpt seems too much nitpicky.
Can we just disable it?

@thaJeztah
Copy link
Member

Overall, I like the formatting that gofumpt makes (most of them would be what I would do myself, but of course that may be preferential).

The change it makes in update.go and devicefilter_test.go feel like improvements to me (less cluttered, and in the latter case definitely less confusing). I'm less "convinced" about the change in process_linux.go (although it points out that interface methods were not documented (and probably should've been; even if not exported)).

I do agree though (hope!) that it won't change too much over time, and stabilizes to prevent too much code-churn.

Alternatively, we could periodically do a round of gofumpt updates (and group them together); not sure if that would be more disruptive though.

@kolyshkin
Copy link
Contributor Author

I agree that gofumpt is more opinionated that gofmt but so far I like all the changes it did.

I also understand that having gofumpt in linters raises the barrier to new runc contributors, as (unlike gofmt) it's a non-standard tool, and it also may create a nuisance for the seasoned contributors. Again, I think, the nice changes it makes outweight it (so far).

We can certainly stop using it if it ever becomes too opinionated/nitpicky (or we can talk to its author), but so far, in my opinion, it hasn't happen -- all these changes are for good.

@kolyshkin
Copy link
Contributor Author

gofumpt seems too much nitpicky.

I bet you haven't seen wsl linter yet 🤣

@kolyshkin
Copy link
Contributor Author

So, due to my bad memory I ended up reimplementing this today, to find out I did that once already.

I have cleaned up the last commit a bit, and added one more to prepare for golangci/golangci-lint-action#403 (so that #3397 will succeed, once rebased on top of this one).

Can we please have this merged? @opencontainers/runc-maintainers @AkihiroSuda PTAL

AkihiroSuda
AkihiroSuda previously approved these changes Mar 1, 2022
... which adds a wee more whitespace fixes.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Also, remove "must be specified without patch version" as this is no
longer true.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
golangci-lint-action v3 no longer installs golang itself, and the
version that comes with Ubuntu is not new/good enough.

Install go 1.17.x explicitly.

Introduce GO_VERSION environment variable to avoid duplication,
and use it instead of 1.x in other places, so that implicit go update
won't bring some unexpected failures.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin changed the title ci: bump golangci-lint to v1.44 ci: bump golangci-lint to v1.44, golangci-lint-action to v3 Mar 7, 2022
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 4e0d689 into opencontainers:main Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants