Adds more columns in service get command output #56

navidshaikh · 2019-04-04T16:48:26Z

This changeset adds basic details about service in
service get command output.
text/tabwriter is used to print columns on stdout.

Fixes ##40

cppforlife · 2019-04-08T17:36:59Z

/ok-to-test

navidshaikh · 2019-04-09T09:40:16Z

/retest

pkg/kn/commands/service_list_test.go

pkg/util/printers/tabwriter.go

rhuss

Looks good to me, but I'm not sure whether tabwriter.Writer is the proper approach as it assumes always equally sized columns (although with a minimum width). I don't think it is optimal for distributing screen real estate.

For example, typically the NAME column is much larger than a status column. E.g. for plain pods its not uncommon to have name of 20 chars, but the READY column will be only around 3-5 chars. Having this equally sized with 20 chars is waste of space and doesnt look good anyway.

I recommend a more customized format. Not sure what Go package is a good fit (maybe fromhttps://github.com/kubernetes/cli-runtime). If it would be Perl, I would use perl formats.

tl;dr - I think we already think about flexible, non equally sized column widths.

pkg/kn/commands/service_list.go

navidshaikh · 2019-04-15T10:21:23Z

@rhuss : Thanks for your review, each column width is re-adjusted based on the cell's max width (of that column). Hard-coding the padding and column width wont scale IMO.
For printers.Flush(), lets use defer to make sure it's called before returning. Sending an update to the PR.

rhuss · 2019-04-15T10:34:16Z

@rhuss : Thanks for your review, each column width is re-adjusted based on the cell's max width (of that column). Hard-coding the padding and column width wont scale IMO.

Yeah right. My fault, I've misread the documentation here. I think you are right, we should stay with this approach.

sixolet · 2019-04-15T19:54:51Z

/hold
The previous approach for getting a printer enabled a lot of output flags shared with kubectl. This one doesn't seem to, if I read it correctly?

I think we should not implement our own table-writing code.

rhuss · 2019-04-16T12:36:31Z

The previous approach for getting a printer enabled a lot of output flags shared with kubectl. This one doesn't seem to, if I read it correctly?

I agree, that we should reuse the formatting machinery provided by cli-runtime for creating various output formats when specific options are provided.

I think we should not implement our own table-writing code.

Sorry for my ignorance (just ramping up with the k8s codebase), but doesn't kubectl itself uses custom code for human readable output ? I.e. in https://github.com/kubernetes/kubernetes/blob/c082ace102e217846833baace01d80966369ecd8/pkg/kubectl/cmd/get/get.go#L513 if o.IsHumanReadablePrinter == true then a tabWriter is used like in this PR.

I think we should combine both, but I'm afraid we have to write our own formatting code for the default console output, not covered by CLI options.

rhuss · 2019-04-16T12:38:12Z

in kubectl, the tab-writer is used when this combination of options is true:

// human readable printers have special conversion rules, so we determine if we're using one.
	if (len(*o.PrintFlags.OutputFormat) == 0 && len(templateArg) == 0) || *o.PrintFlags.OutputFormat == "wide" {
		o.IsHumanReadablePrinter = true
	}

rhuss · 2019-04-16T14:34:44Z

I'm a bit confused: Both kubernetes itself and cli-runtime have a PrintFlags abstraction for obtaining a suitable Printer depending on context. But only Kubernetes itself has a HumanReadableFlags field for obtaining the printer for printing columnwise on the console (as known to kubectl get). kubectl doesn't use cli-runtime for the flags.

I'm pretty sure that the supported printers (JSON, YAML, Name, Template) are not suitable for direct console output.

@sixolet Any suggestion how to proceed? We could either implement a HumanReadableWriter on our own or just use a tabbed writer for the beginning (if no other options have been selected).

navidshaikh · 2019-04-16T16:01:08Z

@rhuss : Thanks for the feedback on human readable output format and enabling the generic output format flags, I have updated the PR respectively.

Now the command supports printing with generic output format flags. Excerpts

./kn service list --help
List available services.

Usage:
  kn service list [flags]

Flags:
      --all-namespaces                If present, list the requested object(s) across all namespaces. Namespace in current context is ignored even if specified with --namespace.
      --allow-missing-template-keys   If true, ignore any errors in templates when a field or map key is missing in the template. Only applies to golang and jsonpath output formats. (default true)
  -h, --help                          help for list
  -n, --namespace string              List the requested object(s) in given namespace.
  -o, --output string                 Output format. One of: json|yaml|name|go-template|go-template-file|template|templatefile|jsonpath|jsonpath-file.
      --template string               Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].
[..]

and the service list

./kn service list
NAME   DOMAIN                   LATESTCREATED   LATESTREADY   AGE
s1     s1.default.example.com   s1-t4mbr        s1-t4mbr      5h
s2     s2.default.example.com   s2-8xrvv        s2-8xrvv      36m

and the service list with jsonpath

