Better gRPC error handling #1251
Conversation
I like this, it's a great improvement that's very much needed. I just have some doubt about the returning an |
64b9066
to
adfa4a2
Is this considered a breaking change to the API? If so, does the standard process for documenting it need to be done still? |
🤦🏼 right, we should prepare a migration guide. I'll write down some notes later today... |
@@ -75,13 +75,13 @@ func ParseLibraryReferenceArgs(args []string) ([]*LibraryReferenceArg, error) { | |||
// ParseLibraryReferenceArgAndAdjustCase parse a command line argument that reference a | |||
// library and possibly adjust the case of the name to match a library in the index | |||
func ParseLibraryReferenceArgAndAdjustCase(instance *rpc.Instance, arg string) (*LibraryReferenceArg, error) { | |||
libRef, err := ParseLibraryReferenceArg(arg) | |||
libRef, _ := ParseLibraryReferenceArg(arg) |
silvanocerza
Aug 11, 2021
Contributor
Shouldn't we fix this by handling this err
instead of ignoring it?
Shouldn't we fix this by handling this err
instead of ignoring it?
per1234
Aug 11, 2021
Contributor
Probably already clear, but I did this because this assignment of err
was ignored and the error
type from the declaration was incompatible with the change of lib.LibrarySearch()
's return type from error
to *status.Status
.
If it's to be ignored, I think it's best to make that explicit by using _
. But if an error return from ParseLibraryReferenceArg()
is significant in this context, then of course it's better to handle it.
Probably already clear, but I did this because this assignment of err
was ignored and the error
type from the declaration was incompatible with the change of lib.LibrarySearch()
's return type from error
to *status.Status
.
If it's to be ignored, I think it's best to make that explicit by using _
. But if an error return from ParseLibraryReferenceArg()
is significant in this context, then of course it's better to handle it.
cmaglie
Aug 11, 2021
Author
Member
Yep, the err
was ignored also before this PR (it was overwritten without being checked). This should be fixed in another PR BTW.
Yep, the err
was ignored also before this PR (it was overwritten without being checked). This should be fixed in another PR BTW.
The translation files must be updated, the rest is good. I'll approve as soon that's fixed. |
After trying the current solution for a bit I found two annoying problems:
PROPOSAL To address the two problems above, while rebasing this PR on master, I'd like to try a different approach:
|
Yes, I 100% agree with this new design. I think it also make it easier to handle errors internally and recover them gracefully. |
4a6bd7c
to
c146dae
Previously, a custom error was returned when attempting to upgrade a platform that was already at the latest available version. There is dedicated code for handling this specific error. Now that the function has been changed to return a status.Status instead of error, the previous check for the return being this error is no longer possible. The capability is restored by replacing the error with a protocol buffer message.
The status details of the function are used to identify the specific cause of a non-nil status. This is done via a type assertion. Previously, this type assertion was configured such that a details of any type other than the expected would result in a panic. At the moment, that will not occur because we only add details of one type. However, the whole point of the type assertion is to support details of multiple types, and if other types are added a panic will not be the appropriate behavior. A better approach is to check the result of the type assertion, handling the non-nil status as a generic error if its details are of a different type.
Co-authored-by: per1234 <[email protected]>
777b59c
to
63dd036
All in all it's a great improvement but there are some small things to fix. |
Co-authored-by: Silvano Cerza <[email protected]>
This is a proposal to uniform gRPC error handling.
For now only the
upload
command has been ported, as a proof-of-concept. It will be extended to all thearduino-cli
code base if approved.This PR, once fully implemented, should:
status.Status
instead of anerror
(theStatus
message is how the gRPC transports errors on RPC calls).codes
Status.details
field (for example theUploadRequiresProgrammerError
)