fix(kubectl): explain crds whose resource name is the same as builtin objects #89505

knight42 · 2020-03-26T08:21:36Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Currently, if the resource name of a custom resource is the same as one of builtin objects(like knative Service and core Service), kubectl explain fails to find the correct resource even a distinguishable short name is given.

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#749

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Explain CRDs whose resource name are the same as builtin objects

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2020-03-26T08:21:45Z

Hi @knight42. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

knight42 · 2020-03-26T08:36:03Z

/assign @apelisse

seans3 · 2020-03-26T16:56:59Z

/ok-to-test

seans3

It would be nice to add a unit test to show this works as expected.

seans3 · 2020-03-26T17:06:53Z

/priority important-soon

knight42 · 2020-03-27T05:39:25Z

@seans3 I have updated the tests to show that SplitAndParseResourceRequest would return the resource name with group appended, but explain short name would require a mock apiserver, not sure how to verify it in the tests.

knight42 · 2020-03-31T11:00:33Z

/assign @deads2k

knight42 · 2020-03-31T11:02:40Z

@seans3 I managed to add an e2e test to verify that kubectl explain works as expected, could you take a look?

soltysh

Please also squash your changes into a single commit to start with.
/assign

soltysh · 2020-04-08T18:49:19Z

staging/src/k8s.io/kubectl/pkg/explain/explain.go

+	}
+	// Only returning the resource name is not enough. CRDs may have the same resource name.
+	// TODO(knight42): not sure whether we need to speicfy the version here.
+	return fmt.Sprintf("%s.%s", singular, gvr.Group), fieldsPath, nil


why not changing this and return GroupVersionResource, fieldsPath and error here?

How this will work in the situation where I specify --api-version that is different than what you discovered here? Should there be an error in explain flow? I'd expect one.

How this will work in the situation where I specify --api-version that is different than what you discovered here

I would like to respect the given apiVersion, since users are likely to know what they are doing if they specify --api-version explicitly.

why not changing this and return GroupVersionResource, fieldsPath and error here?

fixed

soltysh · 2020-04-08T18:54:28Z

@knight42 pls ping me on slack when you update this.

…bjects

knight42 · 2020-04-09T10:25:50Z

/test pull-kubernetes-conformance-kind-ga-only-parallel

knight42 · 2020-04-09T10:56:54Z

/test pull-kubernetes-conformance-kind-ga-only-parallel

soltysh

/lgtm
/approve

k8s-ci-robot · 2020-04-20T14:12:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knight42, soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/kubectl/OWNERS~~ [soltysh]
~~test/e2e/apimachinery/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2020-04-20T20:04:19Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-04-21T02:22:24Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-04-21T05:31:20Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

knight42 · 2020-04-21T07:39:21Z

/test pull-kubernetes-e2e-gce

liggitt · 2020-04-27T02:46:59Z

test/e2e/apimachinery/crd_publish_openapi.go

@@ -120,6 +126,11 @@ var _ = SIGDescribe("CustomResourcePublishOpenAPI [Privileged:ClusterAdmin]", fu
 			framework.Failf("%v", err)
 		}

