Add optional colors for kubectl printing... #40963

brendandburns · 2017-02-04T07:07:28Z

@kubernetes/sig-cli-pr-reviews

This PR adds an initial path for optional color support.

Currently:

kubectl get pods --color

Will print colorized output.

Eventually, I will add an environment variable as well. (and colors for other things)

k8s-reviewable · 2017-02-04T07:08:34Z

This change is

k8s-github-robot · 2017-02-04T07:18:37Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: brendandburns

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

~~OWNERS~~ [brendandburns]
~~pkg/kubectl/OWNERS~~ [brendandburns]

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

brendandburns · 2017-02-04T16:49:13Z

smarterclayton · 2017-02-04T19:01:45Z

Why color the name column instead of the status column? The users eye will need to go the status column anyway on a failure, and so this actually makes it harder to spot invalid items in that case (makes the user do more work, because the color will pull the eye). Likewise, for normal operation the color against a black background makes the name harder to read in the "normal" case, which makes the user do more work to scan a list.

smarterclayton · 2017-02-04T19:04:15Z

Generally you want color to pull the eye to the spot that is abnormal, not to anchor it on the things that are "normal". I might suggest we want the entire row to be bold on error, and normal weight on normal status.

Finally, all this is moving to the server side, so doing this now is putting extra weight on clients now that complicates that design (or is at minimum an extra requirement). How is this going to work for third party resources? I'd like to keep this really minimal for right now, and not complicate those other efforts that are actually required in the short term.

brendandburns · 2017-02-05T00:20:43Z

@smarterclayton we're moving human readable printing server side? That can't be right...

Feedback on the colors and coloring the name vs. the status makes sense to me, though I think there's a degree to wanting a measure of the pod "health" in general.

e.g. number of restarts, number of containers running, etc. all contribute to a pods "healthiness"

I worry that we'll end up with lots of different colors if we don't focus on coloring the entire row a single shade...

