Improvements to CLI usability and maintainability #34502

fabianofranz · 2016-10-10T22:39:03Z

Improves kubectl from an usability perspective by

Fixing how we handle terminal width in help. Some sections like the flags use the entire available width, while others like long descriptions breaks lines but don't follow a well established max width (screenshot below). This PR adds a new responsive writer that will adjust to terminal width and set 80, 100, or 120 columns as the max width, but not more than that given POSIX best practices and recommendations for better readability.
Adds our own normalizers for long descriptions and cmd examples which allows us better control about how things like lists, paragraphs, line breaks, etc are printed. Features markdown support. Looks like templates.LongDesc and templates.Examples instead of dedent.Dedend.
Allows simple reordering and reuse of help and usage sections.
Adds verify-cli-conventions.sh which intends to run tests to make sure cmd developers are using what we propose as kubectl conventions. Just a couple simple tests for now but the framework is there and it's easy to extend.
Update kubectl conventions to use our own normalizers instead of dedent.Dedent.

Release note:

Improves how 'kubectl' uses the terminal size when printing help and usage.

@kubernetes/kubectl

This change is

adohe-zz · 2016-10-10T23:33:29Z

@fabianofranz great job 👍

0xmichalis · 2016-10-11T11:07:18Z

pkg/kubectl/cmd/templates/command_groups.go