+		ginkgo.By("kubectl explain works for CR with the same resource name as built-in object")
+		if err := verifyKubectlExplain(f.Namespace.Name, customServiceShortName+".spec", `(?s)DESCRIPTION:.*Specification of CustomService.*FIELDS:.*dummy.*<string>.*Dummy property`); err != nil {


this modified a conformance test and introduced a test that has been flaking. This needs to be done in a non-conformance test first to demonstrate the test is stable.

@liggitt should I remove this test from the conformance tests first?

Yes, revert the conformance test to its previous state and isolate the new test in a non-conformance e2e test (marked as flaky until the test has been deflaked)

k8s-ci-robot requested review from apelisse and juanvallejo March 26, 2020 08:22

knight42 mentioned this pull request Mar 26, 2020

kubectl explain: shows the wrong resource kubernetes/kubectl#749

Closed

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 26, 2020

k8s-ci-robot assigned apelisse Mar 26, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 26, 2020

seans3 reviewed Mar 26, 2020

View reviewed changes

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 26, 2020

knight42 force-pushed the fix/kubectl-explain-crd branch from 433457b to 51fe4ae Compare March 27, 2020 04:07

knight42 force-pushed the fix/kubectl-explain-crd branch from 099cf4d to 777a6d0 Compare March 31, 2020 09:52

k8s-ci-robot assigned deads2k Mar 31, 2020

soltysh requested changes Apr 8, 2020

View reviewed changes

k8s-ci-robot assigned soltysh Apr 8, 2020

knight42 force-pushed the fix/kubectl-explain-crd branch from 777a6d0 to 370894d Compare April 9, 2020 06:19

fix(kubectl): explain crds with the same resource name with builtin o…

397f107

…bjects

knight42 force-pushed the fix/kubectl-explain-crd branch from 370894d to 397f107 Compare April 9, 2020 08:44

soltysh approved these changes Apr 20, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2020

k8s-ci-robot merged commit 4362bf7 into kubernetes:master Apr 21, 2020

k8s-ci-robot added this to the v1.19 milestone Apr 21, 2020

knight42 deleted the fix/kubectl-explain-crd branch April 21, 2020 08:54

hasheddan mentioned this pull request Apr 26, 2020

[Flaky Test]: gce-master-scale-correctness (ci-kubernetes-e2e-gce-scale-correctness) #90506

Closed

liggitt reviewed Apr 27, 2020

View reviewed changes

knight42 restored the fix/kubectl-explain-crd branch April 27, 2020 03:35

liggitt mentioned this pull request Apr 28, 2020

fix flaky conformance test #90509

Merged

knight42 deleted the fix/kubectl-explain-crd branch May 29, 2020 12:17

knight42 mentioned this pull request Jun 25, 2020

REQUEST: New membership for knight42 kubernetes/org#1955

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(kubectl): explain crds whose resource name is the same as builtin objects #89505

fix(kubectl): explain crds whose resource name is the same as builtin objects #89505

knight42 commented Mar 26, 2020 •

edited

k8s-ci-robot commented Mar 26, 2020

knight42 commented Mar 26, 2020

seans3 commented Mar 26, 2020

seans3 left a comment

seans3 commented Mar 26, 2020

knight42 commented Mar 27, 2020

knight42 commented Mar 31, 2020

knight42 commented Mar 31, 2020 •

edited

soltysh left a comment

soltysh Apr 8, 2020

soltysh Apr 8, 2020

knight42 Apr 9, 2020

knight42 Apr 9, 2020

soltysh commented Apr 8, 2020

knight42 commented Apr 9, 2020

knight42 commented Apr 9, 2020

soltysh left a comment

k8s-ci-robot commented Apr 20, 2020

fejta-bot commented Apr 20, 2020

fejta-bot commented Apr 21, 2020

fejta-bot commented Apr 21, 2020

knight42 commented Apr 21, 2020

liggitt Apr 27, 2020

knight42 Apr 27, 2020

liggitt Apr 27, 2020

fix(kubectl): explain crds whose resource name is the same as builtin objects #89505

fix(kubectl): explain crds whose resource name is the same as builtin objects #89505

Conversation

knight42 commented Mar 26, 2020 • edited

k8s-ci-robot commented Mar 26, 2020

knight42 commented Mar 26, 2020

seans3 commented Mar 26, 2020

seans3 left a comment

Choose a reason for hiding this comment

seans3 commented Mar 26, 2020

knight42 commented Mar 27, 2020

knight42 commented Mar 31, 2020

knight42 commented Mar 31, 2020 • edited

soltysh left a comment

Choose a reason for hiding this comment

soltysh Apr 8, 2020

Choose a reason for hiding this comment

soltysh Apr 8, 2020

Choose a reason for hiding this comment

knight42 Apr 9, 2020

Choose a reason for hiding this comment

knight42 Apr 9, 2020

Choose a reason for hiding this comment

soltysh commented Apr 8, 2020

knight42 commented Apr 9, 2020

knight42 commented Apr 9, 2020

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 20, 2020

fejta-bot commented Apr 20, 2020

fejta-bot commented Apr 21, 2020

fejta-bot commented Apr 21, 2020

knight42 commented Apr 21, 2020

liggitt Apr 27, 2020

Choose a reason for hiding this comment

knight42 Apr 27, 2020

Choose a reason for hiding this comment

liggitt Apr 27, 2020

Choose a reason for hiding this comment

knight42 commented Mar 26, 2020 •

edited

knight42 commented Mar 31, 2020 •

edited