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

grpc: accept interface in NewClient functions #1025

Merged
merged 4 commits into from Jan 28, 2020
Merged

Conversation

dfawley
Copy link
Contributor

@dfawley dfawley commented Jan 24, 2020

Addresses grpc/grpc-go#1287

I will send a v2 version of this change shortly.

@dsnet
Copy link
Member

dsnet commented Jan 25, 2020

I can make the v2 equivalent changes for you if you want to avoid the hassle of learning to use Gerrit. Would you like me to do that?

@dfawley
Copy link
Contributor Author

dfawley commented Jan 27, 2020

Thanks for the offer, @dsnet. I already made the changes actually, but got stuck because the go.mod in the testdata directory specified an older release of grpc-go and I couldn't figure out how to make go get fetch an unreleased HEAD version containing the new interfaces. When I specify the hash for the commit, it says:

$ go get google.golang.org/grpc@7afcfdd66b1241ed5a29e865d838ce19c57fc2d6
go: finding google.golang.org/grpc 7afcfdd66b1241ed5a29e865d838ce19c57fc2d6
go get: inconsistent versions:
	google.golang.org/grpc@v1.2.1-0.20200124224931-7afcfdd66b12 from google.golang.org/grpc@7afcfdd66b1241ed5a29e865d838ce19c57fc2d6 requires google.golang.org/grpc@v1.23.0 (not google.golang.org/grpc@v1.2.1-0.20200124224931-7afcfdd66b12 from google.golang.org/grpc@7afcfdd66b1241ed5a29e865d838ce19c57fc2d6)

I don't understand where the "v1.2.1" part comes from, since nothing seems to reference grpc@v1.2.1 that I can see. There may be something wrong with our master branch - it was used to release v1.0.x and v1.2.0 directly (these tags are in master), whereas now we release on branches, so maybe now go mod thinks master contains a "working" v1.2.1 release. Do you have any ideas for how to work around this? Should we tag master with 2.0.0-alpha or something to trick go mod into treating master as always being at a later version than any release branch? I don't really want to delete the tags since there were official releases at those commits.

Anyway, our next release is tomorrow so I can wait until then for the protov2 changes, unless you know how to make go get happy. Thanks!

@dfawley dfawley requested a review from dsnet January 27, 2020 17:12
@dfawley
Copy link
Contributor Author

dfawley commented Jan 27, 2020

I updated go.mod to replace grpc with a specific version (vs. specify it directly as a version dependency), which worked around the problem:

CL: https://go-review.googlesource.com/c/protobuf/+/216399

I don't fully understand why that works, but it does. The version number still seems like a problem with our master branch that I'd like to fix, but this is probably fine for now.

EDIT: s/require/replace

@dfawley
Copy link
Contributor Author

dfawley commented Jan 27, 2020

Do you have any ideas for how to work around this?

I'm now thinking we should tag our master branch with v1.<n+1>.0-pre immediately after cutting release branch 1.n - what do you think?

@dfawley
Copy link
Contributor Author

dfawley commented Jan 27, 2020

I tagged our commit that set "v1.27.0-dev" in our user agent (right after we cut the v1.26.x branch) with "v1.27.0-pre", and that seems to have done the trick. Unless we hear otherwise, we will do this upon every new release. I also fixed all the tests, which required similar methodology as in the proto-v2 branch w.r.t. a go.mod file in the testdata/grpc directory so that the proper version of grpc (containing these symbols) would be used. PTAL.

@dsnet
Copy link
Member

dsnet commented Jan 27, 2020

I'm curious, do you have the ability to squash and merge, or should I do that for you?

@dfawley
Copy link
Contributor Author

dfawley commented Jan 27, 2020

I can't merge at all, because I am not in the golang org, so you will have to do it. Thanks for the review!

@dsnet dsnet merged commit d23c512 into golang:master Jan 28, 2020
@golang golang locked and limited conversation to collaborators Sep 4, 2020
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.

None yet

2 participants