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

godeps: update go-restful #30914

Merged
merged 2 commits into from
Aug 21, 2016
Merged

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented Aug 18, 2016

To pickup emicklei/go-restful#311

@kubernetes/sig-api-machinery


This change is Reviewable

@mikedanese mikedanese added this to the v1.4 milestone Aug 18, 2016
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Aug 18, 2016
@caesarxuchao
Copy link
Member

cc @mbohlool

@lavalamp lavalamp assigned mbohlool and unassigned caesarxuchao and thockin Aug 18, 2016
@caesarxuchao
Copy link
Member

LGTM. @nikhiljindal @mbohlool do you have comments?

@lavalamp
Copy link
Member

I'm a bit confused as to whether this is a desirable change or not. If we expect base64 data, reporting that as "string" in swagger is probably unhelpful? @mbohlool to decide :)

@mikedanese
Copy link
Member Author

@lavalamp this is part of the golang encoding/json standard. Validation is broken without this.

@mikedanese
Copy link
Member Author

E.g. we can't create csrs from kubectl without --validate=false

@mikedanese
Copy link
Member Author

The field doc should state that the data is base 64 encoded as is the case for CSR Request field. https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/certificates/v1alpha1/types.go#L43

Currently the swagger spec says array of integer for this which is completely wrong. This is a strict improvement.

@nikhiljindal
Copy link
Contributor

I agree that string is an improvement over int array :)

@mikedanese You will have to run hack/update-api-reference-docs.sh to update api reference docs based on the swagger spec update.

@mikedanese mikedanese added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 18, 2016
@mbohlool
Copy link
Contributor

What if somebody really means byte array? I guess array of integer could be wrong there too, but it is closer, right?
Another solution to this is to define a "type Something []byte" and customize the swagger type for it. You can follow the model of SchemaFormatHandler for Types.

@mikedanese
Copy link
Member Author

mikedanese commented Aug 18, 2016

@mbohlool In golang the json represntation of a []byte is a base64 encoded string. Both encoding/json and ugorji respect this.

From https://godoc.org/encoding/json

Array and slice values encode as JSON arrays, except that []byte encodes as a base64-encoded string, and a nil slice encodes as the null JSON value.

It's not optional. To encode a byte array as an array of ints, you would have to write your own json encoder.

@lavalamp
Copy link
Member

That's not the part I'm waffling over-- it's the conflation between
"string" and "base64 encoded data stored in a string", which are not the
same types at all.

On Thu, Aug 18, 2016 at 2:27 PM, Mike Danese notifications@github.com
wrote:

@mbohlool https://github.com/mbohlool In golang the json represntation
of a []byte is a base64 encoded string. Both encoding/json and ugorji
respect this.

From https://godoc.org/encoding/json

Array and slice values encode as JSON arrays, except that []byte encodes
as a base64-encoded string, and a nil slice encodes as the null JSON value.

It's not optional. To encode a byte array as an array of ints, you would
have to write your own json deserializer.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30914 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglt1m-KJdOQQsnn1nl7KfVskNl1Vwks5qhM5WgaJpZM4Jnx5X
.

@mbohlool
Copy link
Contributor

related to this: OAI/OpenAPI-Specification#50

@mbohlool
Copy link
Contributor

in open-api/swagger 2.0, this should be represented as type="string", format="byte" to distinct two cases. I guess some swagger 1.2 tools will understand that too.

@k8s-bot
Copy link

k8s-bot commented Aug 18, 2016

GCE e2e build/test passed for commit d086e98.

@mikedanese
Copy link
Member Author

That's not the part I'm waffling over-- it's the conflation between
"string" and "base64 encoded data stored in a string", which are not the
same types at all.

I'm all for making this type doc more explicit but swagger validation is expecting an int array right now, which is not even possible to pass in for a []byte since go won't deserialize an int array to a []byte field. Swagger spec generator and golang stdlib json spec have a mismatch between the mapping of go types to json types. This fixes that and fixes a bug that breaks the kubectl experience for an alpha feature we want to release in 1.4 kubernetes/enhancements#43 .

Do you agree that "int array" is worse then "string" in swagger spec to represent "string that is base64 encoded bytes"? Can we improve this doc incrementally so as to fix a bug for the v1.4 release?

@lavalamp
Copy link
Member

Yes I agree this is at least an improvement.

/lgtm

On Thu, Aug 18, 2016 at 3:17 PM, Mike Danese notifications@github.com
wrote:

That's not the part I'm waffling over-- it's the conflation between
"string" and "base64 encoded data stored in a string", which are not the
same types at all.

I'm all for making this type doc more explicit but swagger validation is
expecting an int array right now, which is not even possible to pass in for
a []byte since go won't deserialize an int array to a []byte field. Swagger
spec generator and golang stdlib json spec have a mismatch between the
mapping of go types to json types. This fixes that and fixes a bug that
breaks the kubectl experience for an alpha feature we want to release in
1.4 kubernetes/enhancements#43
kubernetes/enhancements#43 .

Do you agree that "int array" is worse then "string" in swagger spec to
represent "string that is base64 encoded bytes"? Can we improve this doc
incrementally so as to fix a bug for the v1.4 release?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30914 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAngljiQJDOBYNmFKxEiEd5yfF13UkLVks5qhNoCgaJpZM4Jnx5X
.

@mbohlool
Copy link
Contributor

I also agree that this is an improvement, I think we should set the type to "byte" too and definitely fix this for swagger 2.0 generation, if it is already an issue. but this PR looks good to me.

@mbohlool mbohlool added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2016
@mikedanese
Copy link
Member Author

Thanks for review!

@mbohlool
Copy link
Contributor

You are welcome. I also meant to set the format to "byte" not the type. The type should stay as string. Thanks for the fix.

@k8s-github-robot
Copy link

This PR is not for the master branch but does not have the cherrypick-approved label. Adding the do-not-merge label.

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 20, 2016
@mikedanese mikedanese removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 20, 2016
@mikedanese
Copy link
Member Author

Why is @k8s-merge-robot saying this?

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 21, 2016

GCE e2e build/test passed for commit d086e98.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants