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

Set default_api_level=API_HYBRID for protobuf-go v1.36.0 #1635

Merged
merged 2 commits into from
Dec 24, 2024
Merged

Conversation

bufdev
Copy link
Member

@bufdev bufdev commented Dec 18, 2024

Open questions:

  • Does a new revision get automatically created for v1.36.0?
  • Should we delete the protoopaque files by wrapping protoc-gen-go and deleting them from the resulting CodeGeneratorResponse on the fly? I'd argue no, as while the build tag logic isn't our favorite, that's just our opinion, and we should match protoc-gen-go behavior unless there's a strong reason not to.

@bufdev
Copy link
Member Author

bufdev commented Dec 18, 2024

You could actually argue that we shouldn’t make this the default for remote plugins, but should for generated SDKs, so we may want to close this PR in the. end.

@bufdev bufdev changed the title Set default_api_level=API_HYBRID for protobuf-go v1.36.0 WIP DO NOT MERGE: Set default_api_level=API_HYBRID for protobuf-go v1.36.0 Dec 18, 2024
@mfridman
Copy link
Member

mfridman commented Dec 18, 2024

Yes, we'll get a revision bump and subsequent go get ... requests will generate hybrid (opaque+open) code.

I don't think we want to modify the response because there might be some users who explicitly want this. Also, we probably don't want to couple the generator output too closely with the implementation of Generated SDKs or deviate too much from the default behavior of protoc-gen-go.

If we were to go down this route, we'd be better off applying a patch to exclude those files from being generated in the first place. We have some prior art for git-based patches and custom builds.

We're in a predicament of having to pick exactly one sane default. Although I think default_api_level=API_HYBRID is the least-worst option because it satisfies the most amount of use cases, including the new OPAQUE API, which was not previously possible without .proto changes (which we actively discourage).

Unless @pkwarren @stefanvanburen have any objections I think we should roll forward with this.

@mfridman
Copy link
Member

mfridman commented Dec 18, 2024

I wonder if we should also bump grpc/go to depend on the updated protocolbuffers/go version?

@bufdev bufdev changed the title WIP DO NOT MERGE: Set default_api_level=API_HYBRID for protobuf-go v1.36.0 Set default_api_level=API_HYBRID for protobuf-go v1.36.0 Dec 24, 2024
@bufdev bufdev merged commit 3efffd8 into main Dec 24, 2024
5 checks passed
@bufdev bufdev deleted the api-hybrid branch December 24, 2024 14:45
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.

2 participants