-
Notifications
You must be signed in to change notification settings - Fork 2.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
add error details to error json #515
add error details to error json #515
Conversation
Ok, I'm not sure I understand what is happening on travis. Could someone please have a look and share a hint? 😉 |
@tmc do you have any thoughts on this? |
I'm having the same problem in #489. It seems some commit between go 1.9 and If someone finds a fix to this, keep me posted on my PR, please. |
Looking at this, it seems to me that the test we have for the go version is just wrong and it is running our clean test against go master and go 1.9. I fixed it in #520. I'm going to get lunch and let the CI run then merge that. Sorry for all the trouble. It was an interesting bug if you wanna stare at it. |
Could you rebase this onto the current HEAD? I updated the travis config so this shouldn't be a problem any more. |
982e89b
to
0c4d78a
Compare
@@ -248,6 +249,21 @@ func (s *_ABitOfEverythingServer) Timeout(ctx context.Context, msg *empty.Empty) | |||
} | |||
} | |||
|
|||
func (s *_ABitOfEverythingServer) ErrorWithDetails(ctx context.Context, msg *empty.Empty) (*empty.Empty, error) { | |||
stat := status.New(codes.Unknown, "with details") |
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 it would be better to collapse the next few lines
stat, err := status.New(codes.Unknown, "with details").
WithDetails([]proto.message{
...
})
0c4d78a
to
5283d2e
Compare
Fixes grpc-ecosystem#405. If we go with this, that is. In a separate commit, I'll update the generated files. Signed-off-by: Stephan Renatus <[email protected]>
Signed-off-by: Stephan Renatus <[email protected]>
5283d2e
to
1a446cc
Compare
This is arguably more correct than what was introduced in grpc-ecosystem#515. So, any error details no have a "@type" field indicating their underlying proto.Message's type, following the Cloud API docs[1]. [1]: https://cloud.google.com/apis/design/errors#http_mapping Signed-off-by: Stephan Renatus <[email protected]>
This is arguably more correct than what was introduced in #515. So, any error details no have a "@type" field indicating their underlying proto.Message's type, following the Cloud API docs[1]. [1]: https://cloud.google.com/apis/design/errors#http_mapping Signed-off-by: Stephan Renatus <[email protected]>
Signed-off-by: Stephan Renatus <[email protected]>
) This is arguably more correct than what was introduced in grpc-ecosystem#515. So, any error details no have a "@type" field indicating their underlying proto.Message's type, following the Cloud API docs[1]. [1]: https://cloud.google.com/apis/design/errors#http_mapping Signed-off-by: Stephan Renatus <[email protected]>
I've tried to pick up where the discussion in #405 has trailed off.
I'm not sure about the import changes of the generated api_clients, but I'll see what happens when travis comes in.