(I'd be open to coloring the entire row, or restricting coloring only to "abnormal" rows)

0xmichalis · 2017-02-05T08:47:20Z

Is this going to work consistently across different terminals? Is it something that has been discussed in @kubernetes/sig-cli-misc ?

juanvallejo · 2017-02-06T16:08:32Z

pkg/kubectl/resource_printer.go

+		podColor = "<fg 11>" // yellow
+	}
+	if readyContainers == 0 {
+		podColor = "<fg 1>" // red


@brendandburns

I worry that we'll end up with lots of different colors if we don't focus on coloring the entire row a single shade...

Based on this comment, and @smarterclayton's feedback, perhaps we could keep red for rows where the pod status is failing, and no color / how it is currently for any other case. WDYT?

Yeah, that works for me. I'd love to have yellow for "warning" (pending, some restarts recently, etc) and "red" for "not working"

I'll update the pr.

Still haven't seen this one addressed. Additionally if I add watch flag I'm getting:

$ kubectl get pods --color -w NAME READY STATUS RESTARTS AGE <fg 2>hello-5b209<reset> 1/1 Running 0 18s

and that's definitely not what I'd expect.

I understand the benefit, but we need to reconcile this with kubernetes/community#363

brendandburns · 2017-02-07T00:14:32Z

@Kargakis this should work across all Xterm capable terminals. It is off by default as well (and I think it will be off by default for a long time) so I think we don't have to have perfect fidelity everywhere, since it will only affect people who opt into it.

brendandburns · 2017-02-07T00:15:36Z

Also, @smarterclayton considering the server side bits, the colors actually come as a markup language, so I think that will actually make it easier for the server side, since the server can return markup colors and the client can figure out how to render them.

soltysh · 2017-02-09T08:35:48Z

My $0.02, why coloring would ever belong to server side? I can understand the desire to have a consistent output across client apps, but coloring should belong to client app, imho. The server side printing is a whole different story and I do have a mixed feelings about it, too.

👍 for having color emphasize the abnormal state and none for ok.

brendandburns · 2017-02-12T06:46:27Z

@soltysh I agree entirely, and honestly this is why I have significant concerns about server-side printing.

However, if we move table-based printing to the server, the information needed to do the color-based decision making will be lost and stored on the server, not in the client, so the server would be the only place to make that decision.

brendandburns · 2017-02-16T04:18:51Z

Friendly ping on this.

Thanks
--brendan

fabianofranz · 2017-02-20T21:32:00Z

I like the idea of making it opt-in at least until it stabilizes.

Any strong reason for choosing that particular library? When I R&D about colors a few months ago I found https://github.com/fatih/color, it supports Windows cmd.exe and can also make the existing fmt printing functions colorizable (although I didn't check how that was implemented). Not strong about one or another though.

fabianofranz · 2017-02-20T21:32:34Z

@k8s-bot test this

k8s-ci-robot · 2017-02-20T22:30:20Z

@brendandburns: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins unit/integration	`a583455`	link	`@k8s-bot unit test this`
Jenkins verification	`a583455`	link	`@k8s-bot verify test this`
Jenkins kops AWS e2e	`a583455`	link	`@k8s-bot kops aws e2e test this`
Jenkins GCI GCE e2e	`a583455`	link	`@k8s-bot gci gce e2e test this`
Jenkins non-CRI GCE e2e	`a583455`	link	`@k8s-bot non-cri e2e test this`
Jenkins GCE etcd3 e2e	`a583455`	link	`@k8s-bot gce etcd3 e2e test this`
Jenkins GCE e2e	`a583455`	link	`@k8s-bot cvm gce e2e test this`

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.

Details

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.

soltysh · 2017-02-22T14:00:31Z

However, if we move table-based printing to the server, the information needed to do the color-based decision making will be lost and stored on the server, not in the client, so the server would be the only place to make that decision.

That will only be true for the extended printing options, that @smarterclayton is proposing, right?

soltysh · 2017-02-23T11:02:54Z

pkg/kubectl/resource_printer.go

+		podColor = "<fg 11>" // yellow
+	}
+	if readyContainers == 0 {
+		podColor = "<fg 1>" // red


Still haven't seen this one addressed. Additionally if I add watch flag I'm getting:

$ kubectl get pods --color -w NAME READY STATUS RESTARTS AGE <fg 2>hello-5b209<reset> 1/1 Running 0 18s

and that's definitely not what I'd expect.

brendandburns · 2017-02-27T05:18:05Z

@fabianofranz I actually started this PR using https://github.com/fatih/color.

The trouble that I ran into was that the colorization characters broke the golang TabWriter class, see this question: http://stackoverflow.com/questions/35398497/how-do-i-get-colors-to-work-with-golang-tabwriter for someone who ran into the same problem.

Because this library uses markup, and you can train TabWriter to ignore HTML characters this library actually works and also does the right things with indents.

@soltysh will work on getting the comments/problems addressed.

Thanks!

k8s-github-robot · 2017-02-27T05:18:12Z

@brendandburns PR needs rebase

brendandburns · 2017-03-13T04:49:14Z

fwiw, it turns out we already colorize:

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/clusterinfo.go#L126

smarterclayton · 2017-03-13T06:04:43Z

I'd like this to be done under the context of moving to the server side, rather than be add before it lands and complicate it. I think color will help some, but I don't want to add more features to printing until the proposals are approved for server side, and no one is broken without it. Hopefully this has enough context that you can describe in the proposal what usecases are necessary (at least at a first glance, we want to flag cells with informational bits like "alert" or "warn"). On Mar 13, 2017, at 12:49 AM, Brendan Burns <[email protected]> wrote: fwiw, it turns out we already colorize: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/clusterinfo.go#L126 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40963 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p5V9a_C44wVmETjIPtmkAXb7W96Gks5rlMrRgaJpZM4L3EaZ> .

smarterclayton · 2017-03-13T06:30:02Z

As the carrot, the backend glue is all prototyped, so working together could get it into place fairly quickly. On Mar 13, 2017, at 12:49 AM, Brendan Burns <[email protected]> wrote: fwiw, it turns out we already colorize: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/clusterinfo.go#L126 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40963 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p5V9a_C44wVmETjIPtmkAXb7W96Gks5rlMrRgaJpZM4L3EaZ> .

brendandburns · 2017-03-15T03:41:40Z

@smarterclayton where is the proposal for server-side printing?

I'm happy to pile on there.

soltysh · 2017-03-15T12:20:42Z

@brendandburns proposal: kubernetes/community#363, prototype: #40848

bgrant0607 · 2017-04-14T17:27:55Z

pkg/kubectl/cmd/util/factory_object_mapping.go

 			ShowLabels:         GetFlagBool(cmd, "show-labels"),
 			AbsoluteTimestamps: isWatch(cmd),
 			ColumnLabels:       columnLabel,
+			Color:              GetFlagBool(cmd, "color"),


Does this detect whether a terminal is attached or not, as well?

bgrant0607 · 2017-04-14T17:29:50Z

pkg/kubectl/cmd/get.go

 	"k8s.io/kubernetes/pkg/util/i18n"
 	"k8s.io/kubernetes/pkg/util/interrupt"
+
+	"github.com/reconquest/loreley"


AFAICT, this doesn't have a license. We can't merge this.

We need a verify script to look for this type of thing. It's repeatedly caused us grief.

Filed #44505

bgrant0607 · 2017-04-14T17:37:05Z

BTW, I also want to nuke the kubeconfig preferences section, which contains only the colors preference currently. #10693

k8s-github-robot · 2017-07-14T00:52:04Z

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 @brendandburns @fabianofranz

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

brendandburns added 2 commits February 3, 2017 23:04

Add github.com/reconquest/loreley dependency

d28e25f

Add initial paths for colorizing output.

f32195f

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

update bazel.

a583455

k8s-github-robot assigned fabianofranz Feb 4, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Feb 4, 2017

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

juanvallejo reviewed Feb 6, 2017

View reviewed changes

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

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

soltysh requested changes Feb 23, 2017

View reviewed changes

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

bgrant0607 reviewed Apr 14, 2017

View reviewed changes

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2017

k8s-github-robot closed this Jul 14, 2017

Add optional colors for kubectl printing... #40963

Add optional colors for kubectl printing... #40963

Uh oh!

Conversation

brendandburns commented Feb 4, 2017 • edited by k8s-github-robot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-reviewable commented Feb 4, 2017

Uh oh!

k8s-github-robot commented Feb 4, 2017

Uh oh!

brendandburns commented Feb 4, 2017

Uh oh!

smarterclayton commented Feb 4, 2017

Uh oh!

smarterclayton commented Feb 4, 2017

Uh oh!

brendandburns commented Feb 5, 2017

Uh oh!

0xmichalis commented Feb 5, 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!

brendandburns commented Feb 7, 2017

Uh oh!

brendandburns commented Feb 7, 2017

Uh oh!

soltysh commented Feb 9, 2017

Uh oh!

brendandburns commented Feb 12, 2017

Uh oh!

brendandburns commented Feb 16, 2017

Uh oh!

fabianofranz commented Feb 20, 2017

Uh oh!

fabianofranz commented Feb 20, 2017

Uh oh!

k8s-ci-robot commented Feb 20, 2017

Uh oh!

soltysh commented Feb 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Feb 27, 2017

Uh oh!

k8s-github-robot commented Feb 27, 2017

Uh oh!

brendandburns commented Mar 13, 2017

Uh oh!

smarterclayton commented Mar 13, 2017 via email

Uh oh!

smarterclayton commented Mar 13, 2017 via email

Uh oh!

brendandburns commented Mar 15, 2017

Uh oh!

soltysh commented Mar 15, 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!

bgrant0607 commented Apr 14, 2017

Uh oh!

k8s-github-robot commented Jul 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

brendandburns commented Feb 4, 2017 •

edited by k8s-github-robot

Loading