+func AddAdditionalCommands(g CommandGroups, message string, cmds []*cobra.Command) CommandGroups {
+	group := CommandGroup{Message: message}
+	for _, c := range cmds {
+		// Don't show commands that has no short description


s/has/have/

k8s-ci-robot · 2016-10-13T21:18:30Z

Jenkins verification failed for commit 832e644. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

adohe-zz · 2016-10-13T23:57:27Z

pkg/kubectl/cmd/annotate.go

-		It is intended to store non-identifying auxiliary data, especially data manipulated by tools and system extensions.
-		If --overwrite is true, then existing annotations can be overwritten, otherwise attempting to overwrite an annotation will result in an error.
-		If --resource-version is specified, then updates will use this resource version, otherwise the existing resource-version will be used.
+		* An annotation is a key/value pair that can hold larger (compared to a label), and possibly not human-readable, data.


why add * here?

It's list items. Not strictly required, but if we don't use list items here we have to either give each one its own paragraph, or make it all part of a single paragraph (no line wrapping). Markdown unwraps single line breaks.

adohe-zz · 2016-10-14T00:08:18Z

hack/verify-cli-conventions.sh

+set -o pipefail
+
+KUBE_ROOT=$(dirname "${BASH_SOURCE}")/..
+source "${KUBE_ROOT}/hack/lib/init.sh"


whether we should add this check to normal verify process? like check this when commit.

Yep, it does it already. verify runs everything that satisfies hack/verify-*.sh, which includes this one.

https://github.com/fabianofranz/kubernetes/blob/001e58e83a82f881b4a753f33d08908d885aaa2e/hack/make-rules/verify.sh#L90

fabianofranz · 2016-10-14T19:20:22Z

What else? I'd like to get this in when possible given rebases here are painful. :)

0xmichalis · 2016-10-17T09:27:30Z

pkg/kubectl/cmd/templates/markdown.go

+
+const linebreak = "\n"
+
+// ASCIIRenderer is a blackfriday rendered intended for rendering markdown


Can you add a

var _ blackfriday.Renderer = ASCIIRenderer{}

to make it explicit that we are conforming to an external interface?

0xmichalis · 2016-10-17T09:31:08Z

pkg/kubectl/cmd/templates/templates.go

 		`{{$rootCmd := rootCmd .}}` +
 		`{{$visibleFlags := visibleFlags (flagsNotIntersected .LocalFlags .PersistentFlags)}}` +
 		`{{$explicitlyExposedFlags := exposed .}}` +
 		`{{$optionsCmdFor := optionsCmdFor .}}` +
 		`{{$usageLine := usageLine .}}`

-	mainHelpTemplate = `{{with or .Long .Short }}{{. | trim}}{{end}}{{if or .Runnable .HasSubCommands}}{{.UsageString}}{{end}}`
+	SectionAliases = `{{if gt .Aliases 0}}Aliases:


Can you add godoc to these constants?

Godoc added.

0xmichalis · 2016-10-17T09:37:57Z

pkg/util/term/term_writer_test.go

+		w := NewMaxWidthWriter(b, tc.maxWidth)
+		_, err := w.Write([]byte(tc.input))
+		if err != nil {
+			t.Errorf("%s: Unexpected error: '%s'", k, err)


%v for errors - otherwise you can use %q to quote by default and use err.Error() instead of err

Good catch, fixed.

0xmichalis · 2016-10-17T09:38:49Z

Added a few more comments but looks good overall.

fabianofranz · 2016-10-17T13:32:54Z

@Kargakis all comments addressed, thanks!

fabianofranz · 2016-10-17T20:34:57Z

@janetkuo @adohe anything else?

0xmichalis · 2016-10-17T22:43:01Z

Tagging, additional comments can be addressed in follow-ups

k8s-github-robot · 2016-10-18T05:54:33Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-10-18T06:41:23Z

Automatic merge from submit-queue

…mprovements Automatic merge from submit-queue Improvements to CLI usability and maintainability Improves `kubectl` from an usability perspective by 1. Fixing how we handle terminal width in help. Some sections like the flags use the entire available width, while others like long descriptions breaks lines but don't follow a well established max width (screenshot below). This PR adds a new responsive writer that will adjust to terminal width and set 80, 100, or 120 columns as the max width, but not more than that given POSIX best practices and recommendations for better readability. ![terminal_width](https://cloud.githubusercontent.com/assets/158611/19253184/b23a983e-8f1f-11e6-9bae-667dd5981485.png) 2. Adds our own normalizers for long descriptions and cmd examples which allows us better control about how things like lists, paragraphs, line breaks, etc are printed. Features markdown support. Looks like `templates.LongDesc` and `templates.Examples` instead of `dedent.Dedend`. 3. Allows simple reordering and reuse of help and usage sections. 3. Adds `verify-cli-conventions.sh` which intends to run tests to make sure cmd developers are using what we propose as [kubectl conventions](https://github.com/kubernetes/kubernetes/blob/master/docs/devel/kubectl-conventions.md). Just a couple simple tests for now but the framework is there and it's easy to extend. 4. Update [kubectl conventions](https://github.com/kubernetes/kubernetes/blob/master/docs/devel/kubectl-conventions.md) to use our own normalizers instead of `dedent.Dedent`. **Release note**:  ```release-note Improves how 'kubectl' uses the terminal size when printing help and usage. ``` @kubernetes/kubectl

googlebot added the cla: yes label Oct 10, 2016

fabianofranz mentioned this pull request Oct 10, 2016

Improves cmd descriptions and how terminal size is handled on oc help openshift/origin#10993

Merged

k8s-github-robot assigned janetkuo Oct 10, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Oct 10, 2016

k8s-github-robot mentioned this pull request Oct 11, 2016

[k8s.io] Networking should check kube-proxy urls {Kubernetes e2e suite} #32436

Closed

0xmichalis reviewed Oct 11, 2016

View reviewed changes

fabianofranz force-pushed the cli_usability_improvements branch from e90490d to ad9e8d1 Compare October 11, 2016 17:04

fabianofranz mentioned this pull request Oct 11, 2016

UPSTREAM: 34028: Add --dry-run option to oc create sub-commands openshift/origin#11238

Merged

9 tasks

janetkuo added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 12, 2016

janetkuo assigned adohe-zz and 0xmichalis Oct 12, 2016

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

fabianofranz force-pushed the cli_usability_improvements branch from ad9e8d1 to 8e0dfdb Compare October 13, 2016 19:03

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

fabianofranz force-pushed the cli_usability_improvements branch from 8e0dfdb to 832e644 Compare October 13, 2016 20:39

k8s-github-robot mentioned this pull request Oct 13, 2016

[k8s.io] Deployment scaled rollout deployment should not block on annotation check {Kubernetes e2e suite} #34717

Closed

adohe-zz reviewed Oct 13, 2016

View reviewed changes

adohe-zz reviewed Oct 14, 2016

View reviewed changes

fabianofranz force-pushed the cli_usability_improvements branch 2 times, most recently from ad9e32e to ff913f0 Compare October 14, 2016 01:02

adohe-zz mentioned this pull request Oct 14, 2016

use dedent for config/get_contexts.go #32109

Closed

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

fabianofranz force-pushed the cli_usability_improvements branch from ff913f0 to 24784ef Compare October 14, 2016 14:13

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

k8s-github-robot mentioned this pull request Oct 14, 2016

[k8s.io] Port forwarding [k8s.io] With a server that expects a client request should support a client that connects, sends data, and disconnects [Conformance] {Kubernetes e2e suite} #27680

Closed

0xmichalis reviewed Oct 17, 2016

View reviewed changes

fabianofranz added 3 commits October 17, 2016 11:17

Add heredoc and go-wordwrap dependencies

8438089

Add responsive writers which adjust to terminal sizes

6d6aeb0

Add normalizers for cmd examples and descriptions

6f66c0e

fabianofranz force-pushed the cli_usability_improvements branch from 24784ef to c5044ac Compare October 17, 2016 13:32

fabianofranz added 4 commits October 17, 2016 11:49

Use our own normalizers for cmd examples and descriptions

f6d1ac7

Use responsive writer in help

4d641c4

Tools for checking CLI conventions

3f7579c

Use our own normalizers in the conventions doc

0fd5c8d

fabianofranz force-pushed the cli_usability_improvements branch from c5044ac to 0fd5c8d Compare October 17, 2016 13:50

0xmichalis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2016

k8s-github-robot merged commit c19569f into kubernetes:master Oct 18, 2016

lukaszo mentioned this pull request Oct 24, 2016

Fix formatting for generated md file #34225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to CLI usability and maintainability #34502

Improvements to CLI usability and maintainability #34502

fabianofranz commented Oct 10, 2016 •

edited

Loading

adohe-zz commented Oct 10, 2016

0xmichalis Oct 11, 2016

k8s-ci-robot commented Oct 13, 2016

adohe-zz Oct 13, 2016

fabianofranz Oct 14, 2016

adohe-zz Oct 14, 2016

fabianofranz Oct 14, 2016

fabianofranz commented Oct 14, 2016

0xmichalis Oct 17, 2016

fabianofranz Oct 17, 2016

0xmichalis Oct 17, 2016

fabianofranz Oct 17, 2016

0xmichalis Oct 17, 2016

fabianofranz Oct 17, 2016

0xmichalis commented Oct 17, 2016

fabianofranz commented Oct 17, 2016

fabianofranz commented Oct 17, 2016

0xmichalis commented Oct 17, 2016

k8s-github-robot commented Oct 18, 2016

k8s-github-robot commented Oct 18, 2016


		const linebreak = "\n"

		// ASCIIRenderer is a blackfriday rendered intended for rendering markdown

Improvements to CLI usability and maintainability #34502

Improvements to CLI usability and maintainability #34502

Conversation

fabianofranz commented Oct 10, 2016 • edited Loading

adohe-zz commented Oct 10, 2016

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 13, 2016

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

fabianofranz commented Oct 14, 2016

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

Choose a reason for hiding this comment

0xmichalis commented Oct 17, 2016

fabianofranz commented Oct 17, 2016

fabianofranz commented Oct 17, 2016

0xmichalis commented Oct 17, 2016

k8s-github-robot commented Oct 18, 2016

k8s-github-robot commented Oct 18, 2016

fabianofranz commented Oct 10, 2016 •

edited

Loading