./kn service list -o "jsonpath={range .items[*]}{.metadata.name}{\"\\n\"}{end}"
s1
s2

knative-metrics-robot · 2019-04-16T16:24:11Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service_list.go	77.3%	79.5%	2.2

navidshaikh · 2019-04-17T09:45:11Z

@sixolet : The previous approach for getting printer from cli-runtime is now restored. If no output flags are specified, the tabwriter is used to print the human-readable output.
PTAL
cc @cppforlife

knative-prow-robot · 2019-04-24T15:53:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: navidshaikh
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: evankanderson

If they are not already assigned, you can assign the PR to them by writing /assign @evankanderson in a comment when ready.

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:

OWNERS

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

navidshaikh · 2019-04-24T15:57:45Z

Based on the recent discussions, renamed service list command to service get. service get with an argument should be a separate PR.

navidshaikh · 2019-04-25T09:11:41Z

I think we should not implement our own table-writing code.

@sixolet @cppforlife What are other recommendations ?

I was checking https://github.com/kubernetes/kubernetes/tree/master/pkg/printers to see if we can re-use it, however this would add dependency on kube-versioned packages. I think we don't want that ?

sixolet

So, I still have some trouble with the approach here. I'd like to see something that enables us to reuse some of the resource printing capabilities of k8s.io genericclioptions, and not have to reimplmeent that from scratch. That means using some of their interfaces, like before. I've linked them below in other comments.

sixolet · 2019-04-26T23:01:03Z

pkg/util/age.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package printers


pkg/util or pkg/util/printers?

sixolet · 2019-04-26T23:01:57Z

pkg/util/age.go

@@ -0,0 +1,30 @@
+// Copyright © 2018 The Knative Authors


I'm not in love with "util" as a name of things. it seems like the thing people name a bin where they stick things they don't know where else to put.

sixolet · 2019-04-26T23:02:45Z

pkg/util/printers/tabwriter.go

@@ -0,0 +1,34 @@
+// Copyright © 2018 The Knative Authors


I'd love pkg/output or pkg/printing or something.

sixolet · 2019-04-26T23:06:52Z

pkg/kn/commands/service_get.go

+				return nil
+			}
+			// if no output format flag is set, lets print the human readable output
+			printer := printers.NewTabWriter(cmd.OutOrStdout())


This needs to be a printer, implementing the interface here: https://github.com/kubernetes/cli-runtime/blob/master/pkg/printers/interface.go#L34

