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

use specified discovery information if possible #50012

Merged
merged 1 commit into from
Sep 15, 2017
Merged

use specified discovery information if possible #50012

merged 1 commit into from
Sep 15, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Aug 2, 2017 •
edited by liggitt
Loading

Fixes #49948

This uses the available discovery information if available, but it seems we never updated "normal" resources to show the singular name, so its often not available. I've left this code compatible.

@enisoc @ash2k
@kubernetes/sig-api-machinery-misc

custom resources that use unconventional pluralization now work properly with kubectl and garbage collection

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 2, 2017
@deads2k deads2k mentioned this pull request Aug 2, 2017
Closed
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Aug 2, 2017
@ncdc
Copy link
Member

ncdc commented Aug 2, 2017

Is this easily unit testable?

ash2k
ash2k reviewed Aug 2, 2017
View reviewed changes
staging/src/k8s.io/client-go/discovery/restmapper.go Outdated
// TODO only do this if it supports listing
versionMapper.Add(gv.WithKind(resource.Kind+"List"), scope)
versionMapper.AddSpecific(gv.WithKind(resource.Kind+"List"), plural, singular, scope)
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/kubernetes/apiextensions-apiserver/blob/master/pkg/apis/apiextensions/v1beta1/types.go#L46

What if ListKind is not Kind + "List"? Looks like APIResource struct does not expose this information so we cannot use it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if ListKind is not Kind + "List"? Looks like APIResource struct does not expose this information so we cannot use it here.

Hrm. Not fixable in this pull.

@deads2k
Copy link
Contributor Author

deads2k commented Aug 2, 2017

Is this easily unit testable?

I delegated one to the other, so the existing callers test the core functionality. The actual discovery bits? Not easily. An integration test can't, an e2e test maybe, but it would be a disruptive one (affects global discovery).

@ncdc
Copy link
Member

ncdc commented Aug 2, 2017

You don't think you could unit test NewRESTMapper?

@mml
Copy link
Contributor

mml commented Aug 3, 2017

/assign mbohlool

@liggitt
Copy link
Member

liggitt commented Aug 7, 2017

The actual discovery bits? Not easily. An integration test can't, an e2e test maybe, but it would be a disruptive one (affects global discovery).

@ironcladlou has an integration test that could be made to exercise this :)

@caesarxuchao caesarxuchao assigned ironcladlou and unassigned vishh and tallclair Aug 7, 2017
@ironcladlou
Copy link
Contributor

Here's an integration test that exercises discovery and which will fail whenever the generated CRD's singular name ends with an "s".

@liggitt liggitt added this to the v1.7 milestone Aug 8, 2017
liggitt
liggitt reviewed Aug 8, 2017
View reviewed changes
staging/src/k8s.io/client-go/discovery/restmapper.go Outdated
// TODO only do this if it supports listing
versionMapper.Add(gv.WithKind(resource.Kind+"List"), scope)
versionMapper.AddSpecific(gv.WithKind(resource.Kind+"List"), plural, singular, scope)
Copy link
Member

Choose a reason for hiding this comment

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

adding the Kind+List to the non-list singular/plural resources doesn't look right... the Add of the Kind+"List" previously did the following:

Add(schema.GroupVersionKind{Group:"mygroup.example.com", Version:"v1beta1", Kind:"MyObj"})
	m.singularToPlural[{mygroup.example.com v1beta1 myobj}] = {mygroup.example.com v1beta1 myobjs}
	m.pluralToSingular[{mygroup.example.com v1beta1 myobjs}] = {mygroup.example.com v1beta1 myobj}

	m.resourceToKind[{mygroup.example.com v1beta1 myobj}] = mygroup.example.com/v1beta1, Kind=MyObj
	m.resourceToKind[{mygroup.example.com v1beta1 myobjs}] = mygroup.example.com/v1beta1, Kind=MyObj

	m.kindToPluralResource[mygroup.example.com/v1beta1, Kind=MyObj] = {mygroup.example.com v1beta1 myobjs}
	m.kindToScope[mygroup.example.com/v1beta1, Kind=MyObj] = namespace

Add(schema.GroupVersionKind{Group:"mygroup.example.com", Version:"v1beta1", Kind:"MyObjList"})
	m.singularToPlural[{mygroup.example.com v1beta1 myobjlist}] = {mygroup.example.com v1beta1 myobjlists}
	m.pluralToSingular[{mygroup.example.com v1beta1 myobjlists}] = {mygroup.example.com v1beta1 myobjlist}

	m.resourceToKind[{mygroup.example.com v1beta1 myobjlist}] = mygroup.example.com/v1beta1, Kind=MyObjList
	m.resourceToKind[{mygroup.example.com v1beta1 myobjlists}] = mygroup.example.com/v1beta1, Kind=MyObjList

	m.kindToPluralResource[mygroup.example.com/v1beta1, Kind=MyObjList] = {mygroup.example.com v1beta1 myobjlists}
	m.kindToScope[mygroup.example.com/v1beta1, Kind=MyObjList] = namespace

with this change, it does:

Add(schema.GroupVersionKind{Group:"mygroup.example.com", Version:"v1beta1", Kind:"MyObj"})
	m.singularToPlural[{mygroup.example.com v1beta1 myobj}] = {mygroup.example.com v1beta1 myobjs}
	m.pluralToSingular[{mygroup.example.com v1beta1 myobjs}] = {mygroup.example.com v1beta1 myobj}

	m.resourceToKind[{mygroup.example.com v1beta1 myobj}] = mygroup.example.com/v1beta1, Kind=MyObj
	m.resourceToKind[{mygroup.example.com v1beta1 myobjs}] = mygroup.example.com/v1beta1, Kind=MyObj

	m.kindToPluralResource[mygroup.example.com/v1beta1, Kind=MyObj] = {mygroup.example.com v1beta1 myobjs}
	m.kindToScope[mygroup.example.com/v1beta1, Kind=MyObj] = namespace

