Add protobuf format

[kconwayinvision]

This patch adds an implementation of v2/format.Format for protobuf encoding
of the event envelope. A future patch will add an implementation of the
protobuf datacodec. The full spec for reference: https://github.com/cloudevents/spec/blob/master/protobuf-format.md.

[kconwayinvision]

I tried to mirror the JSON format as closely as possible in terms of code structure, testing, and naming. I've left several NOTE comment blocks in places where I wanted to draw attention to an issue or ambiguity I wasn't sure how to solve. One issue not called out by a NOTE is that the current protobuf generated code for the envelope is fixed to a point in time. How would folks like re-generating the code to be automated or scripted? For example, should the protobuf regenerate on each commit or only on-demand by calling a script? Also, do you have any guidance for how I might bring in the compiler as a binary tool for the project?

[slinkydeveloper]

Thanks for your contribution, I have a couple of comments:

• We want to have the protobuf format in a separate gomod (in a similar fashion to the various protocol implementations, in order to avoid coupling the "core sdk" to protobuf deps). So I guess you should move the protobuf code in /format/protobuf and create a go mod there
• We need to automatize the code generation of protobuf, can you add a go:generate comment to protobuf.go so it will execute the protoc compiler?

[kconwayinvision]

OK, I'll make some changes. Would you clarify a few things for me, please?

move the protobuf code in /format/protobuf and create a go mod there

Is the request that I create a subdirectory with a unique package name or that I include a go.mod file so that it is a separately distributed module entirely? The protocols in /v2 seem to be regular packages but the top-level protocols are modules so I'm not certain which pattern to follow. If a distinct module, is the request that I move this code to a new top-level directory called /format or that I create these modules under the /v2 path?

go:generate comment to protobuf.go so it will execute the protoc compiler

Should the build assume that there is a protoc binary installed or should I also add automation around downloading/installing the compiler. If the latter, do you have any suggestions for how you'd like this done? I don't see any other examples of managing build tools outside of the github actions. Is there a precedent you would like set for non-CI, non-Go dependency management or would you prefer it to work like the format action and fail the build if code generation is out of date?

[slinkydeveloper]

The protocols in /v2 seem to be regular packages but the top-level protocols are modules so I'm not certain which pattern to follow. If a distinct module, is the request that I move this code to a new top-level directory called /format or that I create these modules under the /v2 path?

I guess you need the /v2 path, so /formats/protobuf/v2 and then go mod init

[slinkydeveloper]

Should the build assume that there is a protoc binary installed or should I also add automation around downloading/installing the compiler.

Let's assume protoc is setup and setup it in github actions (It seems like it's not already included https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md, so maybe just an apt-get install will do it?)

[kconwayinvision]

OK, I've moved the protobuf code into its own module. If this looks correct then I'll take a look at getting actions to install and run protoc.

[slinkydeveloper]

You don't need to place it under /v2/binding/format/protobuf/v2, you should be able to do just /binding/format/protobuf/v2

[kconwayinvision]

You don't need to place it under /v2/binding/format/protobuf/v2, you should be able to do just /binding/format/protobuf/v2

Ah, OK. I moved it to that path.

[dan-j]

Added some comments, I think the overall implementation of the format is OK, the main concern is around making sure the workflow for generating updated protobuf specs is right.

[dan-j]

I don't think this is used?

[kconwayinvision]

This method is a copy of similar methods exported in https://github.com/cloudevents/sdk-go/blob/master/v2/event/content_type.go#L12. I was trying to match the existing implementations as much as possible. I can remove this in the next patch.

[dan-j]

I pointed out cloudevent.pb.go that you've used a development build of protoc-gen-go. We need to make sure developers running go generate have the correct dependencies.

One way I have solved this problem in the past is to have an additional tools.go file with a build tag so that it is not built with the source, but dependencies are tracked in go.mod, see this comment for more background:

// +build tools

package pb

// import packages required to run protoc commands, the versions will be tracked in go.mod
import (
	_ "google.golang.org/protobuf/cmd/protoc-gen-go"
)

Then you have a shell script, generate.sh:

# puts the protoc-gen-go executable in $GOBIN
go install google.golang.org/protobuf/cmd/protoc-gen-go
protoc --go_out=. cloudevent.proto

And update this gen.go file to simply be:

//go:generate sh generate.sh
package pb

Disclaimer: I've copied the above snippets from a codebase which I maintain, but we use vendoring so had to modify a bit, not sure whether it's 100% correct.

