Skip to content
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.

Sign up for GitHub

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

Jump to bottom

#1098: remove error field from error messages #1242

Merged
merged 8 commits into from
Apr 28, 2020
Merged

#1098: remove error field from error messages #1242

merged 8 commits into from
Apr 28, 2020

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Apr 25, 2020 •
edited
Loading

Fixes #1098.

⚠️ Doesn't remove internal/errors.proto's StreamError. That can be done next ;)

srenatus
srenatus commented Apr 25, 2020
View reviewed changes
internal/errors.pb.go Outdated
@@ -1,7 +1,7 @@
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
// protoc-gen-go v1.21.0
// protoc v3.10.1
// protoc v3.7.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this, or commit it from all the other .pb.go files where the bazel run has changed just this line. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be able to remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(How) do we control the protoc version used?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in the build environment Dockerfile at the moment. Clearly this file isn't being regenerated.

@srenatus srenatus force-pushed the sr/issue-1098/remove-error-field-from-error-messages branch 4 times, most recently from d49998e to 690b66f Compare April 26, 2020 07:41
@srenatus srenatus force-pushed the sr/issue-1098/remove-error-field-from-error-messages branch from 690b66f to 3578c48 Compare April 26, 2020 12:16
@srenatus srenatus force-pushed the sr/issue-1098/remove-error-field-from-error-messages branch from 3578c48 to 263253a Compare April 26, 2020 12:57
@srenatus
Copy link
Contributor Author

Looks like removing the Error message type has implications on the openapi spec:

Not a blocker, just something I hadn't had on my radar. I'll see into replacing that with the status message (similar to protobufAny).

@srenatus
protoc-gen-swagger: use google.rpc.Status instead of .grpc.gateway.ru…
4775173 
…ntime.Error

Signed-off-by: Stephan Renatus <[email protected]>
@srenatus
protoc-gen-swagger: rename AddStreamError -> AddErrorDefs, add to tests
166f49a 
There are probably better ways to write this in than calling
`AddErrorDefs` everywhere.

Signed-off-by: Stephan Renatus <[email protected]>
@srenatus srenatus force-pushed the sr/issue-1098/remove-error-field-from-error-messages branch from e068cb8 to 166f49a Compare April 27, 2020 12:05
@srenatus
Copy link
Contributor Author

So far, the generate failures look reasonable. Let's drop StreamError next.

@srenatus
Copy link
Contributor Author

I haven't fully understood how the StreamError handling works -- I think I'd rather we got this in, and I (or someone else) pick up removing StreamError next. What do you think?

@srenatus srenatus marked this pull request as ready for review April 27, 2020 13:11
@srenatus
rebuild all the things
a641c87 
Note: I've also run

    find examples -name "*runtime_error*" -delete

before. It seems like those are not captured by any of the *clean make
targets.

Signed-off-by: Stephan Renatus <[email protected]>
@srenatus
run "bazel run :gazelle"
a416b83 
Signed-off-by: Stephan Renatus <[email protected]>
@srenatus srenatus force-pushed the sr/issue-1098/remove-error-field-from-error-messages branch from d055703 to a416b83 Compare April 27, 2020 13:21
johanbrandhorst
johanbrandhorst reviewed Apr 27, 2020
View reviewed changes
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great Stephan! Could you have a quick grep through the docs to see if we talk about the old error structure anywhere?

examples/internal/integration/integration_test.go Outdated Show resolved Hide resolved
runtime/errors.go Show resolved Hide resolved
runtime/errors.go Outdated Show resolved Hide resolved
@srenatus srenatus force-pushed the sr/issue-1098/remove-error-field-from-error-messages branch from a06f357 to 2aaabe5 Compare April 28, 2020 08:41
@codecov-io
Copy link

Codecov Report

Merging #1242 into v2 will increase coverage by 0.08%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v2    #1242      +/-   ##
==========================================
+ Coverage   54.45%   54.54%   +0.08%     
==========================================
  Files          42       42              
  Lines        4429     4422       -7     
==========================================
  Hits         2412     2412              
+ Misses       1754     1748       -6     
+ Partials      263      262       -1
Impacted Files Coverage Δ
protoc-gen-swagger/main.go 26.12% <0.00%> (ø)
runtime/errors.go 46.47% <75.00%> (-3.53%) ⬇️
protoc-gen-swagger/genswagger/generator.go 11.90% <100.00%> (+1.58%) ⬆️
protoc-gen-swagger/genswagger/template.go 57.91% <100.00%> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6d0536...2aaabe5. Read the comment docs.

@srenatus
Copy link
Contributor Author

@johanbrandhorst I've done a quick grep, but I'm afraid I wasn't very thorough.

@johanbrandhorst
Copy link
Collaborator

Great work Stephan! Lets get this in :). You've reminded me that we need to have a general look at the error functions, I believe we have too many ways to configure error handling at the moment.

@johanbrandhorst johanbrandhorst merged commit 5ece275 into grpc-ecosystem:v2 Apr 28, 2020
@srenatus srenatus deleted the sr/issue-1098/remove-error-field-from-error-messages branch April 28, 2020 15:27
johanbrandhorst added a commit that referenced this pull request Apr 30, 2020
@johanbrandhorst
runtime: replace StreamError with status.Status
858b295 
Following on from #1242, this replace the StreamError
with a status.Status type. Also, remove the ability to configure
the stream error handler. The existing handler was specific to the
old type, and we can add something better back in later
if necessary.

Fixes #1098
@johanbrandhorst johanbrandhorst mentioned this pull request Apr 30, 2020
Merged
johanbrandhorst added a commit that referenced this pull request May 2, 2020
@johanbrandhorst
runtime: replace StreamError with status.Status
53da419 
Following on from #1242, this replace the StreamError
with a status.Status type. Also, remove the ability to configure
the stream error handler. The existing handler was specific to the
old type, and we can add something better back in later
if necessary.

Fixes #1098
@idoqo idoqo mentioned this pull request Mar 17, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanbrandhorst johanbrandhorst johanbrandhorst approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@srenatus @codecov-io @johanbrandhorst @googlebot