Smarter generic getters and describers by fabianofranz · Pull Request #44222 · kubernetes/kubernetes

fabianofranz · 2017-04-07T20:35:32Z

Makes printers and describers smarter for generic resources.

This traverses unstructured objects and prints their attributes for generic resources (TPR, federated API, etc) in kubectl get and kubectl describe. Makes use of the object's field names to come up with a best guess for describer labels and get headers, and field value types to understand how to better print it, indent, etc.

A nice intermediate solution while we don't have get and describe extensions.

Examples:

$ kubectl get serviceclasses
NAME                    KIND                                          BINDABLE   BROKER NAME   OSB GUID
user-provided-service   ServiceClass.v1alpha1.servicecatalog.k8s.io   false      ups-broker    4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468

$ kubectl describe serviceclasses/user-provided-service
Name:		user-provided-service
Namespace:	
Labels:		<none>
Annotations:	FOO=BAR
		openshift.io/deployment.phase=test
OSB Metadata:	<nil>
Kind:		ServiceClass
Metadata:
  Self Link:		/apis/servicecatalog.k8s.io/v1alpha1/serviceclassesuser-provided-service
  UID:			1509bd96-1b05-11e7-98bd-0242ac110006
  Resource Version:	256
  Creation Timestamp:	2017-04-06T20:10:29Z
Broker Name:		ups-broker
Bindable:		false
Plan Updatable:		false
OSB GUID:		4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468
API Version:		servicecatalog.k8s.io/v1alpha1
Plans:
  Name:		default
  OSB GUID:	86064792-7ea2-467b-af93-ac9694d96d52
  OSB Free:	true
  OSB Metadata:	<nil>
Events:		<none>

Release note:

Improved output on 'kubectl get' and 'kubectl describe' for generic objects.

PTAL @pmorie @pwittrock @kubernetes/sig-cli-pr-reviews

k8s-reviewable · 2017-04-07T20:35:39Z

This change is

fabianofranz · 2017-04-07T20:36:57Z

Also @liggitt since you wrote the previous version.

fabianofranz · 2017-04-07T21:45:35Z

@k8s-bot gci gce e2e test this

pmorie · 2017-04-08T00:47:54Z

I'm extremely interested in this PR for service-catalog, just for the record

fabianofranz · 2017-04-10T14:54:03Z

fyi @jwforres

ericchiang · 2017-04-10T15:51:29Z

I'm assigned but pretty unfamiliar with this code. Maybe someone else would be a better reviewer?

pwittrock · 2017-04-10T23:20:49Z

@fabianofranz Does this print all the fields, or how does it decide which ones to print?

fabianofranz · 2017-04-11T00:41:53Z

@pwittrock Current rules are

describe prints all fields, and knows the field levels (for indentation) based on the field type (e.g. it's a slice or map inside another map then it's one level below, etc)
get first calculates how many columns are left to be used by discovered fields (depending on the number of columns already taken by --show-labels, --label-columns, etc) and fills those "n" columns left with the first "n" top-level fields not yet printed, in lexical order.

pwittrock · 2017-04-11T14:52:21Z

first "n" top-level fields not yet printed

What are top-level fields? Are those the first leaf nodes encountered in a breadth-first search? Does it include both spec and status, or just spec?

fabianofranz · 2017-04-11T15:14:48Z

@pwittrock Sorry, yeah I meant leaf nodes at the tree root (so in a BFS), taken in lexical order.

Meaning, in the yam below it would print:

first 2 columns with the name and apiVersion+kind (these are hardcoded and always printed)
then 3 columns chosen from leaf nodes at root in a BFS, in lexical order: bindable, brokerName, osbGuid.

apiVersion: servicecatalog.k8s.io/v1alpha1
kind: ServiceClass
metadata:
  creationTimestamp: 2017-03-03T04:11:17Z
  name: user-provided-service
  resourceVersion: "7"
  selfLink: /apis/servicecatalog.k8s.io/v1alpha1/serviceclassesuser-provided-service
  uid: 72fef5ce-ffc7-11e6-b111-0242ac110005