Add(schema.GroupVersionKind{Group:"mygroup.example.com", Version:"v1beta1", Kind:"MyObjList"})
	m.singularToPlural[{mygroup.example.com v1beta1 myobj}] = {mygroup.example.com v1beta1 myobjs}
	m.pluralToSingular[{mygroup.example.com v1beta1 myobjs}] = {mygroup.example.com v1beta1 myobj}

	m.resourceToKind[{mygroup.example.com v1beta1 myobj}] = mygroup.example.com/v1beta1, Kind=MyObjList
	m.resourceToKind[{mygroup.example.com v1beta1 myobjs}] = mygroup.example.com/v1beta1, Kind=MyObjList

	m.kindToPluralResource[mygroup.example.com/v1beta1, Kind=MyObjList] = {mygroup.example.com v1beta1 myobjs}
	m.kindToScope[mygroup.example.com/v1beta1, Kind=MyObjList] = namespace

so it's overwriting the resourceToKind maps to point to the List kinds, which is not right.

Copy link
Member

@liggitt liggitt Aug 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm actually not sure what the purpose of adding the Kind+"List" was previously, given that the resource names it guessed were so wrong. the only thing correct was the scope

Copy link
Member

@liggitt liggitt Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@deads2k would be good to get this fixed for 1.8 (the plural thing, not necessarily the list bit)

@liggitt liggitt mentioned this pull request Aug 8, 2017
Merged
monopole pushed a commit to monopole/kubernetes that referenced this pull request Aug 8, 2017
Merge pull request kubernetes#50291 from liggitt/restmapper-flake
9b6e6f5 
Automatic merge from submit-queue

Change test to work around restmapper pluralization bug

Fixes kubernetes#50022

Works around the pluralization bug to unblock the queue.
Once the restmapper bug is fixed in kubernetes#50012, we should add tests specifically for unconventional singular/plural word endings.
@mbohlool
Copy link
Contributor

Are we still shooting for 1.7 here? I also have the same question about Kind+"List". It looks to me that if we added unnecessary endpoints or changed the surface of APIs in any way we should have seen some swagger/OpenAPI changes but there is none which is good. If we remove those Kind+"List" does it change the surface of APIs?

If we don't want to merge this for 1.7 please remove it from v1.7 milestone.

@k8s-github-robot
Copy link

Adding do-not-merge/release-note-label-needed because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-action-required", "release-note-experimental" or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

@k8s-github-robot k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 30, 2017
@liggitt liggitt removed the queue/fix label Aug 31, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Sep 13, 2017

I'm actually not sure what the purpose of adding the Kind+"List" was previously, given that the resource names it guessed were so wrong. the only thing correct was the scope

kubectl doesn't respect kubectl get podlist (bad mapping as you noted). I'm happy to simplify our RESTMapper.

@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. release-note-label-needed labels Sep 13, 2017
@liggitt
Copy link
Member

liggitt commented Sep 13, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

Associated issue: 49948

The full list of commands accepted by this bot can be found here.

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

@liggitt
Copy link
Member

liggitt commented Sep 13, 2017

verified by making NewRandomNameCustomResourceDefinition return a resource with unconventional pluralization and running

go test -v k8s.io/kubernetes/test/integration/garbagecollector -run TestCustomResourceCascadingDeletion$

would like to see a follow up that exercises unconventional pluralization in a test

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@liggitt
Copy link
Member

liggitt commented Sep 14, 2017

/retest

1 similar comment
@liggitt
Copy link
Member

liggitt commented Sep 14, 2017

/retest

@fejta
Copy link
Contributor

fejta commented Sep 14, 2017

/retest
ref kubernetes/test-infra#4568

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

/test all [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 e190f39 into kubernetes:master Sep 15, 2017
@k8s-ci-robot
Copy link
Contributor

@deads2k: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-bazel 1c8a9e4 link /test pull-kubernetes-e2e-gce-bazel

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@liggitt liggitt mentioned this pull request Sep 15, 2017
Merged
@liggitt
Copy link
Member

liggitt commented Sep 15, 2017

picked to 1.7 in #52545

@liggitt liggitt mentioned this pull request Sep 18, 2017
Closed
@nikhita nikhita mentioned this pull request Sep 19, 2017
Closed
@MrHohn MrHohn mentioned this pull request Sep 19, 2017
Closed
@wojtek-t wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 2, 2017
k8s-github-robot pushed a commit that referenced this pull request Oct 2, 2017
Merge pull request #52545 from liggitt/automated-cherry-pick-of-#5001…
dd388c7 
…2-upstream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #50012

Cherry pick of #50012 on release-1.7.

#50012: use specified discovery information if possible

```release-note
custom resources that use unconventional pluralization now work properly with kubectl
```
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@deads2k deads2k deleted the cli-17-discovery branch July 3, 2018 18:01
@tcnghia tcnghia mentioned this pull request Jul 13, 2018
Closed
This was referenced Apr 15, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@ash2k ash2k ash2k left review comments

Assignees

@ironcladlou ironcladlou

@liggitt liggitt

@mbohlool mbohlool

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.