Skip to content
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.build to generate go from proto #73

Closed
wants to merge 1 commit into from
Closed

Use buf.build to generate go from proto #73

wants to merge 1 commit into from

Conversation

mmorel-35
Copy link
Contributor

@mmorel-35 mmorel-35 commented Nov 10, 2023

I have used buf.build to regenerate the go files from the proto file .

This just needs to execute from root folder

go install github.com/bufbuild/buf/cmd/buf@latest 
buf generate --template buf.gen.go.yaml

I don’t have any knowledge on how bazel work to adapt the actual workflow but there is a documentation for this here :

https://buf.build/docs/build-systems/bazel

Signed-off-by: Matthieu MOREL [email protected]

@mmorel-35 mmorel-35 marked this pull request as ready for review November 10, 2023 07:13
Comment on lines 25 to 28
- uses: bufbuild/buf-push-action@v1
with:
buf_token: ${{ secrets.BUF_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to publish proto as buf.build/cncf/xds in buf.build registry

@mmorel-35 mmorel-35 closed this Nov 27, 2023
@mmorel-35 mmorel-35 deleted the go branch November 27, 2023 20:34
@mmorel-35 mmorel-35 restored the go branch January 14, 2024 18:20
@mmorel-35 mmorel-35 reopened this Jan 14, 2024
@mmorel-35 mmorel-35 marked this pull request as draft January 14, 2024 18:21
@mmorel-35
Copy link
Contributor Author

@phlax , what do you think about this ?

@mmorel-35 mmorel-35 marked this pull request as ready for review January 14, 2024 19:09
@phlax
Copy link
Member

phlax commented Jan 15, 2024

what do you think about this ?

i think its generally a good idea - i tried to introduce buf to this repo previously (#47) altho in that case just for linting

having said that i think it should be using bazel to install/run/build buf - my pr above covers at least the install part - i would be happy to help setting up a build target

cc @htuch @markdroth @adisuissa

@mmorel-35
Copy link
Contributor Author

Can you contribute to my PR or shall include your modifications myself ?

@phlax
Copy link
Member

phlax commented Jan 15, 2024

lets get a general nod from repo maintainers before proceeding - i think the next step would be to land the buf/bazel installation, and then figure it out from there

@markdroth
Copy link
Contributor

I don't know much about buf, but if the idea is to use that instead of protobuf to generate the go protos, we would need to make sure that the resulting generated code is compatible with protobuf, since there are existing go data planes using these protos with protobuf. @dfawley can speak to this w.r.t. grpc-go. @howardjohn may also want to weigh in from the Istio side.

@howardjohn
Copy link
Contributor

I don't know much about buf, but if the idea is to use that instead of protobuf to generate the go protos, we would need to make sure that the resulting generated code is compatible with protobuf, since there are existing go data planes using these protos with protobuf. @dfawley can speak to this w.r.t. grpc-go. @howardjohn may also want to weigh in from the Istio side.

We use buf in Istio. I think its a great tool, especially if not using bazel (but may still be useful without bazel, I just haven't done the mix of the two; the alternative is a mess of makefiles generally). Its sort of like a build tool specific for proto, it is still invoking the same proto plugin binaries so the generation should be identical; this PR shows more changes since its also changing the versions of those binaries, but in general it should only change the comment (where it says which version of protoc was used for generation).

@htuch
Copy link
Contributor

htuch commented Jan 17, 2024

Makes sense to me, will defer to Go folks.

@mmorel-35
Copy link
Contributor Author

mmorel-35 commented Jan 26, 2024

Hi @htuch ,
Any news from the "Go folks" ?
Can @phlax start working on bazel based buf code generation and validation ?

By the way, following your comment

this PR shows more changes since its also changing the versions of those binaries

I tried to align the used version with the actually used for the generation but it seems like protoc-gen-validate was not in v1.0.2 as gomod says it

@phlax
Copy link
Member

phlax commented Jan 26, 2024

@mmorel-35 busy with other things atm - ill separate out the bazel/buf install into its own PR later or over w/end

@mmorel-35
Copy link
Contributor Author

mmorel-35 commented Jan 26, 2024

@phlax ,

busy with other things atm

It was more in the sense that, there is nothing blocking you on there point of view. Like if they are not interested there is no point to start working on it. Once this is not a blocker for them, you can start if and when ever you want. Sorry if this was misleading.

ill separate out the bazel/buf install into its own PR

I totally agree

Comment on lines +11 to +13
sha256 = "27b3202ba6e9eb37dca0f902c8f96b6330670b33d8669ad739f60ab61bdaf296",
strip_prefix = "protoc-gen-validate-1.0.2",
urls = ["https://github.com/envoyproxy/protoc-gen-validate/archive/refs/tags/v1.0.2.tar.gz"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dfawley
Copy link
Member

dfawley commented Jan 26, 2024

it is still invoking the same proto plugin binaries so the generation should be identical

Yes, this is the key; they still run our codegen, so this change seems more about "how the code generators are executed" vs. "what code is generated". So from gRPC-Go's perspective, this change is a no-op.

@mmorel-35 mmorel-35 marked this pull request as draft February 5, 2024 21:29
@mmorel-35 mmorel-35 closed this by deleting the head repository Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants