-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Introduce pkg/grpcutil #617
Conversation
This function was used for handling errors in the graceful library and was never necessary for the v3 API.
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.
Some comments on gateway.go
We should also add tests on server connectivity in the future.
pkg/grpcutil/gateway.go
Outdated
if tlsConfig != nil { | ||
var gwTLSConfig *tls.Config | ||
gwTLSConfig = tlsConfig.Clone() | ||
gwTLSConfig.InsecureSkipVerify = true |
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.
Can we add a comment on why we use InsecureSkipVerify here?
pkg/grpcutil/gateway.go
Outdated
|
||
srvmux := runtime.NewServeMux( | ||
// Include empty fields with default values. | ||
runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.JSONPb{EmitDefaults: true}), |
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 remember why do we want to include the empty fields?
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.
Nope
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.
Yeah, just get rid of it, I don't think there will be any issues.
This change refactors gRPC code used within the v3 API package into generic code that can be used for managing gRPC and gRPC Gateway.
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
This change refactors all of the gRPC code used in the v3 package to be generic and reusable.