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

Use protobuf/jsonpb instead of golang json in some cases #178

Merged
merged 1 commit into from
Nov 8, 2017

Conversation

miaoyq
Copy link
Contributor

@miaoyq miaoyq commented Nov 4, 2017

For #154

/cc @Random-Liu

Signed-off-by: Yanqiang Miao miao.yanqiang@zte.com.cn

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 4, 2017
@@ -0,0 +1,829 @@
// Go support for Protocol Buffers - Google's data interchange format
Copy link
Member

Choose a reason for hiding this comment

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

This is a vendor update, does vendor.conf need update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only update jsonpb here, I think we dont need update vendor.conf unless other model depend on the new version of protobuf. :)

@Random-Liu
Copy link
Contributor

@miaoyq How do we handle extra information then?

@miaoyq
Copy link
Contributor Author

miaoyq commented Nov 5, 2017

@Random-Liu We could handle extra info via "outputStatusInfo"

@miaoyq
Copy link
Contributor Author

miaoyq commented Nov 5, 2017

@Random-Liu "protobufObjectToJSON(status)" handles resp.Status, and "json.Marshal" handles resp.Info

@feiskyer
Copy link
Member

feiskyer commented Nov 6, 2017

"protobufObjectToJSON(status)" handles resp.Status, and "json.Marshal" handles resp.Info

LGTM. @Random-Liu do you have any other concerns?

@miaoyq
Copy link
Contributor Author

miaoyq commented Nov 6, 2017

"protobufObjectToJSON(status)" handles resp.Status, and "json.Marshal" handles resp.Info

LGTM. @Random-Liu do you have any other concerns?

@Random-Liu @feiskyer Updated, also related to #166.

Copy link
Contributor

@Random-Liu Random-Liu left a comment

Choose a reason for hiding this comment

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

LGTM with one nit.

if err != nil {
return "", err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unnecessary empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Random-Liu
Copy link
Contributor

@miaoyq For some other commands, we also support yaml/json output. We should also use jsonpb for those cases. I'm fine with doing that in another PR, but please make sure the issue is open.

@miaoyq
Copy link
Contributor Author

miaoyq commented Nov 7, 2017

@miaoyq For some other commands, we also support yaml/json output. We should also use jsonpb for those cases. I'm fine with doing that in another PR, but please make sure the issue is open.

Will do.

Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>
@Random-Liu
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2017
@Random-Liu Random-Liu merged commit b08a4b7 into kubernetes-sigs:master Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants