CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Upgrade to Go 1.18 #6709
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
Upgrade to Go 1.18 #6709
Conversation
By taking advantage of smarter traversal of dependencies, a `go mod tidy` using Go 1.18 remove some items from go.sum. Signed-off-by: Tom Godkin <tomgodkin@pm.me>
Hi @BooleanCat. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Sorry I should change that when I was working on #6605.
/ok-to-test
We still support Go 1.17, so probably it's better to keep go.mod version to be 1.17 too |
The Go directive does not impose a minimum Go version. If containerd is compiled with an earlier version of Go, it will point out the mismatch only if a compile error occurs. Compatibility with Go 1.17 is still supported and can be guaranteed with Ci. |
@@ -1,6 +1,6 @@ | |||
module github.com/containerd/containerd | |||
|
|||
go 1.17 | |||
go 1.18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. In here there should be a minimally supported version, so unless you do want to drop go 1.17 support right now (which is probably not the right thing to do, since go 1.17 is fully supported and is probably most used version at the moment), I would advise against this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTOH maybe I'm wrong #6709 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A go directive indicates that a module was written assuming the semantics of a given version of Go
I am not sure exactly what this means, but I guess that unless the code starts using Go 1.18 features, it should not say "go 1.18" in go.mod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Go team has been very ambiguous wishy-washy on this; the version does indicate the minimum version that's expected to be compatible, but they were not enforcing that (yet??), and started doing things like this in their own repositories (e.g. golang/sys@0981d60) - effectively disregarding their own recommendations to at least support latest+previous (even if the version in go mod is not enforced yet).
Changing the version does affect what versions can be used to manage modules though; i.e., running go mod tidy
with go 1.17 will no longer produce the same result (which means that anyone working on this project must use go1.18).
For this one, I'm a bit on the fence; is there anything we gain from the removed lines in go.sum? (the change from <1.17 to 1.17 did bring the improved "indirect" dependencies, so was clearer on benefits)
If we do decide to change, we should also update the go.mod
in the integration client (which still looks to be on 1.15);
containerd/integration/client/go.mod
Line 3 in 388ee88
go 1.15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small benefit I can see (which is what led me to to make this change) is that it results in a less complicated go mod graph
which brings two benefits:
- Fewer false positives on security scanning tools
- For downstream tools using the containerd library, a potentially smaller
go mod vendor
output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can change this to go 1.18
as long as we build containerd against 1.17 on our CI. This will guaranty compatibility.
@AkihiroSuda @mxpv Please take a look. The directive doesn't block Go 1.17. |
By taking advantage of smarter traversal of dependencies, a
go mod tidy
using Go 1.18 remove some items from go.sum.Signed-off-by: Tom Godkin tomgodkin@pm.me