brokerName: ups-broker
osbGuid: 4F6E6CF6-FFDD-425F-A2C7-3C9258AD2468
bindable: false
planUpdatable: false
plans:
- name: default
  osbFree: true
  osbGuid: 86064792-7ea2-467b-af93-ac9694d96d52

If spec and status have children (which is the case in "Broker" objects in the svc catalog, for example) they would never be printed in this generic rule for get.

This is the current state, but I could improve if it makes sense, like trying to take something from the children of spec and status if they are present. The only concern is to try to be generic and not add too much special purpose code.

pwittrock · 2017-04-11T15:28:31Z

@fabianofranz I agree keeping it as simple as possible for as long as possible is best. Spec and Status are relatively standard, so it is too bad this won't work well for them, but I think that is ok for now.

pwittrock · 2017-04-11T17:53:00Z

pkg/printers/internalversion/describe.go

 }

+func printUnstructuredContent(w PrefixWriter, level int, content map[string]interface{}, skipPrefix string, skip ...string) {
+	for field, value := range content {


Do the values always appear in the same order? If not, can we make them consistent?

Not in describe, but if we want it consistent I can use the same strategy used in get (an ordered slice of the map keys).

If you don't mind, lets do that. I think it would be frustrating if you run the same describe command 2x and the output is reordered. Esp if you are trying to debug - e.g. describe, look for field, do something, describe, look for field (where did it go?)

pwittrock · 2017-04-11T17:55:48Z

pkg/printers/internalversion/describe_test.go

+			expected: "\n  Creation Timestamp:\t2017-04-01T00:00:00Z\n",
+		},
+		{
+			expected: "\nItems:\n  Item Bool:\ttrue\n",


why is itemInt missing?

It's not missing, I just didn't care to test it. I'll add.

pwittrock · 2017-04-11T18:14:17Z

pkg/printers/humanreadable.go

+		var discoveredFieldNames []string                    // we want it predictable so this will be used to sort
+		ignoreIfDiscovered := []string{"kind", "apiVersion"} // these are already covered
+		for field, value := range content {
+			if slice.ContainsString(ignoreIfDiscovered, field, nil) {


Does this always discover the same fields? Is the order always the same?

Yes, here in get the order is always the same so it will always discover the same fields.

fabianofranz · 2017-04-11T21:25:44Z

@pwittrock thanks for the review, comments addressed.

pwittrock · 2017-04-12T15:11:20Z

pkg/printers/humanreadable.go

 	}

 	if _, err := meta.Accessor(obj); err == nil {
+		// we don't recognize this type, but we can still attempt to print some reasonable information about.


Is there a test for this?

Added some in humanreadable_test.go.

pwittrock · 2017-04-12T15:13:12Z

pkg/printers/internalversion/describe_test.go

+	printUnstructuredContent(w, LEVEL_0, obj.UnstructuredContent(), "", ".dummy1", ".metadata.dummy3")
+	output := out.String()
+
+	for _, test := range testCases {


Now that the order is consistent, we should make sure we test the ordering. Doing so should simplify this check.

fabianofranz · 2017-04-13T01:16:14Z

@pwittrock comments addressed.

pwittrock · 2017-04-17T19:59:16Z

pkg/util/slice/slice.go


+// ContainsString checks if a given slice of strings contains the provided string.
+// If a modifier func is provided, it is called with the slice item before the comparation.
+func ContainsString(slice []string, s string, modifier func(s string) string) bool {


Is this still used?

Yes, still in a few other places.

pwittrock · 2017-04-17T20:02:13Z

/lgtm

fabianofranz · 2017-04-17T22:33:20Z

Needs approval, @smarterclayton mind taking a look?

fabianofranz · 2017-04-17T23:58:46Z

Needs approval, @smarterclayton mind taking a look?

Or @bgrant0607

smarterclayton · 2017-04-18T21:33:20Z

Approve but one minor nit. If you want to do in a follow up that's fine

/approve

k8s-github-robot · 2017-04-18T21:44:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianofranz, pwittrock, smarterclayton

Details

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

~~OWNERS~~ [smarterclayton]

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

fabianofranz · 2017-04-19T00:23:32Z

@k8s-bot cvm gce e2e test this

k8s-github-robot · 2017-04-19T00:41:58Z

Automatic merge from submit-queue (batch tested with PRs 44222, 44614, 44292, 44638)

smarterclayton · 2017-06-25T18:00:35Z

In 1.8 we'll have most of the generic printing on the server in place. Any objections to taking this specific part out and relying on that? In general, any object will return name and age, CRD can return based on their openapi extensions, and extension APIs can return full structured fields.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 7, 2017

k8s-github-robot assigned ericchiang Apr 7, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 7, 2017

fabianofranz force-pushed the better_generic_getters_and_describers branch from 29b6665 to 8bfe8d3 Compare April 7, 2017 20:37

fabianofranz changed the title ~~Better generic getters and describers~~ Smarter generic getters and describers Apr 7, 2017

fabianofranz mentioned this pull request Apr 7, 2017

Need show resource current status when get/describe kubernetes-retired/service-catalog#645

Closed

fabianofranz assigned smarterclayton and adohe-zz and unassigned ericchiang Apr 10, 2017

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

fabianofranz force-pushed the better_generic_getters_and_describers branch from 8bfe8d3 to c34168f Compare April 11, 2017 00:56

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

fabianofranz assigned pwittrock Apr 11, 2017

fabianofranz requested a review from pwittrock April 11, 2017 16:54

pwittrock reviewed Apr 11, 2017

View reviewed changes

fabianofranz force-pushed the better_generic_getters_and_describers branch from c34168f to 5062f1f Compare April 11, 2017 21:24

pwittrock reviewed Apr 12, 2017

View reviewed changes

fabianofranz force-pushed the better_generic_getters_and_describers branch from 5062f1f to ccf8e6a Compare April 13, 2017 01:14

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2017

fabianofranz added 3 commits April 17, 2017 15:42

Adds github.com/fatih/camelcase dependency

85bd965

Smarter describer for generic resources

151770c

Smarter get printer for generic resources

b276f17

fabianofranz force-pushed the better_generic_getters_and_describers branch from ccf8e6a to b276f17 Compare April 17, 2017 18:43

pwittrock reviewed Apr 17, 2017

View reviewed changes

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2017

k8s-github-robot merged commit 409b0a6 into kubernetes:master Apr 19, 2017

smarterclayton mentioned this pull request Jun 25, 2017

Refactor and simplify generic printer for unknown objects #48033

Merged

ardaguclu mentioned this pull request Dec 17, 2025

Enhance smartLabelFor function #135683

Merged

Conversation

fabianofranz commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-reviewable commented Apr 7, 2017

Uh oh!

fabianofranz commented Apr 7, 2017

Uh oh!

fabianofranz commented Apr 7, 2017

Uh oh!

pmorie commented Apr 8, 2017

Uh oh!

fabianofranz commented Apr 10, 2017

Uh oh!

ericchiang commented Apr 10, 2017

Uh oh!

pwittrock commented Apr 10, 2017

Uh oh!

fabianofranz commented Apr 11, 2017

Uh oh!

pwittrock commented Apr 11, 2017

Uh oh!

fabianofranz commented Apr 11, 2017

Uh oh!

pwittrock commented Apr 11, 2017

Uh oh!

pwittrock Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianofranz commented Apr 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianofranz commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwittrock commented Apr 17, 2017

Uh oh!

fabianofranz commented Apr 17, 2017

Uh oh!

fabianofranz commented Apr 17, 2017

Uh oh!

smarterclayton commented Apr 18, 2017

Uh oh!

k8s-github-robot commented Apr 18, 2017

Uh oh!

fabianofranz commented Apr 19, 2017

Uh oh!

k8s-github-robot commented Apr 19, 2017

Uh oh!

smarterclayton commented Jun 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

fabianofranz commented Apr 7, 2017 •

edited

Loading

pwittrock Apr 11, 2017 •

edited

Loading