[WIP] Fix hardcoded string, add fetch validResource from APIServer #51324

shiywang · 2017-08-25T08:58:04Z

if DiscoveryClient failed, return previous hard coded string.
this change may make kubectl get/annotate.. -h a little slow first time, @deads2k do you think it's acceptable ?

kubectl get -h  0.13s user 0.02s system 128% cpu 0.117 total
./kubectl get -h  0.13s user 0.02s system 5% cpu 2.600 total

before:

shiywang:amd64 root [dynamicResource] $ kubectl get -h
Display one or many resources. 

Valid resource types include: 

  * all  
  * certificatesigningrequests (aka 'csr')  
  * clusters (valid only for federation apiservers)  
  * clusterrolebindings  
  * clusterroles  
  * componentstatuses (aka 'cs')  
  * configmaps (aka 'cm')  
  * daemonsets (aka 'ds')  
  * deployments (aka 'deploy')  
  * endpoints (aka 'ep')  
  * events (aka 'ev')  
  * horizontalpodautoscalers (aka 'hpa')  
  * ingresses (aka 'ing')  
  * jobs  
  * limitranges (aka 'limits')  
  * namespaces (aka 'ns')  
  * networkpolicies  
  * nodes (aka 'no')  
  * persistentvolumeclaims (aka 'pvc')  
  * persistentvolumes (aka 'pv')  
  * pods (aka 'po')  
  * poddisruptionbudgets (aka 'pdb')  
  * podsecuritypolicies (aka 'psp')  
  * podtemplates  
  * replicasets (aka 'rs')  
  * replicationcontrollers (aka 'rc')  
  * resourcequotas (aka 'quota')  
  * rolebindings  
  * roles  
  * secrets  
  * serviceaccounts (aka 'sa')  
  * services (aka 'svc')  
  * statefulsets  
  * storageclasses  
  * thirdpartyresources  

This command will hide resources that have completed, such as pods that are in the Succeeded or Failed phases. You can
see the full results for any resource by providing the '--show-all' flag.

after:

shiywang:amd64 root [dynamicResource] $ ./kubectl get -h
Display one or many resources. 

Valid resource types include:

  * all
  * bindings 
  * certificatesigningrequests  (aka 'csr')
  * clusterrolebindings 
  * clusterroles 
  * clusters (valid only for federation apiservers)
  * componentstatuses  (aka 'cs')
  * configmaps  (aka 'cm')
  * controllerrevisions 
  * customresourcedefinitions  (aka 'crd')
  * daemonsets  (aka 'ds')
  * deployments  (aka 'deploy')
  * endpoints  (aka 'ep')
  * events  (aka 'ev')
  * externaladmissionhookconfigurations 
  * horizontalpodautoscalers  (aka 'hpa')
  * ingresses  (aka 'ing')
  * initializerconfigurations 
  * jobs 
  * limitranges  (aka 'limits')
  * localsubjectaccessreviews 
  * namespaces  (aka 'ns')
  * networkpolicies  (aka 'netpol')
  * nodes  (aka 'no')
  * persistentvolumeclaims  (aka 'pvc')
  * persistentvolumes  (aka 'pv')
  * poddisruptionbudgets  (aka 'pdb')
  * podpresets 
  * pods  (aka 'po')
  * podsecuritypolicies  (aka 'psp')
  * podtemplates 
  * replicasets  (aka 'rs')
  * replicationcontrollers 
  * resourcequotas  (aka 'quota')
  * rolebindings 
  * roles 
  * secrets 
  * selfsubjectaccessreviews 
  * serviceaccounts  (aka 'sa')
  * services  (aka 'svc')
  * statefulsets  (aka 'sts')
  * storageclasses  (aka 'sc')
  * subjectaccessreviews 
  * tokenreviews 
 

This command will hide resources that have completed, such as pods that are in the Succeeded or Failed phases. You can
see the full results for any resource by providing the '--show-all' flag.

k8s-github-robot · 2017-08-25T08:58:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shiywang
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

Associated issue: 51023

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

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

pkg/kubectl/OWNERS

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

shiywang · 2017-08-25T09:00:23Z

pkg/kubectl/cmd/util/factory_object_mapping.go

