-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
api: Add gRPC status proto definition #10631
Conversation
Signed-off-by: Tony Allen <tony@allen.gg>
@@ -0,0 +1,40 @@ | |||
syntax = "proto3"; |
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.
Where is this used or planned to be used?
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.
See the linked PR in the description.
// GRPC status. | ||
message GrpcStatus { | ||
// Supplies GRPC response code. | ||
Status status = 1 [(validate.rules).enum = {defined_only: true not_in: 0}]; |
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.
Does this mean that it can't be OK?
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 had the same question/comment in the other PR.
option java_multiple_files = true; | ||
option (udpa.annotations.file_migrate).move_to_package = "envoy.type.grpc.v3"; | ||
|
||
// [#protodoc-title: GRPC status codes] |
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.
Nit: s/GRPC/gRPC/ everywhere
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'm actually not sure we need this PR at all if we switch to expressing the status as an int as @lizan suggested in the other PR?
/wait
// [#protodoc-title: GRPC status codes] | ||
|
||
// GRPC response codes supported. | ||
enum Status { |
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.
See @lizan comment in the other PR about not using an enum for this.
// GRPC status. | ||
message GrpcStatus { | ||
// Supplies GRPC response code. | ||
Status status = 1 [(validate.rules).enum = {defined_only: true not_in: 0}]; |
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 had the same question/comment in the other PR.
@@ -0,0 +1,40 @@ | |||
syntax = "proto3"; |
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.
See the linked PR in the description.
@mattklein123 I'm more than happy to drop this PR in favor of just using an integer, but my only concern would be readability of the configurations. I'm fine either way, so feel free to just close this out if you'd prefer the integer specification. |
Signed-off-by: Tony Allen <tony@allen.gg>
My feeling is that we should use an integer per @lizan and just clearly document what the integer means with a link/reference to the code mappings. So yeah I will close. |
Adds a gRPC status proto definition, analogous to the HTTP status found in
http_status.proto
. Currently, this proto is not in use, but is needed for #9658.Risk Level: Low
Testing: n/a
Docs Changes: Proto documented
Release Notes: n/a