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

Bump CI to use supported go version, fix a single Go 1.20 issue in mount pkg; rm uneeded errorlint annotations #129

Merged
merged 6 commits into from
Jul 3, 2024

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Aug 25, 2023

  1. Bump Go to supported version, other CI components.
  2. Bump go to 1.17 in */go.mod.
  3. Bump golang.org/x/sys to v0.11.0. v.0.1.0.
  4. mount: fix an errorlint issue with Go 1.20+.
  5. mount: rm unneeded errorlint annotations (thanks to Allow bare errors from golang.org/x/sys/unix polyfloyd/go-errorlint#47).
  6. sequential: fix a formatting issue (file is not gofumpt'ed)

@kolyshkin kolyshkin requested a review from thaJeztah August 25, 2023 00:50
@kolyshkin kolyshkin changed the title Bump CI to use supported go version, fix a single Go 1.20 issue in mount pkg Bump CI to use supported go version, fix a single Go 1.20 issue in mount pkg; rm uneeded errorlint annotations Aug 25, 2023
.github/workflows/test.yml Outdated Show resolved Hide resolved
mount/mount_unix.go Outdated Show resolved Hide resolved
@kolyshkin kolyshkin force-pushed the bump-ci branch 2 times, most recently from 5a708b5 to ba81eaa Compare August 25, 2023 19:37
@kolyshkin kolyshkin requested a review from thaJeztah August 26, 2023 01:27
@kolyshkin
Copy link
Collaborator Author

@thaJeztah ptal

mount/go.mod Outdated
@@ -4,5 +4,5 @@ go 1.17

require (
github.com/moby/sys/mountinfo v0.6.2
golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a
golang.org/x/sys v0.11.0
Copy link
Member

Choose a reason for hiding this comment

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

if we don't need anything newer, I prefer follow Go module's "minimum version selection", and use the oldest tagged version (v0.1.0), and leave it up to users of this module to pick the version they want

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, PTAL @thaJeztah

@thaJeztah
Copy link
Member

@kolyshkin are you ok with picking a "minimum" version for golang.org/x/sys, or did you have a specific reason to require the latest version?

@kolyshkin
Copy link
Collaborator Author

@kolyshkin are you ok with picking a "minimum" version for golang.org/x/sys, or did you have a specific reason to require the latest version?

Yes, I don't think we have a requirement for a specific version here. Will update.

Since Go 1.20,

> [t]he fmt.Errorf function now supports multiple occurrences of the %w
> format verb, which will cause it to return an error that wraps all of
> those error operands.

This change, together with newer errorlint, causes the following
warning:

> mount_unix.go:74:56: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
> 					return fmt.Errorf("%w (possible cause: %s)", err, suberr)
>					                                                  ^

As we still want to support Go 1.17, use explicit string for suberr,
and add a TODO to switch to %w once we switch to Go 1.20+.

Fixes: 487129d
Signed-off-by: Kir Kolyshkin <[email protected]>
1. Test with supported Go releases (1.20.x and 1.21.x). Also leave Go
   1.17.x since it costs us little to keep supporting it.

2. Bump golangci-lint to the latest released version (v1.55.1).

3. Bump actions/setup-go to v4.

Signed-off-by: Kir Kolyshkin <[email protected]>
It is already that way for sequential, and Go 1.17 is the oldest version
we use in CI, so let's bring other modules go.mod in sync.

Signed-off-by: Kir Kolyshkin <[email protected]>
This is the oldest tagged version of x/sys. This allows to follow Go
module's "minimum version selection", leaving it up to users of this
module to pick the version they want.

Signed-off-by: Kir Kolyshkin <[email protected]>
golangci-lint v1.54.2 comes with errorlint v1.4.4, which contains
the fix [1] whitelisting all errno comparisons for errors coming from
x/sys/unix. Remove the annotation that is no longer needed.

Unfortunately, switch on a bare unix error (in mountinfo) still needs to
be annotated (see [2]).

[1] polyfloyd/go-errorlint#47
[2] polyfloyd/go-errorlint#54

Signed-off-by: Kir Kolyshkin <[email protected]>
This fixes the following CI error:

> sequential_windows.go:114: File is not `gofumpt`-ed (gofumpt)

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Collaborator Author

@thaJeztah ptal

1 similar comment
@kolyshkin
Copy link
Collaborator Author

@thaJeztah ptal

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! Sorry for the delay, somehow this dropped off my radar 🙈

@thaJeztah
Copy link
Member

Looks like polyfloyd/go-errorlint@c0e6cac is part of go-errorlint v1.5.2, which is included in GolangCI-lint v1.59.1; https://github.com/golangci/golangci-lint/releases/tag/v1.59.1

I'll do a follow-up after this to update golangci-lint and to remove that other nolint-comment; also macos-12 runners were deprecated, so will include updates for that as well.

@thaJeztah thaJeztah merged commit 954b593 into moby:main Jul 3, 2024
13 checks passed
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.

2 participants