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 module required go version #5102

Closed
wants to merge 1 commit into from

Conversation

sunjayBhatia
Copy link

@sunjayBhatia sunjayBhatia commented Jan 4, 2022

The README mentions it should be used with any one of the three latest major releases. (link)

This change bumps the minimum go version to 1.15 as currently 1.17 is
the latest major release.

RELEASE NOTES:

  • Bump required Go version to 1.15 as per README prerequisites

The README mentions it should be used with `any one of the three latest
major releases.` ([link](https://github.com/grpc/grpc-go#prerequisites))

This change bumps the minimum go version to 1.15 as currently 1.17 is
the latest major release.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@dfawley
Copy link
Member

dfawley commented Jan 4, 2022

I'm not sure there's much value in this change by itself. If the code still compiles on 1.14, I don't see a strong reason to artificially restrict ourselves to 1.15+ yet. What is the underlying motivation for this?

@sunjayBhatia
Copy link
Author

I'm not sure there's much value in this change by itself. If the code still compiles on 1.14, I don't see a strong reason to artificially restrict ourselves to 1.15+ yet. What is the underlying motivation for this?

  • The README mentions the latest 3 major releases of Go as prerequisites, 1.14 falls out of that now
  • go-control-plane is bumping to 1.16+ as minimum required (see Update go build version to something current envoyproxy/go-control-plane#532), motivated by the Go support policy being N-1
    • In case new language features get used in go-control-plane or other deps, we dont want to surprise/break grpc-go

@dfawley
Copy link
Member

dfawley commented Jan 4, 2022

go-control-plane is bumping to 1.16+ as minimum required (see Update go build version to something current envoyproxy/go-control-plane#532), motivated by the Go support policy being N-1

Is there any specific thing triggering the desire to change it now? If you take this policy in go-control-plane, then gRPC-Go will need to always be using an older release -- which means any updates to the protos in the main envoy repo will not be accessible to gRPC-Go for at least 6 months.

@sunjayBhatia
Copy link
Author

go-control-plane is bumping to 1.16+ as minimum required (see Update go build version to something current envoyproxy/go-control-plane#532), motivated by the Go support policy being N-1

Is there any specific thing triggering the desire to change it now? If you take this policy in go-control-plane, then gRPC-Go will need to always be using an older release -- which means any updates to the protos in the main envoy repo will not be accessible to gRPC-Go for at least 6 months.

I think just a due diligence thing, but @jpeach would have to answer that one if theres something more specific

@jpeach
Copy link

jpeach commented Jan 4, 2022

go-control-plane is bumping to 1.16+ as minimum required (see Update go build version to something current envoyproxy/go-control-plane#532), motivated by the Go support policy being N-1

Is there any specific thing triggering the desire to change it now? If you take this policy in go-control-plane, then gRPC-Go will need to always be using an older release -- which means any updates to the protos in the main envoy repo will not be accessible to gRPC-Go for at least 6 months.

I think just a due diligence thing, but @jpeach would have to answer that one if theres something more specific

Yeh, it's about not being to far (we were on Go 1.11, which has different modules defaults IIUC), and being on a supported Go release. If it's going to cause unnecessary churn across the ecosystem, we ought to accommodate that, but I do think that being on a supported stack is desirable.

@jpeach
Copy link

jpeach commented Jan 4, 2022

If you take this policy in go-control-plane, then gRPC-Go will need to always be using an older release -- which means any updates to the protos in the main envoy repo will not be accessible to gRPC-Go for at least 6 months.

FWIW, I think that there is general support in the community for separating the proto definitions out from the go-control-plane implementation. Needs some bazel and CI expertise to make that happen.

@dfawley
Copy link
Member

dfawley commented Jan 4, 2022

FWIW, I think that there is general support in the community for separating the proto definitions out from the go-control-plane implementation. Needs some bazel and CI expertise to make that happen.

This would be great to do before such a change. Providing those protos should be done with as old a Go language version as feasible, since they are very basic and fundamental definitions. This is envoyproxy/go-control-plane#391. We could afford to import an older version of the rest of the repo if needed.

Also note that there is a difference between "not supported" and "explicitly made to prevent". With our current go.mod, grpc-go probably works with 1.14, even though we don't support it (in the sense that if a bug is discovered that only manifests on 1.14, we may choose not to fix it). With this PR, we would actively be breaking 1.14 users that may have been able to run previously.

@sunjayBhatia
Copy link
Author

noticed that the testing workflows explicitly check this code compiles on the latest 3 Go releases, so CI should catch a bump to go-control-plane that breaks grpc-go, thats maybe a better check than this change tbh, willing to close given that exists

- type: vet+tests
goversion: 1.17
- type: tests
goversion: 1.17
testflags: -race
- type: extras
goversion: 1.17
- type: tests
goversion: 1.17
goarch: 386
- type: tests
goversion: 1.17
goarch: arm64
- type: tests
goversion: 1.16
- type: tests
goversion: 1.15

@dfawley
Copy link
Member

dfawley commented Jan 5, 2022

CI should catch a bump to go-control-plane that breaks grpc-go

Because of modules, no change in go-control-plane will break us until we update our version of gcp that we use.

@sunjayBhatia
Copy link
Author

CI should catch a bump to go-control-plane that breaks grpc-go

Because of modules, no change in go-control-plane will break us until we update our version of gcp that we use.

Yeah, if there were a change that broke compilation on say go 1.15, the PR to bump go-control-plane would go red and we could address it then

@dfawley
Copy link
Member

dfawley commented Jan 5, 2022

OK, I will close this PR then.

My recommendation would be to bump the version in your go.mod only when you take advantage of language features that require it, not to reflect your support policy.

@dfawley dfawley closed this Jan 5, 2022
@sunjayBhatia sunjayBhatia deleted the bump-require-go-1.15 branch January 5, 2022 17:33
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants