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

fabianofranz · 2016-09-19T15:36:23Z

Related to https://trello.com/c/04lpfTQR

We currently have a number of issues related to how the terminal width is recognized and used in the help (and actual command output) of oc. Some places like the flags section uses the entire terminal width, cmd descriptions limit the width but it's manual and not consistent between commands.

This PR

Adds a new Writer that works in a responsive way by adjusting the help output according to the terminal size. Depending on the terminal width it will use 80, 100, or 120 columns as the max width, but not more than that given POSIX best practices and recommendations for better readability.
Adds markdown support to the cmd long descriptions, which allows us better control and normalization about how things like lists, paragraphs, line breaks, etc are printed.
Adds normalization to cmd examples.
Makes sure every command really uses the std and err outputs provided in the root commands, this is an issue today (only root-level commands are using it, anything under them in the cmd tree ignores the output provided to parents and use Stdout and Stderr).
Adds checks to guarantee all commands are following conventions.

Follow-ups

Upstream this
Reuse it from upstream (related: Reuse help templating from upstream #10957). We didn't do that yet because of the caveat of Usage at the bottom that kubectl uses, need to decide to either embrace it or add something like "help sections" that supports being reordered.
Adjust upstream commands descriptions to use LongDesc.
Use the responsive writer in all commands, not only help (need to check how it plays with tables like in get). We may still want wider output in some commands like that.

@openshift/cli-review @jwforres

smarterclayton · 2016-09-19T15:38:54Z

pkg/cmd/admin/admin.go

@@ -27,15 +27,15 @@ import (
 	exipfailover "github.com/openshift/origin/pkg/cmd/experimental/ipfailover"
 	"github.com/openshift/origin/pkg/cmd/server/admin"
 	"github.com/openshift/origin/pkg/cmd/templates"
+	. "github.com/openshift/origin/pkg/cmd/templates"


Don't use ., just use templates.LongDesc.

I'd also prefer that we be using heredoc is combination with this, where you do auto unwrapping of paragraphs.

Sounds good, it's already a dependency and kube uses it in some cmds. I'll include heredoc to the normalization logic.

Actually better to use dedent. heredoc aligns with the smallest indentation present, meaning it can still leave indentation I don't want when doing the unwrap.

smarterclayton · 2016-09-19T15:41:18Z

Get is moving to the server side so we'll be getting structured responses back (table of output). I think that'll probably be a similar use case, but different.

We really need wrapping with indentation to solve the large non help commands (long descriptions in describe). Can this do proper indentation?

smarterclayton · 2016-09-19T15:41:56Z

pkg/cmd/cli/cli.go

 	return cmds
 }

+func setOutput(cmd *cobra.Command, out io.Writer) {


This is a pretty drastic change - can you explain why this is necessary? Why would we not just require that all of the commands set output properly?

Cascading the writer to the entire command tree would allow us, for example, to extend the use of the responsive writer for every command output (not just help) by setting it only in the root cmd.

Although it makes sense at a first glance, there are places (e.g. tables in get) where using the max width possible also probably makes sense, so I ended up not using this and only enabling the responsive writer in help.

I'll remove this block and let us set it individually in commands where we want it.

fabianofranz · 2016-10-05T20:20:13Z

Removing WIP here, this is good to review @openshift/cli-review @smarterclayton @jwforres

Can this do proper indentation?

It doesn't fix the issue with indentation + long lines wrapping, but I don't think it would be hard to extend it to add something like that. It does, for example, proper indentation on multi-level lists, which is a similar concept.

fabianofranz · 2016-10-05T20:46:18Z

[test]

fabianofranz · 2016-10-07T18:42:13Z

Flaked on #9548 and #9490
re[test]

fabianofranz · 2016-10-10T22:41:59Z

Upstreaming in kubernetes/kubernetes#34502.

fabianofranz · 2016-10-13T19:18:17Z

UP needs review

fabianofranz · 2016-10-14T01:03:28Z

flake #11240 re[test]

fabianofranz · 2016-10-14T18:28:57Z

flake #11240 re[test]

marun · 2016-10-14T19:18:28Z

flake #11374 re-[test]

fabianofranz · 2016-10-15T00:03:31Z

flake #11240 re[test]

…20eefad42b8

fabianofranz · 2016-10-18T18:07:21Z

Upstream merged, rebased here.
[merge]

openshift-bot · 2016-10-18T18:07:31Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10205/) (Image: devenv-rhel7_5206)

openshift-bot · 2016-10-18T18:07:33Z

Evaluated for origin merge up to fc72a9d

fabianofranz · 2016-10-18T19:56:07Z

flake #11240 re[test]

openshift-bot · 2016-10-18T19:57:34Z

Evaluated for origin test up to fc72a9d

openshift-bot · 2016-10-18T21:17:11Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10205/) (Base Commit: a941850)

smarterclayton reviewed Sep 19, 2016

View reviewed changes

fabianofranz force-pushed the 451_cli_usability_improvements branch 2 times, most recently from 4fc5e8d to f5f1c6e Compare September 19, 2016 20:09

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2016

fabianofranz force-pushed the 451_cli_usability_improvements branch from 14f23b7 to e82599a Compare October 3, 2016 19:29

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2016

fabianofranz force-pushed the 451_cli_usability_improvements branch 3 times, most recently from d8e41d3 to 89e31f9 Compare October 5, 2016 00:45

fabianofranz changed the title ~~[WIP] Improves cmd descriptions and how terminal size in handled on oc help~~ [WIP] Improves cmd descriptions and how terminal size is handled on oc help Oct 5, 2016

fabianofranz force-pushed the 451_cli_usability_improvements branch 5 times, most recently from 1587637 to 4b4c5a5 Compare October 5, 2016 20:13

fabianofranz changed the title ~~[WIP] Improves cmd descriptions and how terminal size is handled on oc help~~ Improves cmd descriptions and how terminal size is handled on oc help Oct 5, 2016

fabianofranz force-pushed the 451_cli_usability_improvements branch from ea0bdeb to 4b4c5a5 Compare October 5, 2016 20:45

fabianofranz force-pushed the 451_cli_usability_improvements branch 6 times, most recently from ef0bd5d to 5309135 Compare October 7, 2016 14:53

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2016

fabianofranz force-pushed the 451_cli_usability_improvements branch from a2ca7d0 to 47ef779 Compare October 11, 2016 18:29

fabianofranz mentioned this pull request Oct 11, 2016

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

Merged

9 tasks

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2016

fabianofranz force-pushed the 451_cli_usability_improvements branch from 47ef779 to 284773c Compare October 13, 2016 19:04

fabianofranz force-pushed the 451_cli_usability_improvements branch from 284773c to 8dcddb0 Compare October 13, 2016 19:40

fabianofranz force-pushed the 451_cli_usability_improvements branch 2 times, most recently from 585cba3 to 0aaf947 Compare October 14, 2016 21:30

fabianofranz added 9 commits October 18, 2016 13:37

bump(github.com/mitchellh/go-wordwrap): ad45545899c7b13c020ea92b20722…

a298bd1

…20eefad42b8

Add writers capable of adjusting to terminal sizes

6366c25

Commands must always use the provided err output

dd473fd

Markdown rendering engine

53bccb7

Normalize CLI examples and long descriptions

6267dde

Tools for checking CLI conventions

f428ec4

Update generated docs

7b71b98

Help templates which allow reordering of sections

db3cf28

Update the cli hacking guide

fc72a9d

fabianofranz force-pushed the 451_cli_usability_improvements branch from 0aaf947 to fc72a9d Compare October 18, 2016 17:45

openshift-bot merged commit 9668d38 into openshift:master Oct 19, 2016

fabianofranz mentioned this pull request Oct 20, 2016

oc in windows: cli help wraps awkwardly in default windows console #5336

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fabianofranz commented Sep 19, 2016 •

edited

Loading

smarterclayton Sep 19, 2016

fabianofranz Sep 19, 2016

fabianofranz Sep 21, 2016

smarterclayton commented Sep 19, 2016

smarterclayton Sep 19, 2016

fabianofranz Sep 19, 2016

fabianofranz Oct 5, 2016

fabianofranz commented Oct 5, 2016 •

edited

Loading

fabianofranz commented Oct 5, 2016

fabianofranz commented Oct 7, 2016

fabianofranz commented Oct 10, 2016

fabianofranz commented Oct 13, 2016

fabianofranz commented Oct 14, 2016

fabianofranz commented Oct 14, 2016

marun commented Oct 14, 2016

fabianofranz commented Oct 15, 2016

fabianofranz commented Oct 18, 2016

openshift-bot commented Oct 18, 2016 •

edited

Loading

openshift-bot commented Oct 18, 2016

fabianofranz commented Oct 18, 2016

openshift-bot commented Oct 18, 2016

openshift-bot commented Oct 18, 2016

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

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

Conversation

fabianofranz commented Sep 19, 2016 • edited Loading

smarterclayton Sep 19, 2016

Choose a reason for hiding this comment

fabianofranz Sep 19, 2016

Choose a reason for hiding this comment

fabianofranz Sep 21, 2016

Choose a reason for hiding this comment

smarterclayton commented Sep 19, 2016

smarterclayton Sep 19, 2016

Choose a reason for hiding this comment

fabianofranz Sep 19, 2016

Choose a reason for hiding this comment

fabianofranz Oct 5, 2016

Choose a reason for hiding this comment

fabianofranz commented Oct 5, 2016 • edited Loading

fabianofranz commented Oct 5, 2016

fabianofranz commented Oct 7, 2016

fabianofranz commented Oct 10, 2016

fabianofranz commented Oct 13, 2016

fabianofranz commented Oct 14, 2016

fabianofranz commented Oct 14, 2016

marun commented Oct 14, 2016

fabianofranz commented Oct 15, 2016

fabianofranz commented Oct 18, 2016

openshift-bot commented Oct 18, 2016 • edited Loading

openshift-bot commented Oct 18, 2016

fabianofranz commented Oct 18, 2016

openshift-bot commented Oct 18, 2016

openshift-bot commented Oct 18, 2016

fabianofranz commented Sep 19, 2016 •

edited

Loading

fabianofranz commented Oct 5, 2016 •

edited

Loading

openshift-bot commented Oct 18, 2016 •

edited

Loading