Add grouping for help message + streamlined help messages #887

rhuss · 2020-06-16T09:30:06Z

The top-level looks like

kn is the command line interface for managing Knative Serving and Eventing resources

 Find more information about Knative at: https://knative.dev

Serving Commands:
  service     Manage Knative services
  revision    Manage service revisions
  route       List and show service routes

Eventing Commands:
  source      Manage event sources
  trigger     Manage event triggers

Other Commands:
  plugin      Manage kn plugins
  completion  Output shell completion code
  version     Show the version of this client

Use "kn <command> --help" for more information about a given command.
Use "kn options" for a list of global command-line options (applies to all commands).

The following changes have been applied:

Add CommandGroups for grouping commands together
Add flexible templating for the help messages
Moved global options to an own command ('kn options', much like 'kubectl options')
Aligned wording and typography of help messages

These features has been highly inspired by kubectl grouping & help templating but has been considerably been stripped down to the needs of kn.

Fixes #579

The top-level looks like kn is the command line interface for managing Knative Serving and Eventing objects Find more information about Knative at: https://knative.dev Serving Commands: service Manage Knative services revision Manage service revisions route List and show service routes Eventing Commands: source Manage event sources trigger Manage event triggers Other Commands: plugin Manage kn plugins completion Output shell completion code version Show the version of this client Use "kn <command> --help" for more information about a given command. Use "kn options" for a list of global command-line options (applies to all commands). The following changes have been applied: * Add CommandGroups for grouping commands together * Add flexible templating for the help messages * Moved global options to an own command ('kn options', much like 'kubectl options') * Aligned wording and typography of help messages These features has been highly inspired by kubectl grouping & help templating but has been considerably been stripped down to the needs of kn. Signed-off-by: Roland Huß <[email protected]>

rhuss · 2020-06-16T09:31:09Z

Please note that testing still needs to be added, this PR is however already open for review to gather early feedback.

Co-authored-by: Matt Moore <[email protected]>

danielhelfand

Tested locally for #885 and seems to resolve the issue.

Some general comments would be to check for error messages that already include the prefix to avoid double prints, such as here. Maybe that's handled, but in general should be removed and discouraged in the future.

Also, maybe including the error prefix in some tests is worthwhile as a means of verifying the error prefix?

Edit: I see the note on testing.

Looks good to me otherwise.

rhuss · 2020-06-16T19:28:45Z

Some general comments would be to check for error messages that already include the prefix to avoid double prints, such as here. Maybe that's handled, but in general should be removed and discouraged in the future.

Since there is a single exit point we are free to mangle the error message as we want. But I consider this more like a workaround since adding an "error: " prefix in an error message looks quite redundant (it's clear that it is an error if the prefix is part of an error message, also the formatting of the error message should be left to the layer which deals with the error). However, we don't have much influence on the wording of errors returned from library calls.

I will add some tests to check the error wording.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v wire_gen.go)

cmd/kn/main.go

cmd/kn/main_test.go

rhuss · 2020-06-17T06:19:33Z

Added the missing testing bits.

navidshaikh · 2020-06-17T07:01:36Z

/assign

rhuss · 2020-06-17T10:33:08Z

/retest

pkg/kn/commands/completion/completion.go

pkg/kn/commands/source/apiserver/delete.go

pkg/kn/commands/service/list.go

pkg/kn/commands/service/delete.go

pkg/kn/commands/route/route.go

pkg/kn/commands/revision/delete.go

pkg/kn/commands/plugin/plugin.go

cmd/kn/main_test.go

Co-authored-by: Navid Shaikh <[email protected]>

pkg/kn/root/root.go

pkg/templates/command_groups.go

navidshaikh · 2020-06-17T13:57:26Z

pkg/templates/command_groups.go

+	rootCmd.SetHelpFunc(engine.helpFunc())
+}
+
+func setHelpFlagsToSubCommands(parent *cobra.Command) {


this is removing [flags] from the usage line, we may rename this to disableFlagsInUseLine ?

but it adds "[options]" instead of "[flags]" --> https://github.com/rhuss/knative-client/blob/41338722cdc2124dda2b3a594527749a31d8b735/pkg/templates/template_engine.go#L134

Again, inline with kubectl and "options" is a good wording here, as all the rest is just optional. Everything mandatory should be mentioned in the "Use" attribute.

pkg/templates/templates.go

maximilien · 2020-06-17T23:34:38Z

/test pull-knative-client-integration-tests-latest-release

maximilien

Overall looks good. I like the consolidations and some simplication though there are various places where text is repeated and/or not adding anything useful, perhaps best to omit or remove, .e.g, Synopis of source apiserver and various places. Left a few other specific comments.

Did not review the templating section. I see that @navidshaikh is reviewing. I might take a pass after.

maximilien · 2020-06-17T23:48:18Z

docs/cmd/kn_plugin.md


 ### Synopsis

-Provides utilities for interacting and managing with kn plugins.
+Manage kn plugins


Should we also mention the client-contrib here as a place to find "vetted" plugins?

👍 for this but maybe we should have a first release for the client-contrib plugins. We can add this info any time with a link to a consolidated list.

maximilien · 2020-06-17T23:55:11Z

docs/cmd/kn_source_apiserver_list.md

@@ -1,13 +1,13 @@
 ## kn source apiserver list


shouldn't this read: "## kn source api-server list" ?

see comment below.

maximilien · 2020-06-17T23:57:51Z

docs/cmd/kn_source_binding_delete.md


 ### Synopsis

-Delete a sink binding.
+Delete a sink binding


Wonder how useful the Synopsis is when its the same text as the general description?

I think this is due to the way how the docs are generated and that the BindingDeleteCommand doesn't have a Long description (so the short description is reused).

We should add to every command a longer descriptions (if it makes sense, here e.g. one could add a brief explanation what a sink binding is, maybe with a link to the binding docs).

As this PR doesn't change anything with regard to the Long description, I'd suggest to open a new issue for this and work separately on consolidating the long descriptions.

maximilien · 2020-06-18T00:10:52Z

pkg/kn/commands/source/apiserver/apiserver.go

@@ -26,8 +26,8 @@ import (
 // NewAPIServerCommand for managing ApiServer source
 func NewAPIServerCommand(p *commands.KnParams) *cobra.Command {
 	apiServerSourceCmd := &cobra.Command{
-		Use:   "apiserver",
-		Short: "Kubernetes API Server Event Source command group",
+		Use:   "apiserver COMMAND",


Shouldn't this be api-server COMMAND?

We settled on apiserver for the command (like in kn source apiserver list), so changing it now would be backwards incompatible. We could do this if its improve the UX considerably but should be aware that it involves more work than just renaming it here.

What we can do though is to add an alias api-server for apiserver alongside the suggestions in #812 .

:+1 for alias

maximilien · 2020-06-18T00:13:05Z

pkg/kn/commands/testing_helper.go

-type StdoutCapture struct {
-	r, w *os.File
-	t    *testing.T
+type OutputCapture struct {


Perhaps this testing_helper.go should move to lib/test to make it useable in client-contrib plugins?

Good idea, going to move it over there.

I just moved the OutputCapture to lib/test.

I think the other parts are more for internally testing KnCommands that is specific of how kn organizes it commands (also there might be some change here, too).

knative-metrics-robot · 2020-06-18T06:11:20Z

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

File	Old Coverage	New Coverage	Delta
cmd/kn/main.go	77.0%	78.1%	1.0
pkg/kn/commands/testing_helper.go	91.1%	87.9%	-3.2
pkg/kn/root/root.go	87.2%	89.7%	2.5
pkg/templates/command_groups.go	Do not exist	90.9%
pkg/templates/template_engine.go	Do not exist	87.5%
pkg/templates/templates.go	Do not exist	100.0%

navidshaikh

/retest
/lgtm
/approve

thanks!
aliases could be the next thing to look at..

knative-prow-robot · 2020-06-18T06:38:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh, 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~~ [navidshaikh,rhuss]

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

navidshaikh · 2020-06-18T06:39:10Z

@rhuss : CHANGELOG missing, lets bring it in another PR

rhuss · 2020-06-18T08:54:56Z

Thanks !

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 16, 2020

knative-prow-robot requested review from mattmoor and sixolet June 16, 2020 09:30

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 16, 2020

mattmoor reviewed Jun 16, 2020

View reviewed changes

chore: Add missing file

8f7d646

mattmoor reviewed Jun 16, 2020

View reviewed changes

Update pkg/templates/command_groups.go

aaf97c8

Co-authored-by: Matt Moore <[email protected]>

rhuss mentioned this pull request Jun 16, 2020

Error Prefix Not Shown With Error Messages #885

Closed

danielhelfand reviewed Jun 16, 2020

View reviewed changes

chore: Add some test for error messages

20f35f6

mattmoor reviewed Jun 16, 2020

View reviewed changes

cmd/kn/main.go Outdated Show resolved Hide resolved

cmd/kn/main_test.go Outdated Show resolved Hide resolved

cmd/kn/main_test.go Outdated Show resolved Hide resolved

cmd/kn/main_test.go Outdated Show resolved Hide resolved

rhuss added 2 commits June 16, 2020 22:31

fix formatting

1676df6

chore: Add test

f2a93e3

moar tests

4133872

knative-prow-robot assigned navidshaikh Jun 17, 2020

navidshaikh reviewed Jun 17, 2020

View reviewed changes

rhuss and others added 6 commits June 17, 2020 14:50

Update pkg/kn/commands/completion/completion.go

c57267b

Co-authored-by: Navid Shaikh <[email protected]>

Update pkg/kn/commands/source/apiserver/delete.go

98f46e5

Co-authored-by: Navid Shaikh <[email protected]>

Update pkg/kn/commands/service/list.go

48ac48a

Co-authored-by: Navid Shaikh <[email protected]>

Update pkg/kn/commands/route/route.go

6b8067d

Co-authored-by: Navid Shaikh <[email protected]>

Update pkg/kn/commands/revision/delete.go

dd61c33

Co-authored-by: Navid Shaikh <[email protected]>

Update pkg/kn/commands/plugin/plugin.go

ea526fd

Co-authored-by: Navid Shaikh <[email protected]>

rhuss and others added 3 commits June 17, 2020 14:55

Update pkg/kn/commands/service/delete.go

8fa097f

Co-authored-by: Navid Shaikh <[email protected]>

Update pkg/kn/commands/revision/delete.go

377cf19

Co-authored-by: Navid Shaikh <[email protected]>

Update pkg/kn/commands/service/delete.go

305d9ca

Co-authored-by: Navid Shaikh <[email protected]>

navidshaikh reviewed Jun 17, 2020

View reviewed changes

pkg/kn/root/root.go Show resolved Hide resolved

regen docs

ee71d82

navidshaikh reviewed Jun 17, 2020

View reviewed changes

chore: Update conventions doc

048aaf5

maximilien suggested changes Jun 18, 2020

View reviewed changes

rhuss added 2 commits June 18, 2020 06:44

Move some direct configuration of rootcmd to NewRootCommand()

a30f9b4

Moved CaptureOutput to "test" package for reuse

fdc9704

rhuss force-pushed the p/ghelp-3 branch from 738d57a to fdc9704 Compare June 18, 2020 06:08

navidshaikh approved these changes Jun 18, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2020

knative-prow-robot merged commit b916a5b into knative:master Jun 18, 2020

dsimansk pushed a commit to dsimansk/client that referenced this pull request May 3, 2022

Remove myself from client-approvers/reviewers (knative#887)

cdac15d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add grouping for help message + streamlined help messages #887

Add grouping for help message + streamlined help messages #887

rhuss commented Jun 16, 2020 •

edited

Loading

rhuss commented Jun 16, 2020

danielhelfand left a comment •

edited

Loading

rhuss commented Jun 16, 2020

mattmoor left a comment

rhuss commented Jun 17, 2020

navidshaikh commented Jun 17, 2020

rhuss commented Jun 17, 2020

navidshaikh Jun 17, 2020

rhuss Jun 17, 2020

maximilien commented Jun 17, 2020

maximilien left a comment •

edited

Loading

maximilien Jun 17, 2020

rhuss Jun 18, 2020

maximilien Jun 17, 2020

rhuss Jun 18, 2020

maximilien Jun 17, 2020

rhuss Jun 18, 2020

maximilien Jun 18, 2020

rhuss Jun 18, 2020

navidshaikh Jun 18, 2020

maximilien Jun 18, 2020

rhuss Jun 18, 2020

rhuss Jun 18, 2020

knative-metrics-robot commented Jun 18, 2020

navidshaikh left a comment

knative-prow-robot commented Jun 18, 2020

navidshaikh commented Jun 18, 2020

rhuss commented Jun 18, 2020

Add grouping for help message + streamlined help messages #887

Add grouping for help message + streamlined help messages #887

Conversation

rhuss commented Jun 16, 2020 • edited Loading

rhuss commented Jun 16, 2020

danielhelfand left a comment • edited Loading

Choose a reason for hiding this comment

rhuss commented Jun 16, 2020

mattmoor left a comment

Choose a reason for hiding this comment

rhuss commented Jun 17, 2020

navidshaikh commented Jun 17, 2020

rhuss commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien commented Jun 17, 2020

maximilien left a comment • edited Loading

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

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

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 18, 2020

navidshaikh left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jun 18, 2020

navidshaikh commented Jun 18, 2020

rhuss commented Jun 18, 2020

rhuss commented Jun 16, 2020 •

edited

Loading

danielhelfand left a comment •

edited

Loading

maximilien left a comment •

edited

Loading