This is a solved problem for kubectl and the human-friendly printer implementation is in place already https://github.com/kubernetes/kubernetes/blob/master/pkg/printers/tableprinter.go#L49 ,
however its not importable (or we don't want k8s dep) as genericclioptions.
A bunch of utilities can be re-used though from pkg/printers (re-use = borrow the code?), and we'll just need to add the column-definitions and their printing logic as done here
https://github.com/kubernetes/kubernetes/blob/master/pkg/printers/internalversion/printers.go#L79 for resources operable via kn.

WDYT?
Cc: @bbrowning @rhuss

@navidshaikh If there are no efforts underway to port that part to cli-runtime anyway, I would try to move that tableprinter stuff to kn in an isolated way and if this is successful, create a PR for cli-runtime so that we could use it from there eventually. It depends on the size of the code to move around and how easy it is to isolate it, but I think this could be a strategy from which everyone could benefit.

We could do it the other way round (first move it to cli-runtime), but this is (a) doesn't help for kn in the short term and (b) if we fail at cli-runtime, the effort is wasted. That's not the case with the first approach where the only danger is, that we would have to maintain the ported code on our own.

@rhuss : makes sense, lets move ahead with first approach as you mentioned.

This commit adds basic details about service in service list command output. text/tabwriter is used to print columns on stdout. Fixes #knative#40

This commit adds AGE column representing the resources' age in human readable format calculated from CreationTimestamp field of service.

Now if list is empty, we print "No resources found." string. This commit updates the respective unit test.

Doesn't check the argument length after `service get` to leave room for filtering based on argument.

navidshaikh · 2019-05-09T11:08:01Z

Closing in favor of #90

The remote caching jobs are not configured, and anyway Knative is not using bazel to the point that remote caching is a benefit. Disable it to avoid spurious error messages in the test logs.

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 4, 2019

knative-prow-robot requested review from evankanderson and sixolet April 4, 2019 16:48

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 4, 2019

navidshaikh changed the title ~~WIP: Adds more columns in service list command output~~ Adds more columns in service list command output Apr 5, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2019

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 8, 2019

navidshaikh force-pushed the service-list branch from 2b53f46 to e22252a Compare April 9, 2019 09:25

navidshaikh force-pushed the service-list branch from 7534667 to d2497f2 Compare April 10, 2019 11:29

navidshaikh mentioned this pull request Apr 12, 2019

Adds more columns in revision get command output #70

Closed

cppforlife reviewed Apr 12, 2019

View reviewed changes

pkg/kn/commands/service_list_test.go Outdated Show resolved Hide resolved

rhuss reviewed Apr 15, 2019

View reviewed changes

pkg/util/printers/tabwriter.go Outdated Show resolved Hide resolved

rhuss reviewed Apr 15, 2019

View reviewed changes

pkg/kn/commands/service_list.go Outdated Show resolved Hide resolved

rhuss reviewed Apr 15, 2019

View reviewed changes

pkg/kn/commands/service_list.go Outdated Show resolved Hide resolved

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2019

navidshaikh force-pushed the service-list branch from 796782f to f62bd08 Compare April 24, 2019 15:52

knative-prow-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 24, 2019

navidshaikh changed the title ~~Adds more columns in service list command output~~ Adds more columns in service get command output Apr 24, 2019

sixolet suggested changes Apr 26, 2019

View reviewed changes

navidshaikh added 15 commits May 1, 2019 16:53

Adds more columns in service list command output

294a884

This commit adds basic details about service in service list command output. text/tabwriter is used to print columns on stdout. Fixes #knative#40

Adds AGE column in serivce list command output

2827b7d

This commit adds AGE column representing the resources' age in human readable format calculated from CreationTimestamp field of service.

Updates TestListEmpty test for expected output

4a4a96f

Now if list is empty, we print "No resources found." string. This commit updates the respective unit test.

Updates unit tests for service list command

d15a64f

Adds vendor/k8s.io/apimachinery/pkg/util/duration/duration.go

9d98a55

Returns nil after printing the service list table

0a6ae83

Uses camel case variable names

0e9011d

Removes unused variables

da886c8

Updates the method name and uses defer to flush the printer

8973a25

Moves calculate age utility from util/printers to util/

e2996e0

Adds a switch to enable generic output format flags

4524a7d

Adds test for jsonpath output flag

71c75c9

Renames service list command to service get

9743273

Doesn't check the argument length after `service get` to leave room for filtering based on argument.

Fixes tests

5ecb716

Updates the tests

c9b0e56

navidshaikh force-pushed the service-list branch from 0ff256c to c9b0e56 Compare May 1, 2019 11:28

navidshaikh closed this May 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds more columns in service get command output #56

Adds more columns in service get command output #56

navidshaikh commented Apr 4, 2019 •

edited

Loading

cppforlife commented Apr 8, 2019

navidshaikh commented Apr 9, 2019

rhuss left a comment

navidshaikh commented Apr 15, 2019

rhuss commented Apr 15, 2019

sixolet commented Apr 15, 2019

rhuss commented Apr 16, 2019

rhuss commented Apr 16, 2019

rhuss commented Apr 16, 2019

navidshaikh commented Apr 16, 2019 •

edited

Loading

knative-metrics-robot commented Apr 16, 2019

navidshaikh commented Apr 17, 2019

knative-prow-robot commented Apr 24, 2019

navidshaikh commented Apr 24, 2019

navidshaikh commented Apr 25, 2019

sixolet left a comment

sixolet Apr 26, 2019

sixolet Apr 26, 2019

sixolet Apr 26, 2019

sixolet Apr 26, 2019

navidshaikh Apr 29, 2019

rhuss Apr 30, 2019

navidshaikh May 1, 2019

navidshaikh commented May 9, 2019

Adds more columns in service get command output #56

Adds more columns in service get command output #56

Conversation

navidshaikh commented Apr 4, 2019 • edited Loading

cppforlife commented Apr 8, 2019

navidshaikh commented Apr 9, 2019

rhuss left a comment

Choose a reason for hiding this comment

navidshaikh commented Apr 15, 2019

rhuss commented Apr 15, 2019

sixolet commented Apr 15, 2019

rhuss commented Apr 16, 2019

rhuss commented Apr 16, 2019

rhuss commented Apr 16, 2019

navidshaikh commented Apr 16, 2019 • edited Loading

knative-metrics-robot commented Apr 16, 2019

navidshaikh commented Apr 17, 2019

knative-prow-robot commented Apr 24, 2019

navidshaikh commented Apr 24, 2019

navidshaikh commented Apr 25, 2019

sixolet left a comment

Choose a reason for hiding this comment

sixolet Apr 26, 2019

Choose a reason for hiding this comment

sixolet Apr 26, 2019

Choose a reason for hiding this comment

sixolet Apr 26, 2019

Choose a reason for hiding this comment

sixolet Apr 26, 2019

Choose a reason for hiding this comment

navidshaikh Apr 29, 2019

Choose a reason for hiding this comment

rhuss Apr 30, 2019

Choose a reason for hiding this comment

navidshaikh May 1, 2019

Choose a reason for hiding this comment

navidshaikh commented May 9, 2019

navidshaikh commented Apr 4, 2019 •

edited

Loading

navidshaikh commented Apr 16, 2019 •

edited

Loading