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
Upgraded proto related deps: grpc and protobuf; removed gogo from core. #321
Conversation
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
d709847
to
ed34491
Compare
aafc980
to
d16a4d2
Compare
cb10fb7
to
df04530
Compare
@googlebot I consent |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Just to remind the goals of this PR:
Given the gogo getting... obsolete (is that right?) maybe we can just stop depending on gogo completely. |
Gogo is indeed unlikely to recover, though Kubernetes just reached out to ask what's going on: https://twitter.com/dims/status/1334219405069258753. |
Ok, getting back to this! (: Let's move this forward. Upgrading both protobuf and grpc to latest |
df04530
to
934ecec
Compare
934ecec
to
2809a32
Compare
2809a32
to
b5a78c3
Compare
a4f5db6
to
e83983b
Compare
Should be fine now (: |
e83983b
to
49fc59c
Compare
Codecov Report
@@ Coverage Diff @@
## v2 #321 +/- ##
===========================================
- Coverage 83.58% 63.52% -20.06%
===========================================
Files 30 34 +4
Lines 932 913 -19
===========================================
- Hits 779 580 -199
- Misses 114 279 +165
- Partials 39 54 +15
Continue to review full report at Codecov.
|
Makes sense @yashrsharma44, fixed PTAL 🤗 |
b8cc81b
to
55f20ac
Compare
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
55f20ac
to
582d3a5
Compare
Rebased, PTAL 🤗 |
return nil, fmt.Errorf("jsonpb serializer failed: %v", err) | ||
func logProtoMessageAsJson(logger Logger, pbMsg proto.Message, key string, msg string) { | ||
payload, err := protojson.Marshal(pbMsg) | ||
if err != 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.
So we can break this rule only for errors 😛 - https://www.bwplotka.dev/2020/how-thanos-would-program-in-go/#control-structure-prefer-early-returns-and-avoid-else
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.
No, we should break it only if it makes sense to do so.
I was thinking about this and since it's only oneliner it does not matter if we do with or without else. WDYT?
I think we could update style guide to mention for multi line bodies? 🤔
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 🔥
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 @yashrsharma44
Let's wait for @johanbrandhorst review until tomorrow, then we can merge & iterate if something is wrong!
return nil, fmt.Errorf("jsonpb serializer failed: %v", err) | ||
func logProtoMessageAsJson(logger Logger, pbMsg proto.Message, key string, msg string) { | ||
payload, err := protojson.Marshal(pbMsg) | ||
if err != 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.
No, we should break it only if it makes sense to do so.
I was thinking about this and since it's only oneliner it does not matter if we do with or without else. WDYT?
I think we could update style guide to mention for multi line bodies? 🤔
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.
Nice work! Sorry for the delay on the review. I think we should be able to remove gogofast
now?
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
done! @johanbrandhorst |
Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com