+const validResources = `Valid resource types include:
+
+    * all
+    * certificatesigningrequests (aka 'csr')


some of are not belong to core/legacy api group, remove them ?

some of are not belong to core/legacy api group, remove them ?

I would

shiywang · 2017-08-25T09:00:39Z

/assign @deads2k

deads2k · 2017-08-25T18:48:29Z

pkg/kubectl/cmd/util/factory_object_mapping.go

+	resourceMap := make(map[string]metav1.APIResource)
+	var keys []string
+	discoveryClient, err := f.clientAccessFactory.DiscoveryClient()
+	if err == nil {


invert the if to reduce nesting

deads2k · 2017-08-25T18:48:38Z

pkg/kubectl/cmd/util/factory_object_mapping.go

+	discoveryClient, err := f.clientAccessFactory.DiscoveryClient()
+	if err == nil {
+		apiResList, err := discoveryClient.ServerResources()
+		if err == nil {


invert if to reduce nesting

deads2k · 2017-08-25T18:49:25Z

pkg/kubectl/cmd/util/factory_object_mapping.go

+		if err == nil {
+			for _, apiResources := range apiResList {
+				for _, apiRes := range apiResources.APIResources {
+					if !strings.Contains(apiRes.Name, "/") && apiRes.Name != "apiservices" {


apiservices are resources too! :)

deads2k · 2017-08-25T18:50:31Z

pkg/kubectl/cmd/util/factory_object_mapping.go

@@ -489,3 +490,95 @@ func (f *ring1Factory) OpenAPISchema(cacheDir string) (openapi.Resources, error)
 	// Delegate to the OpenAPIGetter
 	return f.openAPIGetter.getter.Get()
 }
+
+func (f *ring1Factory) ValidResourcesFromDiscoveryClient() string {


I think this method ought to return []string of just the resources strings

resources strings only contain resource name ? or we can directly return sorted []APIResources then write a wrapper/helper function to concatenate those APIResource.Name and APIREsource.ShortNames to string output ?

deads2k · 2017-08-25T18:51:36Z

pkg/kubectl/cmd/util/factory_object_mapping.go

+			row := "Valid resource types include:\n\n  * all\n"
+			for _, k := range keys {
+				if k == "clusters" {
+					row = row + "  * clusters (valid only for federation apiservers)\n"


I'd do this purely on discovery. If they're in the discovery list, they don't need qualification

deads2k · 2017-08-25T18:52:49Z

@kubernetes/sig-cli-misc

k8s-github-robot · 2017-08-30T22:19:58Z

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.

update update

k8s-ci-robot · 2017-08-31T12:18:52Z

@shiywang: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	`585d4e8`	link	`/test pull-kubernetes-bazel`
pull-kubernetes-e2e-gce-bazel	`36b8fab`	link	`/test pull-kubernetes-e2e-gce-bazel`
pull-kubernetes-unit	`36b8fab`	link	`/test pull-kubernetes-unit`
pull-kubernetes-kubemark-e2e-gce	`36b8fab`	link	`/test pull-kubernetes-kubemark-e2e-gce`
pull-kubernetes-e2e-gce-etcd3	`36b8fab`	link	`/test pull-kubernetes-e2e-gce-etcd3`
pull-kubernetes-e2e-kops-aws	`36b8fab`	link	`/test pull-kubernetes-e2e-kops-aws`

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.

k8s-github-robot · 2017-09-01T08:22:04Z

@shiywang PR needs rebase

liggitt · 2017-09-02T15:43:35Z

pkg/kubectl/cmd/annotate.go

@@ -113,7 +113,7 @@ func NewCmdAnnotate(f cmdutil.Factory, out io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:     "annotate [--overwrite] (-f FILENAME | TYPE NAME) KEY_1=VAL_1 ... KEY_N=VAL_N [--resource-version=version]",
 		Short:   i18n.T("Update the annotations on a resource"),
-		Long:    annotateLong,
+		Long:    templates.LongDesc(fmt.Sprintf(annotateLong, f.ValidResourcesFromDiscoveryClient())),


I don't think we should ever talk to the server to display help. I'd rather see a dedicated command for displaying available resources, and reference that command in this help string

I wrote a proposal for this feature, ptal kubernetes/community#1017

k8s-github-robot · 2017-12-03T08:15:45Z

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @deads2k @fabianofranz @ghodss @shiywang

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 25, 2017

k8s-github-robot assigned ghodss and deads2k Aug 25, 2017

k8s-github-robot added the release-note-label-needed label Aug 25, 2017

shiywang commented Aug 25, 2017

View reviewed changes

shiywang force-pushed the dynamicResource branch 3 times, most recently from db78046 to 913a5f4 Compare August 25, 2017 09:19

shiywang changed the title ~~Fix hardcoded string, add fetch validResource from APIServer~~ [WIP] Fix hardcoded string, add fetch validResource from APIServer Aug 25, 2017

deads2k reviewed Aug 25, 2017

View reviewed changes

deads2k assigned fabianofranz Aug 25, 2017

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Aug 25, 2017

shiywang mentioned this pull request Aug 28, 2017

Surface kubectl clusters in kubectl help when talking to federation apiserver #25592

Closed

shiywang force-pushed the dynamicResource branch 2 times, most recently from bd1c7c2 to f098b7a Compare August 29, 2017 05:43

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

Shiyang Wang added 4 commits August 31, 2017 17:45

Fix hardcoded string, add fetch validResource from APIServer

06f89fe

update update

update

d92a968

update

06a5711

make cli-check pass

36b8fab

shiywang force-pushed the dynamicResource branch from f098b7a to 36b8fab Compare August 31, 2017 09:47

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017

liggitt reviewed Sep 2, 2017

View reviewed changes

xiangpengzhao mentioned this pull request Sep 4, 2017

discover valid resources for kubectl instead of current hard-coded way #51865

Closed

dixudx mentioned this pull request Sep 7, 2017

(#21648)add kubectl set port command #51947

Closed

irfanurrehman mentioned this pull request Oct 30, 2017

Surface kubectl clusters in kubectl help when talking to federation apiserver kubernetes-retired/federation#43

Closed

shiywang mentioned this pull request Nov 5, 2017

kubectl CLI should more effectively convey discovery #53387

Closed

k8s-github-robot closed this Dec 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix hardcoded string, add fetch validResource from APIServer #51324

[WIP] Fix hardcoded string, add fetch validResource from APIServer #51324

shiywang commented Aug 25, 2017 •

edited by k8s-github-robot

Loading

k8s-github-robot commented Aug 25, 2017

shiywang Aug 25, 2017

deads2k Aug 25, 2017

shiywang commented Aug 25, 2017

deads2k Aug 25, 2017

deads2k Aug 25, 2017

deads2k Aug 25, 2017

deads2k Aug 25, 2017

shiywang Aug 28, 2017 •

edited

Loading

deads2k Aug 25, 2017

deads2k commented Aug 25, 2017

k8s-github-robot commented Aug 30, 2017

k8s-ci-robot commented Aug 31, 2017

k8s-github-robot commented Sep 1, 2017

liggitt Sep 2, 2017 •

edited

Loading

shiywang Sep 4, 2017 •

edited

Loading

k8s-github-robot commented Dec 3, 2017

[WIP] Fix hardcoded string, add fetch validResource from APIServer #51324

[WIP] Fix hardcoded string, add fetch validResource from APIServer #51324

Conversation

shiywang commented Aug 25, 2017 • edited by k8s-github-robot Loading

k8s-github-robot commented Aug 25, 2017

shiywang Aug 25, 2017

Choose a reason for hiding this comment

deads2k Aug 25, 2017

Choose a reason for hiding this comment

shiywang commented Aug 25, 2017

deads2k Aug 25, 2017

Choose a reason for hiding this comment

deads2k Aug 25, 2017

Choose a reason for hiding this comment

deads2k Aug 25, 2017

Choose a reason for hiding this comment

deads2k Aug 25, 2017

Choose a reason for hiding this comment

shiywang Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

deads2k Aug 25, 2017

Choose a reason for hiding this comment

deads2k commented Aug 25, 2017

k8s-github-robot commented Aug 30, 2017

k8s-ci-robot commented Aug 31, 2017

k8s-github-robot commented Sep 1, 2017

liggitt Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

shiywang Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

k8s-github-robot commented Dec 3, 2017

shiywang commented Aug 25, 2017 •

edited by k8s-github-robot

Loading

shiywang Aug 28, 2017 •

edited

Loading

liggitt Sep 2, 2017 •

edited

Loading

shiywang Sep 4, 2017 •

edited

Loading