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

Updating go-restful to generate "type":"object" instead of "type":"any" in swagger-spec (breaks kubectl 1.1) #22897

Merged
merged 3 commits into from
Apr 15, 2016

Conversation

nikhiljindal
Copy link
Contributor

Updating go-restful to include emicklei/go-restful#270 (replacing type "any" by type "object". Ref swagger-api/swagger-codegen#2347 on why we want to do that)
Ref #4700 (comment)

First commit generated using:

godep restore
go get -u github.com/emicklei/go-restful
godep update github.com/emicklei/go-restful

Second commit generated by running

./hack/update-swagger-spec.sh

Third commit generated by running:

./hack/update-api-reference-docs.sh

cc @kubernetes/sig-api-machinery @bgrant0607

@nikhiljindal
Copy link
Contributor Author

We can wait for 1.2 to wrap up before merging this to prevent cherrypick conflicts.

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 12, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 12, 2016

GCE e2e build/test failed for commit 94b73bf0765c81c069bc2a8628f19a8db4225064.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-github-robot
Copy link

@nikhiljindal PR needs rebase

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

k8s-bot commented Mar 15, 2016

GCE e2e build/test failed for commit 23e90013ea980a5e9cb3d392938e85b56674740f.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 15, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 15, 2016

GCE e2e build/test failed for commit 7482f352f20b7251c032a4d9b2fa5b50c79444b1.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-github-robot
Copy link

@nikhiljindal PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2016
@ghost ghost added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 16, 2016
@ghost
Copy link

ghost commented Mar 16, 2016

LGTM @nikhiljindal, but needs a rebase. We can remove the do-not-merge label as soon as that's done, and v1.2 has been branched (i.e. in the next few hours).

@ghost ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2016
@nikhiljindal nikhiljindal removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 17, 2016
@nikhiljindal
Copy link
Contributor Author

Rebased. Removing the needs-rebase and do-not-merge tags.

@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2016
@nikhiljindal
Copy link
Contributor Author

Adding back LGTM since its just a rebase

@nikhiljindal nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 17, 2016

GCE e2e build/test failed for commit f74de42e8d258597b95e547eecbde501350d5f09.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Mar 17, 2016

GCE e2e build/test failed for commit 8683ff8f735c0513f8c060d7a52837f9c7e09b6b.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@nikhiljindal nikhiljindal removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2016
@nikhiljindal
Copy link
Contributor Author

#24054 is now submitted and cherrypicked in 1.2 so this PR is ready to go.

Rebased and the first commit is gone as #24129 updated go-restful.

This is now ready to go in and should go in soon since swagger spec is now broken on HEAD.
#24129 updated go-restful but did not update the spec (which should have been caught by the verify-swagger-spec script, if not for #24233).
Adding back the LGTM tag, since this was a trivial rebase.

@nikhiljindal nikhiljindal 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. labels Apr 15, 2016
@nikhiljindal nikhiljindal changed the title Updating go-restful Updating go-restful to generate "type":"object" instead of "type":"any" in swagger-spec Apr 15, 2016
@nikhiljindal nikhiljindal added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-label-needed labels Apr 15, 2016
@nikhiljindal nikhiljindal changed the title Updating go-restful to generate "type":"object" instead of "type":"any" in swagger-spec Updating go-restful to generate "type":"object" instead of "type":"any" in swagger-spec (breaks kubectl 1.1) Apr 15, 2016
@nikhiljindal
Copy link
Contributor Author

Updated the title to reflect better release notes

@nikhiljindal nikhiljindal added this to the v1.3 milestone Apr 15, 2016
@nikhiljindal nikhiljindal added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 15, 2016
@nikhiljindal
Copy link
Contributor Author

cc @k8s-oncall since ./hack/verify-all.sh is currently broken on HEAD (but passes in our CI tests on jenkins and travis)

@k8s-bot
Copy link

k8s-bot commented Apr 15, 2016

GCE e2e build/test passed for commit 6eae11e.

@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 Apr 15, 2016

GCE e2e build/test passed for commit 6eae11e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f4473af into kubernetes:master Apr 15, 2016
@ikehz
Copy link
Contributor

ikehz commented Apr 28, 2016

breaks kubectl 1.1

Our versioning policy states that we'll support back two minor versions, so v1.3 should support v1.1. Why did we decide this was okay?

@nikhiljindal
Copy link
Contributor Author

I discussed this with @bgrant0607 and he pointed out that that policy doesnt talk about kubectl version skew.
Users are going to have a bad time if they try to use 1.1 kubectl with 1.3 apiserver since we have changed a lot of things.

I am not sure if any of our docs explicitly talk about kubectl and master version skew.
Also, am not sure if validation was on by default in kubectl 1.1.
Will find out and reply.

@bgrant0607
Copy link
Member

@ihmccreery Our version-skew policy is not clearly documented, and neither is the meaning of "support", but the "Supported Releases" section of that document is definitely not about version-skewed operation. See https://github.com/kubernetes/kubernetes/blob/master/docs/design/versioning.md#upgrades, for instance.

Practically speaking, kubectl even now isn't friendly to version-skewed operation. The most egregious problem is round-tripping #3955, which we plan to address in 1.3, but which means that any version of kubectl prior to 1.3 will still have that problem.

@ikehz
Copy link
Contributor

ikehz commented Apr 29, 2016

Seems to me the implicit assertion here is that we support kubectl only one minor version back. If so, we should document it as such. This is going to cause pain when we release v1.3 for anyone still running a v1.1 kubectl. Admittedly, no one "should" be, but...

@ikehz
Copy link
Contributor

ikehz commented May 3, 2016

cc @kubernetes/goog-gke @roberthbailey

@roberthbailey
Copy link
Contributor

I think the versioning policy was primarily created to account for the version skew between the master components and nodes. The best thing would be to make kubectl fail gracefully when it's incompatible with the apiserver and tell the user (kindly, without an arcane error or stack trace) that they should update. Supporting skew between the latest patch release of version 1.x and the new release of 1.y is important, because people will generally update one part of their system before the other (CLI tool / apiserver), but I don't think we need nearly as a wide a range as we intend for nodes. Also note that updating your CLI tool is quick and painless and doesn't have the side effect of possibly causing down time for your application. So encouraging updating it should be the default.

k8s-github-robot pushed a commit that referenced this pull request May 9, 2016
Automatic merge from submit-queue

Clarify supported version skew between masters, nodes, and clients

Per discussion on #22897.

cc @bgrant0607 @roberthbailey
@erictune
Copy link
Member

erictune commented Jul 2, 2016

@nikhiljindal Does this PR require action by the user when upgrading to 1.3.0? (Think about non-developer users.) If so, please edit your first comment to have a release-note block, like in #28132. If not, please change to release-note-none.

@erictune
Copy link
Member

erictune commented Jul 6, 2016

@nikhiljindal if this is a breaking change, please update CHANGELOG.md.

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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants