-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
ability to customize stream errors #930
Conversation
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.
Thanks a ton for this - really excited about this new functionality! How would you feel about also adding a section about configuring the stream errors to our documentation?
@johanbrandhorst, the build errors aren't making a lot of sense to me. I'm not a bazel user, but I understand the concepts (I've used blaze and pants). I tried to add a dependency from the runtime package to runtime/internal (surprised it was not already present), but that is still failing. Any advice on getting the build green? I'm also really confused about the other errors, which appear to be related to Go modules. The confusing thing is that a few lines up from the error, it states that it resolved "github.com/grpc-ecosystem/grpc-gateway/runtime", but somehow can't resolve the "internal" sub-package therein :( I can keep hacking, but if you can spot any simple issues in here, that would certainly speed things up. |
Something about this import certainly seems to have got Go modules confused - I also have no idea what the hell is wrong here. I'll do some more investigating. |
Wait, |
Doh! Thanks for the tip. (You can probably tell that this patch was rebased from an older version of the repo. We've had it in a fork that is apparently pretty old: 463c5ed, from March 2018.) |
I celebrate the implication that you're trying to go back to the source :). I hope we can make you feel at home! |
Codecov Report
@@ Coverage Diff @@
## master #930 +/- ##
==========================================
+ Coverage 53.5% 53.53% +0.03%
==========================================
Files 40 40
Lines 3957 3964 +7
==========================================
+ Hits 2117 2122 +5
- Misses 1642 1645 +3
+ Partials 198 197 -1
Continue to review full report at Codecov.
|
@johanbrandhorst: I think this is ready for another pass, whenever you get a chance. |
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.
How do you feel about my suggestion to add some documentation about this new feature too?
I added a section to the "Customizing your Gateway" doc page. PTAL. |
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.
Thanks for this fantastic contribution!
* ability to customize stream errors * fix build(?); review feedback * default stream error handler; some other cleanup * don't use ProtoErrorHandlerFunc for any stream calls -- not backwards compatible * add details about stream error handler to docs
Fixes #584