CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Use typeurl.Any instead of github.com/gogo/protobuf/types.Any #6706
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
Conversation
Skipping CI for Draft Pull Request. |
@mxpv @dmcgowan Regarding exported methods that take Any, like https://github.com/containerd/typeurl/blob/fc3423ac466f5ea0956f96572993fbac5afa3233/types.go#L146-L160, does it make sense to take Any as an interface? All we need is TypeURL and Value. gogo/protobuf and google.golang.org/protobuf can agree about the signature.
The Get prefix and casing (Url instead of URL) are less Go-idiomatic though. |
Build succeeded.
|
@kzys yeah, that is a good idea |
Much simpler containerd/typeurl#32 |
db2b205
to
18212ad
Compare
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
36512bc
to
8a586e4
Compare
Build succeeded.
|
Build succeeded.
|
c285162
to
cf17c36
Compare
// typeurl.MarshalAny currently returns &types.Any, but it will | ||
// return typeurl.Any soon. | ||
pbany := &types.Any{ | ||
TypeUrl: any.GetTypeUrl(), | ||
Value: any.GetValue(), | ||
} | ||
|
||
for _, id := range imgcrypt.PayloadToolIDs { | ||
c.ProcessorPayloads[id] = any | ||
c.ProcessorPayloads[id] = pbany | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the local change I made. If that looks okay, I'm going to open a PR against imgcrypt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Opened containerd/imgcrypt#72.
@@ -284,7 +285,7 @@ func (c *container) NewTask(ctx context.Context, ioCreate cio.Creator, opts ...N | |||
if err != nil { | |||
return nil, err | |||
} | |||
request.Options = any | |||
request.Options = protobuf.FromAny(any) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request (CreateTaskRequest) is generated from a proto. So we have to call FromAny() here.
3089cc6
to
c9a0904
Compare
Build succeeded.
|
7cae71d
to
231128c
Compare
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
This is finally ready for review! While there are still gogo/protobuf's Any here and there, I don't want to increase the size of this PR anymore. |
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
This commit upgrades github.com/containerd/typeurl to use typeurl.Any. The interface hides gogo/protobuf/types.Any from containerd's Go client. Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This commit upgrades github.com/containerd/typeurl to use typeurl.Any.
The interface hides gogo/protobuf/types.Any from containerd's Go client.
Signed-off-by: Kazuyoshi Kato katokazu@amazon.com