feature(service describe): Output of service details [vanilla, color, hipster] #75

rhuss · 2019-04-17T13:08:10Z

As discussed in #48 this kn service describe as changed in this commit
mimics kubectl describe <sth> as that it output detail information
in human readable output.

Summary information of a list of services or single services as well
as exporting the service in various formats (JSON, YAML) is reserved to
kn service list (or kn service get as suggested to align with
kubectl conventions).

For this reason, the generic Printer handling from cli-runtime has been
removed as kn service describe only outputs human readable format
(that is btw also true for kubectl describe which does not allow for
-o kind of options).

The presented information is just a start and the full content
should be discussed.

For the moment three modes for presenting information are implemented, which can be combined:

--all : Print more information. By default only are shorter summary is shown
--color : Use a colorful output (but only when on a tty)
--hipster: Experimental output mode for a very compact representation using emojis

Below are some screenshot for different examples, totally open to discuss the format, and it's also ok if the consense is that the emoji thing goes over the top.

`kn service describe random`

`kn service describe random --all`

`kn service describe random --color`

`kn service describe random --all --color`

`kn service describe random --hipster`

`kn service describe random --hipster --all`

`kn service describe random --hipster --all --color`

knative-prow-robot · 2019-04-17T13:08:25Z

Hi @rhuss. Thanks for your PR.

I'm waiting for a knative 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.

cppforlife · 2019-04-17T23:30:45Z

/ok-to-test

navidshaikh · 2019-04-18T06:21:04Z

pkg/kn/commands/service_describe.go

+	}
+}
+
+func (tw *describeWriter) sCol(label string, val string) {


should this be called dw.sRow as its printing row cells?

good point. 'sCol' was meant to be 'singleColumn' (which actually is not true as there are two cols ;0)

let's call it printlnWithLabel ?

printlnWithLabel is fine too, though I think soon we'll need a way to print tables, that should constitute a library for kn.
That library should re-use k8s.io/apimachinery/pkg/apis/meta/v1beta1.TableColumnDefinition and re-write parts of https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/get/table_printer.go .

That way, describe output becomes a two column table with no-headers bool set.

Sounds good to me. Maybe we can delay the switch until we decided what we actually want to show for kn service describe. Some suggestions are mentioned in the description of this PR.

If there are no objections, I would work on this list. We can change the fields presented any time anyway.

That library should re-use k8s.io/apimachinery/pkg/apis/meta/v1beta1.TableColumnDefinition and re-write parts of kubernetes/kubernetes:pkg/kubectl/cmd/get/table_printer.go@master .

I wonder whether the proper spot for such a library would be cli-runtime itself, where the other, machine-readable printers are living ?

Maybe we can delay the switch until we decided what we actually want to show for kn service describe

Yes! I just wanted pitch and get feedback.

I wonder whether the proper spot for such a library would be cli-runtime

Correct, however doing that would eventually increase the timeline for being able to re-use it in kn. I think we should first target it for kn, which needs it sooner and then propose for inclusion in cli-runtime.

pkg/kn/commands/service_describe.go

navidshaikh · 2019-04-18T09:55:52Z

pkg/kn/commands/service_describe.go

 		Use:   "describe NAME",
-		Short: "Describe available services.",
+		Short: "Show details for a given service",


all the short help text in other commands ends with a period (.), lets add it to be consistent when kn help prints text.

navidshaikh · 2019-04-18T10:01:31Z

pkg/kn/commands/service_describe.go

+	*/
+}
+
+func getService(namespace string, name string, p *KnParams) (*v1alpha1.Service, error) {


We'll be using GetService in multiple commands, this is also a target for including in some package. Fine keeping it here for now.

navidshaikh · 2019-05-03T11:08:59Z

I could imagine to add the following additional fields in the output:

@rhuss : Lets proceed with the info you mentioned in the issue description.

cppforlife · 2019-05-14T17:43:44Z

@rhuss any plans to address @navidshaikh comments?

rhuss · 2019-05-15T14:39:31Z

@sixolet I'm going to pick up work this now again

@cppforlife yes, I will address @navidshaikh comments

As I'm currently on the road (conference), I think I will have an update here until the beginning of next week.

googlebot · 2019-05-27T21:03:10Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-28T15:55:31Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

maximilien · 2019-06-07T23:18:26Z

pkg/serving/client.go

+	return revisionSlice, nil
+}
+
+// ============================================================================


What's the meaning of these //====? To separate private functions/methods? If yes, I've been using simple // Private funcs or similar...

Yes, that's an old habit by me (to make it more visible). But happy to remove if this is too distracting.

