-
Notifications
You must be signed in to change notification settings - Fork 39.7k
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
godeps: update go-restful #30914
Conversation
cc @mbohlool |
LGTM. @nikhiljindal @mbohlool do you have comments? |
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 :) |
@lavalamp this is part of the golang encoding/json standard. Validation is broken without this. |
E.g. we can't create csrs from kubectl without --validate=false |
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. |
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. |
What if somebody really means byte array? I guess array of integer could be wrong there too, but it is closer, right? |
@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
It's not optional. To encode a byte array as an array of ints, you would have to write your own json encoder. |
That's not the part I'm waffling over-- it's the conflation between On Thu, Aug 18, 2016 at 2:27 PM, Mike Danese [email protected]
|
related to this: OAI/OpenAPI-Specification#50 |
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. |
GCE e2e build/test passed for commit d086e98. |
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? |
Yes I agree this is at least an improvement. /lgtm On Thu, Aug 18, 2016 at 3:17 PM, Mike Danese [email protected]
|
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. |
Thanks for review! |
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. |
This PR is not for the master branch but does not have the |
Why is @k8s-merge-robot saying this? |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit d086e98. |
Automatic merge from submit-queue |
To pickup emicklei/go-restful#311
@kubernetes/sig-api-machinery
This change is