-
Notifications
You must be signed in to change notification settings - Fork 457
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
Use buf for proto generation #286
Conversation
* Delete old generate code * Add buf yaml files * Update .proto files to use relative package paths * Update proto generation docs * Update example python plugin instructions * Updated proto import path for example python plugin * Update .gitignore rules
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.
This looks great, well done!
- paths=source_relative | ||
- plugin: python | ||
out: plugin-python | ||
- plugin: buf.build/grpc/python:v1.58.1 |
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 easy 😍. If you want, we could use remote plugins the same way for the go and go-grpc plugins and drop those tools from this repo. Usually the argument against it is that we don't want to depend on the BSR but since we're doing this for python anyway, why not?
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.
Yeah I think that would be a nice win to make it easy to use consistent plugin versions too. Added in e340950.
@@ -3,6 +3,7 @@ | |||
|
|||
syntax = "proto3"; | |||
package proto; | |||
option go_package = "./proto"; |
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.
These are somewhat unusual. I might suggest using the "managed mode" feature of Buf's generation instead: https://buf.build/docs/generate/managed-mode. That would mean you can drop this option in all the files and buf would automatically set it to something sensible. It'll almost certainly look something like this:
managed:
enabled: true
go_package_prefix:
default: github.com/hashicorp/go-plugin/examples/bidirectional/proto # go.mod path + proto output path
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.
Is there a way to do this but keep the slightly odd grpctest
package name the same? I know it's only a test package, but it is technically part of the public API so I'd like to be quite strict about avoiding breaking changes, just to make the safety of the change easy to evaluate.
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.
Looks good to me in general. I want to do some testing to see if plugins built against this still work as expected against ones built against previous versions of go-plugin
.
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
I pulled this down and did some testing that this still worked with Vault (all tests seem to pass for me locally, and plugins built using the old go-plugin seem to work with a newer Vault).
Thanks! |
The documented protobuf generation steps no longer work. We could make some smaller incremental updates, but I thought it was a good opportunity to modernise the tooling and migrate to buf. However, I'm happy to close this PR and do the smaller changes if people aren't happy that this is safe enough.
I've split the PR into 3 commits to make the review easier:
buf generate
commands.Additional notes/oddities:
examples/grpc/plugin-go-netrpc
which didn't work before and still doesn't. It's easy to get working with a small code change, but I left it alone for this PR as it's somewhat off topic.