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

Add v1alpha1 generated protos #2

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Add v1alpha1 generated protos #2

merged 2 commits into from
Dec 3, 2019

Conversation

johanstokking
Copy link
Contributor

@johanstokking johanstokking commented Nov 24, 2019

Summary

Closes #1

Changes

Notes for Reviewers

Please have a look at the tooling and how things are being generated, like this one https://github.com/packetbroker/go-api/pull/2/files#diff-f6797f8393b8812416981fa126276a50

We don't have version dependencies for protoc and gogofaster. The only way to do that right seems to be what we do in The Things Stack, but I consider that overkill for now.

@johanstokking johanstokking added this to the November 2019 milestone Nov 24, 2019
@johanstokking johanstokking self-assigned this Nov 24, 2019
@johanstokking johanstokking force-pushed the feature/v1alpha1 branch 2 times, most recently from 247ca21 to 0532e14 Compare November 24, 2019 12:27
@htdvisser
Copy link
Contributor

  1. I don't think the tooling really matters at this point. I think it's important to choose between golang/protobuf and gogo/protobuf because that will have implications for all server and client implementations, and as a consequence, will be relatively difficult to change in the future, although that would be fine for v1alpha2 and later, since those will have a different import path anyway.
  2. The protoc indeed doesn't make much of a difference, but the version of the Go generator tends to change. I explored this in a sideproject and decided to try a go generate that downloads and installs (see generator.go) the dependencies and tooling (see tools.go). Because it's in a Go module, those will be the version that I expect.
  3. We may want to put go.mod in v1alpha1


.PHONY: deps
deps:
$(GO) get github.com/gogo/protobuf/protoc-gen-gogofaster
Copy link
Contributor

Choose a reason for hiding this comment

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

protoc-gen-gogofaster eliminates XXX_unrecognized fields, which I think we should keep for forwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

protoc-gen-gogofaster eliminates XXX_unrecognized fields, which I think we should keep for forwards compatibility.

Yeah I considered this. It can cause unwanted side-effects in the future, for example when storing marshaled protos that may change future behavior and validation. I'm neutral, I don't see the benefit. Clients should be explicitly talking to a versioned API endpoint anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was more thinking about supporting a chain of forwarders that have slightly different versions. If we add a response field to the API (assuming it doesn't change behavior) it would be nice if that field wouldn't be dropped by a forwarder that isn't updated yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that means that an intermediate forwarder receives messages from v1alpha2 and sends it as v1alpha1; those are not compatible anyway, right? When mapping those types, the v1alpha2 specific fields get lost, but I'm not sure in which case we could retain that realistically? Copying XXX_unrecognized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can resolve this later

@johanstokking johanstokking marked this pull request as ready for review December 3, 2019 18:09
@johanstokking johanstokking merged commit dc5222e into master Dec 3, 2019
@johanstokking johanstokking deleted the feature/v1alpha1 branch December 3, 2019 18:33
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.

Development tooling to generate API
2 participants