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.

Sign up for GitHub

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

Jump to bottom

Correct versioned.Event output in Swagger #33007

Merged
merged 2 commits into from
Sep 22, 2016
Merged

Correct versioned.Event output in Swagger #33007

merged 2 commits into from
Sep 22, 2016

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Sep 18, 2016 •
edited by k8s-oncall

Fixes #24240

The value of the `versioned.Event` object (returned by watch APIs) in the Swagger 1.2 schemas has been updated from `*versioned.Event` which was not expected by many client tools. The new value is consistent with other structs returned by the API.

This change is Reviewable

@smarterclayton
Copy link
Contributor Author

@nikhiljindal @mbohlool

@smarterclayton smarterclayton added area/api Indicates an issue on api area. area/swagger labels Sep 18, 2016
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Sep 18, 2016
@smarterclayton smarterclayton mentioned this pull request Sep 18, 2016
Closed
@smarterclayton
Copy link
Contributor Author

I think this is a cherry-pick candidate for 1.4.x because it will block integrators

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2016
@nikhiljindal
Copy link
Contributor

nikhiljindal commented Sep 19, 2016 •
edited

LGTM, thanks!
@smarterclayton Feel free to self apply the label after rebasing

I think this is a cherry-pick candidate for 1.4.x because it will block integrators

Sorry what do you mean by integrators?

@smarterclayton
Copy link
Contributor Author

Anyone using our swagger spec to do anything useful (the *versioned.Event broke a few generators I tried).

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2016
@k8s-bot
Copy link

k8s-bot commented Sep 20, 2016

GCE e2e build/test passed for commit 8fd096e.

@smarterclayton smarterclayton added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Sep 20, 2016
@mbohlool
Copy link
Contributor

LGTM too, I saw this among many validation failures in swagger 1.2. Assumed moving to swagger 2.0 will fixed it (briefly checked 2.0, it looks fine. The 2.0 spec is a valid spec that I can generate clients from).

@smarterclayton
Copy link
Contributor Author

@k8s-bot unit test this issue #33184

@timothysc timothysc mentioned this pull request Sep 21, 2016
Closed
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f230e6c into kubernetes:master Sep 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mbohlool mbohlool

@nikhiljindal nikhiljindal

Labels
area/api Indicates an issue on api area. area/swagger lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@smarterclayton @nikhiljindal @k8s-bot @mbohlool @k8s-github-robot @googlebot