-
Notifications
You must be signed in to change notification settings - Fork 100
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
uninstall via extension / jsonrpc / grpc #1712
uninstall via extension / jsonrpc / grpc #1712
Conversation
jsonRpcResponse: jsonRpcResponse{ | ||
DisableDevice: req.DisableDevice, | ||
}, | ||
Message: req.Message, |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
t.Parallel() | ||
|
||
// set up JSON-RPC test server | ||
jsonRpcServerResult := `{"disable_device": false}` |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
disable_device
field in jsonRpc / gRPC responses and return aErrDeviceDisabled
if foundErrDeviceDisabled
when it receives and error and perform uninstall if found