No I am cool, though we should agree on one approach and use that consistently. Perhaps good to see other opinions on this...

maximilien · 2019-06-07T23:20:13Z

pkg/serving/client.go

+	namespace string
+}
+
+func NewNamespacedClient(client client_v1alpha1.ServingV1alpha1Interface, namespace string) *NamespacedClient {


Might want to include single line comments for /lint so something like:

// NewNamespacedClient creates a new client bound to namespace func NewNamespacedClient(client client_v1alpha1.ServingV1alpha1Interface, namespace string) *NamespacedClient { ... }

etc.

Good point, I will add those. BTW, I plan to split up this monster PR into multiple PRs (based on each other), and extract eg. the part with this "NamespacedClient" in a separate PR and combine it with #134

duglin · 2019-06-11T16:22:20Z

Rebase needed:

Overall I like the output of the default. I'm trying to think of a different word for "all" because in other contexts "all" typically means "more than one", but here you really mean "expanded view" or "non-compact". Perhaps --details? not sure but let's bikeshed a bit on it :-)
I'm not a fan of the hipster format - just seems a bit too cutsy and somewhat more confusing because people can easily interpret icons to mean something different than what was originally intended.

cppforlife · 2019-06-11T22:07:57Z

i would suggest changing this a bit to something like this:

have one view (instead of compact vs all) with a flag --revisions=x (x = 3 by default)
kill hipster (that's my personal pref)
figure out how to get --color to be global level configuration

rhuss · 2019-06-24T13:54:54Z

Agreed that --all is probably not the right name. I like --details (or maybe --verbose if this is not already occupied ?)

Having an option for restricting the number of revisions is also a good idea, however, it would be cool to get all revisions listed without trial and error to find the proper number of --revisions.

I see the fruit salad format has not many fans, so happy to remove the --hipster option if this is the common consensus.

I also agree that --color should be a global option influencing all output.

If there are no objections, I will remove colour and emoji options, so that we can think on a more general level how to introduce colour.

duglin · 2019-06-24T20:25:57Z

"fruit salad" : https://www.youtube.com/watch?v=iRu1Yp2br00&t=33

As discussed in knative#48 this `kn service describe` as changed in this commit mimics `kubectl describe <sth>` as that it output detail information in human readable output. Summary information of a list of services or a single services as well as exporting the service in various formats (JSON, YAML) is reserved to `kn service get` For this reason, the generic Printer handling from cli-runtime has been removed as `kn service describe` only outputs human readable format (that is btw also true for `kubectl` describe which does not allow for `-o` kind of options). This command shows information about the serivce itself, but also a summary about the associated revisions. "kn service describe" knows three modes, which can be combined: `--all` : Print more information. By default only are shorter summary is shown `--color` : Use a colorful output (but only when on a tty) `--hipster`: Experimental output mode for a very compact representation using emojis Of course there can be more and other information to be shown. Let's discuss and add this (and maybe remove the dense format)

knative-prow-robot · 2019-07-08T19:27:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss

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~~ [rhuss]

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

knative-metrics-robot · 2019-07-08T19:28:41Z

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/service_describe.go	78.3%	36.2%	-42.1
pkg/kn/commands/service/service_describe_compact.go	Do not exist	0.0%
pkg/serving/v1alpha1/client.go	86.0%	80.0%	-6.0

knative-prow-robot · 2019-07-08T19:50:59Z

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

Test name	Commit	Details	Rerun command
pull-knative-client-go-coverage	`e67694b`	link	`/test pull-knative-client-go-coverage`
pull-knative-client-integration-tests-latest-release	`e67694b`	link	`/test pull-knative-client-integration-tests-latest-release`
pull-knative-client-integration-tests	`e67694b`	link	`/test pull-knative-client-integration-tests`

Full PR test history. Your PR dashboard.

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.

rhuss · 2019-08-09T11:52:45Z

Closing it for now, might serve as a reference if colour or emojis should ever come back.

bye, bye hipster 🤓

knative-prow-robot requested review from cppforlife and sixolet April 17, 2019 13:08

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. labels Apr 17, 2019

rhuss mentioned this pull request Apr 17, 2019

service get #48

Closed

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

navidshaikh reviewed Apr 18, 2019

View reviewed changes

rhuss force-pushed the pr/get branch from e55642c to 0964d15 Compare May 15, 2019 16:36

navidshaikh mentioned this pull request May 27, 2019

kn service get <service-name> will list all the services #143

Closed

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2019

googlebot added the cla: no Indicates the PR's author has not signed the CLA. label May 27, 2019

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 28, 2019

rhuss force-pushed the pr/get branch from 75a5b39 to a44413c Compare May 28, 2019 15:55

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels May 28, 2019

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 28, 2019

rhuss mentioned this pull request May 30, 2019

refactor(serving): KnClient interface for single point of cluster access #134

Merged

rhuss force-pushed the pr/get branch from a44413c to 744872f Compare May 30, 2019 20:45

This was referenced Jun 2, 2019

List revisions for given service #127

Closed

The road to 0.2.0 #158

Closed

Change "get" to "list" #128

Closed

maximilien reviewed Jun 7, 2019

View reviewed changes

rhuss mentioned this pull request Jul 5, 2019

Fix issue #205 to add route describe feature and test #228

Closed

zhangtbj mentioned this pull request Jul 5, 2019

Implement kn revision describe command #206

Closed

rhuss added 2 commits July 8, 2019 21:10

chore(service): Rebased on master

e67694b

rhuss force-pushed the pr/get branch from 744872f to e67694b Compare July 8, 2019 19:26

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2019

rhuss mentioned this pull request Jul 9, 2019

feature(service describe): Output of service details #252

Merged

rhuss added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2019

rhuss changed the title ~~feature(service describe): Output of service details~~ feature(service describe): Output of service details [plain, color, hipster] Jul 10, 2019

rhuss changed the title ~~feature(service describe): Output of service details [plain, color, hipster]~~ feature(service describe): Output of service details [vanilla, color, hipster] Jul 10, 2019

rhuss closed this Aug 9, 2019

chmouel mentioned this pull request Sep 10, 2019

TaskRun Describe Command tektoncd/cli#303

Merged

3 tasks

rhuss mentioned this pull request Jan 27, 2022

Color Output #1589

Open

rhuss mentioned this pull request Nov 23, 2023

[48h Knative] Color output #1891

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(service describe): Output of service details [vanilla, color, hipster] #75

feature(service describe): Output of service details [vanilla, color, hipster] #75

rhuss commented Apr 17, 2019 •

edited

Loading

knative-prow-robot commented Apr 17, 2019

cppforlife commented Apr 17, 2019

navidshaikh Apr 18, 2019

rhuss Apr 18, 2019

navidshaikh Apr 18, 2019 •

edited

Loading

rhuss Apr 18, 2019

navidshaikh Apr 18, 2019 •

edited

Loading

navidshaikh Apr 18, 2019 •

edited

Loading

navidshaikh Apr 18, 2019

navidshaikh commented May 3, 2019

cppforlife commented May 14, 2019

rhuss commented May 15, 2019

googlebot commented May 27, 2019

googlebot commented May 28, 2019

maximilien Jun 7, 2019

rhuss Jun 8, 2019

maximilien Jun 10, 2019

maximilien Jun 7, 2019

rhuss Jun 8, 2019

duglin commented Jun 11, 2019

cppforlife commented Jun 11, 2019

rhuss commented Jun 24, 2019

duglin commented Jun 24, 2019

knative-prow-robot commented Jul 8, 2019

knative-metrics-robot commented Jul 8, 2019

knative-prow-robot commented Jul 8, 2019

rhuss commented Aug 9, 2019

feature(service describe): Output of service details [vanilla, color, hipster] #75

feature(service describe): Output of service details [vanilla, color, hipster] #75

Conversation

rhuss commented Apr 17, 2019 • edited Loading

kn service describe random

kn service describe random --all

kn service describe random --color

kn service describe random --all --color

kn service describe random --hipster

kn service describe random --hipster --all

kn service describe random --hipster --all --color

knative-prow-robot commented Apr 17, 2019

cppforlife commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navidshaikh Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navidshaikh Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

navidshaikh Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navidshaikh commented May 3, 2019

cppforlife commented May 14, 2019

rhuss commented May 15, 2019

googlebot commented May 27, 2019

googlebot commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Jun 11, 2019

cppforlife commented Jun 11, 2019

rhuss commented Jun 24, 2019

duglin commented Jun 24, 2019

knative-prow-robot commented Jul 8, 2019

knative-metrics-robot commented Jul 8, 2019

knative-prow-robot commented Jul 8, 2019

rhuss commented Aug 9, 2019

rhuss commented Apr 17, 2019 •

edited

Loading

`kn service describe random`

`kn service describe random --all`

`kn service describe random --color`

`kn service describe random --all --color`

`kn service describe random --hipster`

`kn service describe random --hipster --all`

`kn service describe random --hipster --all --color`

navidshaikh Apr 18, 2019 •

edited

Loading

navidshaikh Apr 18, 2019 •

edited

Loading

navidshaikh Apr 18, 2019 •

edited

Loading