-
Notifications
You must be signed in to change notification settings - Fork 61
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
chore: update temporary entries for cloud.google.com/go/spanner in go.mod to public versions supporting PG feature #265
chore: update temporary entries for cloud.google.com/go/spanner in go.mod to public versions supporting PG feature #265
Conversation
….mod to public versions supporting PG feature.
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
* | ||
*/ | ||
|
||
package naming |
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.
Please add a comment linking where this snippet has been taken from
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.
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.
Do you think we need to add here too?
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.
Makes sense to add a comment here too, just to make it clear that this is going to be a copy of the source and not to add any custom code.
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.
Done!! Github is not showing Outdated
for some reasons.
|
||
// Package naming defines the naming API and related data structures for gRPC. | ||
// | ||
// This package is deprecated: please use package resolver instead. |
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.
please add a comment as to where this snippet has been taken from
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.
Done!! Github is not showing Outdated
for some reasons.
LGTM, thanks for this fix. Please update the PR description as well though. You can simply copy paste the readme contents. |
Fixes #<issue_number_goes_here>
This PR also adds naming module to repository.
What is the motivation behind this?
We need this because of a 'dependency hell' situation:
grpc-go
makes breaking changes between minor releases of their module.grpc/naming
API which was removed in v1.30. We depend on etcd v0.5.0-alpha indirectly via tidb v1.1.0-beta.This situation would be solved by bumping tidb to version which depends on etcd v3.6.0+, which does not make use of the removed grpc-go API.
This module is used to solve this dependency hell, by adding the
google.golang.org/grpc/naming
package into version v1.44 of grpc-go.It is the smallest patch that can be applied to solve this particular 'dependency hell' issue with grpc-go. It will not fix other issues when packages depend on other, newer packages.
When can it be removed?
This must be removed when tidb does not need grpc/naming.