Grpc fiber router integration #4
Conversation
leonlnj
requested review from
pradithya and
a team
and removed request for
a team
config/config.go
Outdated
| Service string `json:"service"` | ||
| Method string `json:"method"` | ||
| Protocol string `json:"protocol,omitempty"` | ||
| Service string `json:"service,omitempty"` |
There was a problem hiding this comment.
Shall we move these into a GrpcConfig struct? Everything except Service and Method are applicable to both Http and grpc, right?
There was a problem hiding this comment.
Service and Method are only exclusive to grpc too.
For http, the url and method are set within http.Request of the Request payload. For grpc, this information is stored in dispatcher.
config/config.go
Outdated
| Protocol string `json:"protocol"` | ||
| Service string `json:"service"` | ||
| Method string `json:"method"` | ||
| Protocol string `json:"protocol,omitempty"` |
There was a problem hiding this comment.
Why is this omitempty? I think it would be clearer to force the user to set this value instead of defaulting to http. We can update any depending projects (like Turing) accordingly, when we consume the latest Fiber changes.
config/config_test.go
Outdated
| { | ||
| name: "grpc proxy", | ||
| configPath: "../internal/testdata/config/invalid_grpc_proxy.yaml", | ||
| expectedErrMsg: "fiber: grpc dispatcher: missing config (endpoint/service/method/response-proto)", |
There was a problem hiding this comment.
We removed the response-proto from the config, right? The error message from NewDispatcher should be updated.
|
Added some comments, the rest LGTM. Thanks! |
config/config.go
Outdated
| @@ -67,15 +67,16 @@ func (d Duration) MarshalJSON() ([]byte, error) { | |||
| } | |||
|
|
|||
| // Routes takes in an object of type Routes and returns a map of each route's ID and the route | |||
| func (r Routes) Routes() map[string]fiber.Component { | |||
| func (r Routes) Routes() (map[string]fiber.Component, error) { | |||
| routes := make(map[string]fiber.Component) | |||
| for _, routeConfig := range r { | |||
| if route, err := routeConfig.initComponent(); err != nil { | |||
There was a problem hiding this comment.
nit:
if route, err := routeConfig.initComponent(); err != nil {
return nil, err
}
routes[route.ID()] = route
|
thanks for the quick review! |
Description
Minor bugfix for grpc fiber
Changes
omitemptyto proxy struct'sprotocol, service, methodfield, as these json fields are not required in HTTP setting