[dan-j]

Just noticed, this only solves the problem of having the correct version of protoc-gen-go installed, developers would still need to manually install the protoc compiler. One to address later I think?

[kconwayinvision]

I use a similar technique for other projects. One variation is to make a tools submodule that isolates the build dependencies from the real ones. For example, folks could go get github.com/cloudevents/sdk-go/binding/format/protobuf/v2 without downloading the protoc plugin but folks with the source cloned could cd binding/format/protobuf/v2/tools && GOBIN="/path/to/protoc/plugins" go get.

I can set up either one but they both require that the developer have GOBIN on their path for it to work and protoc to be installed separately. Alternatively, I can set up a Makefile or script that can manage downloading those dependencies and coordinating the path in a way that doesn't use the developer's global GOBIN. The downside is that it's difficult to make something work across all development environments that way and particularly between windows and others.

[dan-j]

Should the build assume that there is a protoc binary installed or should I also add automation around downloading/installing the compiler.

Let's assume protoc is setup and setup it in github actions (It seems like it's not already included https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md, so maybe just an apt-get install will do it?)

@slinkydeveloper do we need the tools in GitHub Actions? I've always approached the problem of code generation as being the developer's responsibility. Automating it in CI adds other complications of how to commit those changes back. Although, you could run it in CI and fail if the sources do not match what's been committed?

[slinkydeveloper]

@slinkydeveloper do we need the tools in GitHub Actions?

Now that I think about that, maybe we don't need it? At the end of the day, this contract won't change very often, and it will change only after changing it in the spec repo... Maybe it's not worth, given the complications of the CI to make it working...

[kconwayinvision]

Maybe it's not worth, given the complications of the CI to make it working...

If that's the case then I can leave the //go:generate comments in for devs with protoc installed and add some docs that link folks to installation and usage instructions. Is there a particular place those kinds of dev docs should go?

I can also re-generate the schema using the latest stable release of protoc and protoc-gen-go. I believe I was using pre-release versions on my workstation leading up to the stable release of the new protobuf API. It wasn't an intentional choice, just how my workstation was configured.

If those two things wrap up the protobuf code generation then I'd still like to get some thoughts on the bigger NOTEs that I left. This comment brings up a question about default encoding that I'm still not sure how to answer. There seems to be some ambiguity in the spec.

I could also use a second option on whether this is the right way to set binary content that is not base64 encoded. It seems the SDK automatically sets Base64 to true when using setter method.

[slinkydeveloper]

Is there a particular place those kinds of dev docs should go?

Maybe CONTRIBUTING.md?

I can also re-generate the schema using the latest stable release of protoc and protoc-gen-go. I believe I was using pre-release versions on my workstation leading up to the stable release of the new protobuf API. It wasn't an intentional choice, just how my workstation was configured.

Sure, better to have the last version committed

[slinkydeveloper]

Hi, is this PR still alive? @kconwayinvision can you fix DCO?

[kconwayinvision]

Hi, is this PR still alive? @kconwayinvision can you fix DCO?

It's still alive to me. DCO is only complaining about my lack of --signoff on the commits which is easy to fix. I still have a TODO to regenerate the protobuf code using the latest stable protoc. I was holding off a bit to see if folks had feedback or comments on the edge cases I called out in the PR. Some of the edge cases might be clearer if I also open a PR for the data codec because the ambiguity or complications in the spec are all related to a coupling between the format and codec. I'm confident the implementations will service setups with a protobuf format and protobuf data codec because I'm using it internally. It's the mixing and matching of format and codec that give me pause.

If it's easier, I can make the small adjustments for DCO and the protoc rebuild for merge and we can get into the edge case details in a second PR for the data codec.

[kconwayinvision]

While I was thinking about it, I went ahead and took care of the small things. Commit is now signed and DCO is happy. The protobuf code generation is redone using released versions of both protoc and protoc-gen-go.

[kconwayinvision]

Hey folks. Checking in to see what I can do to move this PR along. Any comments on the code for the format behavior I highlighted? Would it be easier to see this alongside with the data codec also?

[kconwayinvision]

Checking in again. If there is anything in particular folks are waiting for me to do then I'm not aware of it and need to be told again.

[slinkydeveloper]

cc @dan-j if that's ok for you, i'll merge

[dan-j]

LGTM 👍

[slinkydeveloper]

@kconwayinvision thanks for this amazing PR!

[kconwayinvision]

👍 I'll get a PR up for the data codec soon to complete the feature.