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 support for proto.to_any #46

Closed
wants to merge 4 commits into from

Conversation

michaelkipper
Copy link

Re: #45

This simple PR adds support for a proto.to_any() function, so we can create schema agnostic protobuf messages with Skycfg.

@michaelkipper
Copy link
Author

@jmillikin-stripe: What do you think about this PR?

if "" != proto.MessageName(msg.msg) {
// Returns a golang any.Any type.
any, err = ptypes.MarshalAny(msg.msg)
} else if "" != gogo_proto.MessageName(msg.msg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably only go here if msg's package has a gogo: prefix to be consistent with previous changes in gogocompat.

Previously @jmillikin-stripe also insisted on gogo dependencies going into separate package to avoid hard dependency within core implementation.

// Returns a gogo types.Any type.
any, err = gogo_types.MarshalAny(msg.msg)
} else {
return nil, fmt.Errorf("%s: could not get message name for %s", "proto.to_any", msg.Type())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("%s: could not get message name for %s", "proto.to_any", msg.Type())
return nil, fmt.Errorf("%s: could not get message name for %s", fn.Name(), msg.Type())

// skyProtoMessage with an `Any` proto.Message in it.
func fnProtoToAny(t *starlark.Thread, fn *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
var msg *skyProtoMessage
err := wantSingleProtoMessage("proto.to_any", args, kwargs, &msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err := wantSingleProtoMessage("proto.to_any", args, kwargs, &msg)
err := wantSingleProtoMessage(fn.Name(), args, kwargs, &msg)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Michael Kipper seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@idiamond-stripe
Copy link
Contributor

Hit this today when trying to use the Envoy API in skycfg. The Envoy Filter message takes a google.protobuf.Any, which isn't currently possible to implement using Skycfg [0]. This typed config is one of the options in the extensions/ subdirectory, for example HTTPConnectionManager [1].

It's not currently possible to work around as it's not possible to marshal Skycfg to a protobuf so you can put it into the value block of google.protobuf.Any, nor introspect the type name.

[0] https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/listener/listener_components.proto#L29-L43
[1] https://github.com/envoyproxy/data-plane-api/blob/master/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#L41

@jmillikin-stripe
Copy link
Contributor

Replaced by #83

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.

5 participants