-
Notifications
You must be signed in to change notification settings - Fork 1.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
GRPC dependencies updates #3075
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3075 +/- ##
==========================================
- Coverage 76.76% 75.31% -1.45%
==========================================
Files 230 232 +2
Lines 17179 17592 +413
==========================================
+ Hits 13187 13249 +62
- Misses 3145 3491 +346
- Partials 847 852 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
one proto, which could lead to duplicates of definitions and panic.
2a8d567
to
0573269
Compare
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.
LGTM! although I feel like we now have 3 different packages with protosets and am not certain if we should have 3 different ones 🤷
// a small hack to make sure we are parsing the right file | ||
// otherwise the parser will try to parse "google/protobuf/descriptor.proto" | ||
// with exactly the same name as the one we are trying to parse for testing | ||
if filename != path { | ||
return nil, nil | ||
} |
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.
Why is it trying to load a different file?
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.
It's coming from the library update bufbuild/protocompile@7c5114e
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 didn't go through the vendored changes, but the ones in our repo look good. 👍 Besides that hack that Mihail mentioned, which is not very clear.
Do you know what happened to the test.proto
file after it was deleted in grpc/grpc-go#6164? Is it now embedded in the generated pb.go
files? I couldn't figure out from the PR how they're testing with the new package.
It sucks that we now have to maintain grpc_testing
ourselves, but it's better to have control over it, and we're forced to anyway :-/
Well, yeah, it seems like the source proto file was just deleted, we could maybe check with authors (or maybe just open the PR to them) https://github.com/grpc/grpc-go/blob/master/interop/grpc_testing/test.pb.go#L22 |
What?
This PR brings updates of the dependencies with the fixes (see commits breakdown):
github.com/jhump/protoreflect
Why?
Closes: #3069