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

Switch to versioned ListOptions in server. #18655

Merged
merged 1 commit into from
Dec 22, 2015
Merged

Switch to versioned ListOptions in server. #18655

merged 1 commit into from
Dec 22, 2015

Conversation

wojtek-t
Copy link
Member

Part 2 of #18645

@wojtek-t wojtek-t self-assigned this Dec 14, 2015
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 14, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

@wojtek-t wojtek-t mentioned this pull request Dec 14, 2015
Closed
@k8s-bot
Copy link

k8s-bot commented Dec 14, 2015

GCE e2e test build/test passed for commit 5c1c2837eff3a00158dc47a96bf844fb146d7794.

@wojtek-t wojtek-t changed the title [WIP] Switch to versioned ListOptions in server. [WIP] [Do NOT merge] Switch to versioned ListOptions in server. Dec 14, 2015
@wojtek-t
Copy link
Member Author

@smarterclayton - the PR is almost ready.

The remaining issue is this test failing:
https://github.com/kubernetes/kubernetes/blob/master/pkg/master/master_test.go#L471

The issue is that ListOptions is not registered for the "company.com" API group, which I'm not sure how to solve properly (obviously we can register it in the test, but that doesn't solve the main problem here).
@smarterclayton - any suggestions?

@wojtek-t wojtek-t assigned smarterclayton and unassigned wojtek-t Dec 14, 2015
@smarterclayton
Copy link
Contributor

Let me try the test out from your branch - I think we want to allow this in conversion (third party is a bit of a hack anyway).

@smarterclayton
Copy link
Contributor

The assumption in resthandler (today) is that ListOptions exists in the group version, but third party doesn't provide a scope.Creater that is aware of ListOptions. master.go#1012 creates a thirdpartyresourcedata.NewObjectCreater - I think that method should create the appropriate delegate type (extensions/v1beta1#ListOptions), and conversion should work correctly at that point.

@wojtek-t
Copy link
Member Author

The assumption in resthandler (today) is that ListOptions exists in the group version, but third party doesn't provide a scope.Creater that is aware of ListOptions. master.go#1012 creates a thirdpartyresourcedata.NewObjectCreater - I think that method should create the appropriate delegate type (extensions/v1beta1#ListOptions), and conversion should work correctly at that point.

hmm - I'm not sure I understood.
We are already passing "api.Scheme" as delegate to NewObjectCreater:
https://github.com/kubernetes/kubernetes/blob/master/pkg/master/master.go#L1012
which should be aware of extensions/v1beta1#ListOptions and v1#ListOptions

I think that issue here is that we are passing "company.com#ListOptions" to "delegate" which api.Scheme cannot create an object for.

So maybe the correct fix is to add another case for ListOptions:
https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/thirdpartyresourcedata/codec.go#L294
matching ListOptions and change the group/version to extensions/v1beta1 or v1.

@smarterclayton WDYT?

@smarterclayton
Copy link
Contributor

Yes I think so - roughly what I was trying to convey (translate 3rd party
group into external group).

On Mon, Dec 14, 2015 at 1:05 PM, Wojciech Tyczynski <
notifications@github.com> wrote:

The assumption in resthandler (today) is that ListOptions exists in the
group version, but third party doesn't provide a scope.Creater that is
aware of ListOptions. master.go#1012 creates a
thirdpartyresourcedata.NewObjectCreater - I think that method should create
the appropriate delegate type (extensions/v1beta1#ListOptions), and
conversion should work correctly at that point.

hmm - I'm not sure I understood.
We are already passing "api.Scheme" as delegate to NewObjectCreater:

https://github.com/kubernetes/kubernetes/blob/master/pkg/master/master.go#L1012
which should be aware of extensions/v1beta1#ListOptions and v1#ListOptions

I think that issue here is that we are passing "company.com#ListOptions"
to "delegate" which api.Scheme cannot create an object for.

So maybe the correct fix is to add another case for ListOptions:

https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/thirdpartyresourcedata/codec.go#L294
matching ListOptions and change the group/version to extensions/v1beta1 or
v1.

@smarterclayton https://github.com/smarterclayton WDYT?

—
Reply to this email directly or view it on GitHub
#18655 (comment)
.

@wojtek-t
Copy link
Member Author

OK - thanks (I guess I didn't understand what you were suggesting).
I will fix it tomorrow.

@wojtek-t wojtek-t force-pushed the versioned_list_options_in_server branch from 5c1c283 to 7b94a68 Compare December 15, 2015 08:22
@k8s-bot
Copy link

k8s-bot commented Dec 15, 2015

GCE e2e test build/test passed for commit 7b94a6885fbc51ede9954b9e98d3bf2e24b57f9c.

@wojtek-t wojtek-t changed the title [WIP] [Do NOT merge] Switch to versioned ListOptions in server. Switch to versioned ListOptions in server. Dec 15, 2015
@wojtek-t
Copy link
Member Author

@smarterclayton - it's now ready to go

PTAL

@smarterclayton
Copy link
Contributor

LGTM thanks

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2015
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2015
@wojtek-t wojtek-t force-pushed the versioned_list_options_in_server branch from 7b94a68 to 42d7934 Compare December 17, 2015 09:36
@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed 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. labels Dec 17, 2015
@wojtek-t
Copy link
Member Author

Simple rebase - reapplying lgtm label

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api labels Dec 17, 2015
@wojtek-t wojtek-t force-pushed the versioned_list_options_in_server branch from 42d7934 to 967366f Compare December 21, 2015 07:17
@wojtek-t
Copy link
Member Author

Trivial rebase - reapplying lgtm label

@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed 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. labels Dec 21, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 21, 2015

GCE e2e test build/test passed for commit 967366f710e950fda0fe2deafff9eb6925cad929.

@wojtek-t wojtek-t force-pushed the versioned_list_options_in_server branch from 967366f to 5833682 Compare December 21, 2015 13:23
@wojtek-t
Copy link
Member Author

One more trivial rebase

@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Dec 21, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 21, 2015

GCE e2e test build/test passed for commit 5833682.

@k8s-github-robot
Copy link

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

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

1 similar comment
@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@k8s-bot
Copy link

k8s-bot commented Dec 21, 2015

GCE e2e test build/test passed for commit 5833682.

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

I'm still getting the following error:

git checkout -f cd3115aa5fb61c9c45658bb852d4dbe1c23d53be
FATAL: Could not checkout cd3115aa5fb61c9c45658bb852d4dbe1c23d53be
hudson.plugins.git.GitException: Could not checkout cd3115aa5fb61c9c45658bb852d4dbe1c23d53be

I've seen it in other PRs too - I opened #18960 for it

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

1 similar comment
@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@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 Dec 21, 2015

GCE e2e test build/test passed for commit 5833682.

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@k8s-github-robot
Copy link

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

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@k8s-bot
Copy link

k8s-bot commented Dec 22, 2015

GCE e2e test build/test passed for commit 5833682.

@wojtek-t
Copy link
Member Author

Tests just passed - merging manually to unblock further cleanup.

wojtek-t added a commit that referenced this pull request Dec 22, 2015
@wojtek-t
Merge pull request #18655 from wojtek-t/versioned_list_options_in_server
e108a32 
Switch to versioned ListOptions in server.
@wojtek-t wojtek-t merged commit e108a32 into kubernetes:master Dec 22, 2015
@wojtek-t wojtek-t deleted the versioned_list_options_in_server branch December 28, 2015 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@smarterclayton smarterclayton

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wojtek-t @k8s-github-robot @k8s-bot @smarterclayton @k8s-teamcity-mesosphere @googlebot