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

Preserve API group order in discovery, prefer extensions over apps #43553

Merged
merged 1 commit into from
Mar 23, 2017
Merged

Preserve API group order in discovery, prefer extensions over apps #43553

merged 1 commit into from
Mar 23, 2017

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Mar 23, 2017 •
edited
Loading

Fixes #42392, supercedes #43543

Kubectl 1.5 still uses compiled in types in kubectl edit and apply.

Because kubectl 1.5 does not have the apps/v1beta1/deployment resource compiled in, the preferred group order must have extensions come before apps. The preference order is determined by the order that the groups are listed by the discovery service, with the first elements preferred over the last elements.

This PR:

This has the side benefit of making all TPR API groups (regardless of group name) come after the core API groups, instead of potentially appearing earlier in discovery order

The API server discovery document now prioritizes the `extensions` API group over the `apps` API group. This ensures certain commands in 1.5 versions of kubectl (such as `kubectl edit deployment`) continue to function against a 1.6 API.

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 23, 2017
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 23, 2017
@pwittrock pwittrock self-assigned this Mar 23, 2017
@liggitt liggitt mentioned this pull request Mar 23, 2017
Closed
@liggitt liggitt added this to the v1.6 milestone Mar 23, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 23, 2017

cc @kubernetes/sig-api-machinery-pr-reviews @deads2k @lavalamp

@liggitt liggitt added area/apiserver area/kubectl kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Mar 23, 2017
@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 23, 2017
pwittrock
pwittrock reviewed Mar 23, 2017
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go Outdated
}
sort.Strings(groupNames)
for _, groupName := range groupNames {
for _, groupName := range s.apiGroupNamesForDiscovery {
Copy link
Member

@pwittrock pwittrock Mar 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Consider adding a comment that this maintains the insertion order.

Copy link
Member Author

Choose a reason for hiding this comment

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

comment added

@pwittrock
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2017
@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 Mar 23, 2017
@ethernetdan
Copy link
Contributor

@bgrant0607 @lavalamp @wojtek-t could you PTAL at this release blocker?

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 23, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 23, 2017

@pwittrock @ymqytw wasn't sure if you wanted a release note on this PR or if you were planning to add them in manually... feel free to edit or remove the one I put in the description

@liggitt liggitt removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 23, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 23, 2017

@deads2k would like your take on how (or if) you see this interacting with the 1.7 registration stuff you have in progress (or if the aggregator bypasses this)

@deads2k
Copy link
Contributor

deads2k commented Mar 23, 2017

@deads2k would like your take on how (or if) you see this interacting with the 1.7 registration stuff you have in progress (or if the aggregator bypasses this)

It will be ok. Minor fitting pains, but nothing severe.

@deads2k
Copy link
Contributor

deads2k commented Mar 23, 2017

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 23, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 23, 2017 •
edited
Loading

added comment, retagging
@lavalamp for approval

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2017
@pwittrock
Copy link
Member

lgtm

@pwittrock
Copy link
Member

@liggitt Updated the release note to be focussed on what to expect when upgrading a cluster from 1.5 -> 1.6

bgrant0607
bgrant0607 reviewed Mar 23, 2017
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go
sort.Strings(groupNames)
for _, groupName := range groupNames {

// ranging over apiGroupNamesForDiscovery preserves the registration order
Copy link
Member

Choose a reason for hiding this comment

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

Please document that the order is preserved in the function description, also.

Copy link
Member

Choose a reason for hiding this comment

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

+1 Add a godoc to AddAPIGroupForDiscovery that the call order is significant.

Copy link
Member Author

Choose a reason for hiding this comment

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

opened #43623 with doc

@bgrant0607
Copy link
Member

@liggitt @pwittrock

I like this much better, thanks. One minor comment. Given the time crunch, I'm ok with it not being addressed.

/approve

cc @caesarxuchao

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgrant0607, liggitt, pwittrock

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 23, 2017

will open a follow up with additional godoc once master reopens

@bgrant0607
Copy link
Member

ref #43542

@bgrant0607
Copy link
Member

Thanks, @liggitt

@lavalamp
Copy link
Member

Yeah, thanks, this is much better.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b1e665f into kubernetes:master Mar 23, 2017
@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 23, 2017
@k8s-github-robot
Copy link

@liggitt PR needs rebase

@liggitt liggitt mentioned this pull request Mar 23, 2017
Closed
@janetkuo janetkuo mentioned this pull request Mar 23, 2017
Merged
@liggitt liggitt deleted the discovery-order branch March 24, 2017 14:09
@soltysh soltysh mentioned this pull request Feb 13, 2020
Closed
soltysh added a commit to soltysh/kubernetes that referenced this pull request Feb 13, 2020
@soltysh
Prefer apps version over deprecated extensions
c0acc54 
Due to backwards compatibility issues
(kubernetes#42392)
introduced in 1.6 when adding new API group we've set extensions to be
preferred group over apps
(kubernetes#43553).
Now that extensions is entirely deprecated it's time to revert this
change and prefer correct API groups.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@bgrant0607 bgrant0607 bgrant0607 left review comments

@pwittrock pwittrock pwittrock left review comments

Assignees

@lavalamp lavalamp

@yifan-gu yifan-gu

@pwittrock pwittrock

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-blocker release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@liggitt @k8s-reviewable @pwittrock @ethernetdan @deads2k @bgrant0607 @k8s-github-robot @lavalamp @yifan-gu @k8s-ci-robot