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

Drop gogo support #89

Merged
merged 5 commits into from
Aug 17, 2021
Merged

Drop gogo support #89

merged 5 commits into from
Aug 17, 2021

Conversation

seena-stripe
Copy link
Collaborator

Summary

Removes gogo support from Skycfg

In moving to the new go protobuf v2 api, Skycfg will only be able to expose the types in a provided protoregsitry.Types, which accepts values https://pkg.go.dev/google.golang.org/protobuf/reflect/protoregistry#Types.RegisterMessage that implement https://pkg.go.dev/google.golang.org/protobuf@v1.27.1/reflect/protoreflect#MessageType.

However, https://github.com/gogo/protobuf (an alternative go code generator that Skycfg supported) does not support the new API (implementing MessageType and other interfaces). As we move Skycfg to using the new protobuf api, I don't see a way to support gogo and thus this PR drops it.

For any user interested in using Skycfg with gogo, Skycfg was tagged with v0.1.0 prior to the v2 api switch, and users can continue to use that version with gogo support, but with the old protobuf api.

Testing

I ran bazel build, test and go build on skycfg and everything compiles (after removing the gogo tests)

Copy link
Collaborator

@kathleen-stripe kathleen-stripe left a comment

Choose a reason for hiding this comment

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

just a small nit

README.md Outdated
creates will handle the conversion of these types.
Skycfg has dropped support for
[gogo/protobuf](https://github.com/gogo/protobuf), an alternative protobuf go
code generator, when upgrading to using the go-protobuf v2 `pi. For using gogo
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: typo in "`pi" - I'm assuming that's supposed to be "api."

@seena-stripe
Copy link
Collaborator Author

Nice, great catch

@seena-stripe seena-stripe merged commit 3410450 into trunk Aug 17, 2021
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