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

Remove support for Go1.6-1.8 #2428

Merged
merged 3 commits into from
Nov 1, 2018
Merged

Remove support for Go1.6-1.8 #2428

merged 3 commits into from
Nov 1, 2018

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Nov 1, 2018

This change is Reviewable

Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

Missing README changes.

Reviewed 51 of 51 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dfawley)


channelz/service/service_test.go, line 54 at r1 (raw file):

// This behavior will make reflect.DeepEqual fail upon unset time.Time field,
// and cause false positive fatal error.
// TODO: Go1.7 is no longer supported - does this need a change?

@lyuxuan to take a look.


credentials/credentials.go, line 315 at r1 (raw file):

func init() {
	cipherSuiteLookup[tls.TLS_RSA_WITH_AES_128_CBC_SHA256] = "TLS_RSA_WITH_AES_128_CBC_SHA256"

Merge these with the map above.


credentials/credentials.go, line 327 at r1 (raw file):

//
// If cfg is nil, a new zero tls.Config is returned.
func cloneTLSConfig(cfg *tls.Config) *tls.Config {

I think we can just inline this function. Add a TODO?

Copy link
Member Author

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Done

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @menghanl)


channelz/service/service_test.go, line 54 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

@lyuxuan to take a look.

Let's follow-up on that in a separate PR.


credentials/credentials.go, line 315 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Merge these with the map above.

Done.


credentials/credentials.go, line 327 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

I think we can just inline this function. Add a TODO?

Done.

Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@menghanl
Copy link
Contributor

menghanl commented Nov 1, 2018

And change labels to keep this in the release notes.

@menghanl menghanl added Type: Dependencies Updating/adding/removing dependencies and removed Type: Internal Cleanup Refactors, etc no release notes labels Nov 1, 2018
@menghanl menghanl added this to the 1.17 Release milestone Nov 1, 2018
@dfawley dfawley merged commit 59a2cfb into grpc:master Nov 1, 2018
@dfawley dfawley deleted the go1618nomo branch December 6, 2018 22:07
@mitar
Copy link

mitar commented Dec 14, 2018

It would be great if you would version your install links. Now go get -u google.golang.org/grpc fails on go 1.8 in my Docker file while before it was working. Maybe consider providing things like go get -u google.golang.org/grpc-v1.16 to be able to pin to a specific version.

@dfawley
Copy link
Member Author

dfawley commented Dec 14, 2018

@mitar I don't think go get works like that. What you're describing is more like how modules work (experimental in go1.11). Otherwise, you will need to vendor your dependencies (manually or by using a tool like dep) if you want to use an unsupported version of Go.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Dependencies Updating/adding/removing dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants