Update to grpc-gateway v2 #6567
Conversation
hack/generate-proto.sh
Outdated
| @@ -16,16 +16,18 @@ | |||
|
|
|||
| set -e | |||
|
|
|||
| docker build -t gen-proto -f hack/proto/Dockerfile --target generate-files proto | |||
| docker build --platform linux/amd64 -t gen-proto -f hack/proto/Dockerfile --target generate-files proto | |||
There was a problem hiding this comment.
Do we really need to mention the platform switch?
There was a problem hiding this comment.
Oops. I was trying to build from an M1 and it was failing. I'll back this out.
briandealwis
force-pushed
the
grpc-gateway-v2
branch
from
7d22452 to
c447965
Compare
Codecov Report
@@ Coverage Diff @@
## main #6567 +/- ##
==========================================
- Coverage 70.48% 70.38% -0.11%
==========================================
Files 515 517 +2
Lines 23150 23365 +215
==========================================
+ Hits 16317 16445 +128
- Misses 5776 5857 +81
- Partials 1057 1063 +6
Continue to review full report at Codecov.
|
|
CI/CD failing with lint error: The kokoro failure is the known flake - |
|
Thanks @aaron-prindle — I left that code in to see as the integration tests were failing when cloning the state using the JSON marshaller as |
|
Marking as blocked: hold off until next release. |
| var suggestions2 protoV2.Suggestion | ||
| suggestions2.Action = suggestion.Action | ||
| suggestions2.SuggestionCode = suggestion.SuggestionCode | ||
| suggestions2.Action = suggestion.Action | ||
| suggestionsV2[i] = &suggestions2 |
There was a problem hiding this comment.
feel free to add a todo here for me to put this into a function since we use it a couple times
There was a problem hiding this comment.
Are there other places?
There was a problem hiding this comment.
I think there are just 2, here and in func V2fromV1() a bit further down in the same file
Fixes #5986
Related: #6556
Description
This PR separates out an upgrade of
grpc-gatewaythat @baleshk incorporated into his CloudEvents PR in #6556.Protobufs must be passed by-reference and not by-value
A side-effect is that I had to change our code pass protobuf objects by reference instead of by value (more below). The newer proto generation now marks protobuf objects with a
DoNotCopymarker to generate warnings when they are copied. Protobuf objects are not meant to be copied by value: they include an internal field that is meant to be accessed atomic access, and this field has been there for a while.Swagger/OpenAPI output has changed
@baleshk noted that error handling is removed in the newer gateway. As a result the swagger output is changed.
The gRPC Gateway migration guide notes that they now use the RPC status return.
This change is for the better:
OLD:
NEW:
User facing changes (remove if N/A)
Follow-up Work (remove if N/A)
Filed #6600 to track follow-on work: the Swagger editor complains about our Swagger output as we're missing a version field, and we have a lot of badly named types (
protoState) that would be nice to clear up.