-
Notifications
You must be signed in to change notification settings - Fork 19
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
update: generate protobufs with tags field support and fix makefile #55
update: generate protobufs with tags field support and fix makefile #55
Conversation
Signed-off-by: Jason Dellaluce <[email protected]>
Signed-off-by: Jason Dellaluce <[email protected]> Co-authored-by: Leonardo Grasso <[email protected]>
@jasondellaluce: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Welcome @jasondellaluce! It looks like this is your first PR to falcosecurity/client-go 🎉 |
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.
PR changes look good to me.
The only thing I'm not getting is why the file names are different now.
Can we keep the file names of the generated *.pb.go the same as before?
Ie., pkg/api/outputs/outputs.pb.go
rather than the new pkg/api/outputs/output_grpc.pb.go
Thank you for your review! This is caused by the changes at: https://github.com/falcosecurity/client-go/pull/55/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R35 From my understanding, it seems like the "new way" is to generate two separate files, one containing the gRPC service definitions (_grpc.pb.go) and one containing the protobuf message information (.pb.go). This is due to the fact the two files are now generated by two distinct tools,
This topic is also related to the error of #54, which currently breaks our
|
Oh, I see. They split the services out. Gotcha! /lgtm |
LGTM label has been added. Git tree hash: 8b1042c0f87c65ddfe847157d3023ed5329f7d13
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jasondellaluce, leodido The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
/kind feature
Any specific area of the project related to this PR?
/area api
/area makefile
What this PR does / why we need it:
This PR contributes with the following:
tags
in theoutputs
services.go.mod
.Which issue(s) this PR fixes:
Fixes #54
Does this PR introduce a user-facing change?: