uninstall via extension / jsonrpc / grpc #1712
Conversation
James-Pickett
changed the title
James-Pickett
changed the title
| jsonRpcResponse: jsonRpcResponse{ | ||
| DisableDevice: req.DisableDevice, | ||
| }, | ||
| Message: req.Message, |
There was a problem hiding this comment.
Naive question -- why are the other things really obvious looking struct members, but this one is a nested thing?
There was a problem hiding this comment.
this is because the jsonRpc implementation is doing a standard json unmarshalling where as the grpc implementation is taking the auto-generated protobuf structpb.AgentApiResponse and converting it the struct type publishLogsResponse via manually setting the fields
we could also probably just marshall the protobuf to json then unmarshall it into the publishLogsResponse
There was a problem hiding this comment.
Hrm. Caveat, I'm not sure what correct is...
My hunch is that we should do the same thing for jsonrpc and grpc, but I'm not sure if we should improve the conversion or just go the manual route.
At some point, I think we should level up all the API surfaces via some kind of API specification, but that feels a bit out of scope for this.
James-Pickett
changed the title
| t.Parallel() | ||
|
|
||
| // set up JSON-RPC test server | ||
| jsonRpcServerResult := `{"disable_device": false}` |
There was a problem hiding this comment.
I think the service package became a little frankensteiny over the years. Mixing jsonRPC and gRPC implementations. I'm tempted to split them up and perhaps have a common types package. Not sure if it's worth the effort at this time since we're not really using the grpc portion.
There was a problem hiding this comment.
I think go-kit has a recommendation for how to do this. It's core design is all about splitting up transport implementation from protocol definition.
| if isNodeInvalidErr(err) { | ||
|
|
||
| switch { | ||
| case errors.Is(err, service.ErrDeviceDisabled{}): |
There was a problem hiding this comment.
hahahaha. This really points at how this method signature was limiting. This feels like a reasonable workaround, but definitely points to needing to refresh this code.
|
|
||
| switch { | ||
| case errors.Is(err, service.ErrDeviceDisabled{}): | ||
| uninstall.Uninstall(ctx, e.knapsack, true) |
There was a problem hiding this comment.
Should we return here? Falling through looks wrong
| if isNodeInvalidErr(err) { | ||
| switch { | ||
| case errors.Is(err, service.ErrDeviceDisabled{}): | ||
| uninstall.Uninstall(ctx, e.knapsack, true) |
There was a problem hiding this comment.
same -- should we return?
disable_devicefield in jsonRpc / gRPC responses and return aErrDeviceDisabledif foundErrDeviceDisabledwhen it receives and error and perform uninstall if found