Display help text on empty subcommand by default #11953

markusthoemmes · 2021-10-13T09:56:07Z

What this PR does / why we need it:

This is a small quality-of-life improvement, where instead of guiding the user to use --help when they didn't enter a specific subcommand, it shows the help text by default.

https://github.com/kubernetes/kubernetes/blob/efce40b931b5250498b9d8bcd67f987095ab6f74/hack/lib/util.sh#L714 in Kubernetes inspired me to do this. The docker CLI is behaving this way as well and Kubernetes uses this to check if docker buildx exists. I suppose the K8s side could potentially change in this case as well, but I wanted to see if y'all think this is a worthwhile improvement for human users anyway.

How to verify it

Run podman. It used to output

Error: missing command 'podman COMMAND'
Try 'podman --help' for more information.

With this it prints the help text immediately.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

I realize that this might be contentious in nature and I haven't found an issue relating to this. Please let me know if we should instead open an issue and discuss there first.

Signed-off-by: Markus Thömmes <[email protected]>

markusthoemmes · 2021-10-13T09:59:13Z

/assign @rhatdan

rhatdan · 2021-10-13T19:28:44Z

LGTM
Have to either add tests or add [NO NEW TESTS NEEDED] message to commit.

TomSweeneyRedHat · 2021-10-13T23:49:22Z

I'm on the fence on this one. I like the general idea, but the output from the help command is 50+ lines long, and if the user is on a 23 line terminal, it will just be Holy Scroll Batman! . I wonder if a new podman --shorthelp command might be good, with output like:

Error: missing command 'podman COMMAND'
Valid commands are: attach, auto-update, build, commit, container, cp, create, diff, events, exec, export, generate, healthcheck, help, history, image, images, import, info, init, inspect, kill, load, login, logout, logs, manifest, mount, network, pause, play, pod, port, ps, pull, push, rename, restart, rm, rmi, run, save, search, start, stats, stop, system, tag, top, umount, unpause, unshare, untag, version, volume, and wait.  Or try `podman --help` for more details.

But that maybe a bit daunting too....

TomSweeneyRedHat · 2021-10-13T23:49:50Z

@markusthoemmes forgot to add, THX! for bringing this forward.

rhatdan · 2021-10-14T11:07:52Z

We should probably do this on subcommands as well, what I don't like is scroll off the screen when you specify a bad option.

 $ podman network
Error: missing command 'podman network COMMAND'
Try 'podman network --help' for more information.
$ podman container
Error: missing command 'podman container COMMAND'
Try 'podman container --help' for more information.

Git does a nice job at this.

Docker does a better job then us on this also.

Both Git and Docker stack the options at the top and the commands at the bottom, which is probably something we should consider.

markusthoemmes · 2021-10-14T11:18:03Z

@rhatdan AFAIK this does work on subcommands as well 🤔 At least on everyone that's using the respective helper (at least your examples worked)

markusthoemmes · 2021-10-14T11:25:56Z

I'll dig through the tests for a bit then 😅

rhatdan · 2021-10-14T11:28:07Z

If it does that would be good.

markusthoemmes · 2021-10-14T11:42:20Z

If I understand the test I just changed correctly, it should actually check that.

Signed-off-by: Markus Thömmes <[email protected]>

jwhonce

Thanks for the PR. Just one change to restore the original semantics.

jwhonce · 2021-10-15T15:03:38Z

cmd/podman/validate/args.go

@@ -27,7 +27,7 @@ func SubCommandExists(cmd *cobra.Command, args []string) error {
 		}
 		return errors.Errorf("unrecognized command `%[1]s %[2]s`\n\nDid you mean this?\n\t%[3]s\n\nTry '%[1]s --help' for more information.", cmd.CommandPath(), args[0], strings.Join(suggestions, "\n\t"))
 	}
-	return errors.Errorf("missing command '%[1]s COMMAND'\nTry '%[1]s --help' for more information.", cmd.CommandPath())
+	return cmd.Help()


I like the change, except for the semantics have changed where what was reported as an error now returns exit code 0. For backwards compatibility with existing scripts and to aid future script writers. Can we do something like:

Suggested change

return cmd.Help()

cmd.Help()

return errors.Errorf("missing command '%[1]s COMMAND", cmd.CommandPath()

I'm wedded to that text, just the idea of keeping the old semantics

Yes we still need to exit with nonzero.

Good catch @jwhonce

Signed-off-by: Markus Thömmes <[email protected]>

markusthoemmes · 2021-11-05T10:17:36Z

@rhatdan @jwhonce thanks for the feedback and sorry for taking a while to add it. This should be ready for another look now!

rhatdan · 2021-11-05T12:38:02Z

LGTM
/approve
Thanks @markusthoemmes

openshift-ci · 2021-11-05T12:38:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, rhatdan

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

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

rhatdan · 2021-11-05T12:38:14Z

@jwhonce PTAL

rhatdan · 2021-11-06T11:10:10Z

@mheon PTAL

markusthoemmes · 2021-11-08T15:29:31Z

/assign @jwhonce @mheon

mheon · 2021-11-08T17:22:55Z

/lgtm

Display help text on empty subcommand by default

cc875fd

Signed-off-by: Markus Thömmes <[email protected]>

openshift-ci bot assigned rhatdan Oct 13, 2021

Adjust tests to verify all subcommands show the help message

184de39

Signed-off-by: Markus Thömmes <[email protected]>

markusthoemmes force-pushed the help-default branch from f02f349 to 184de39 Compare October 14, 2021 11:55

jwhonce requested changes Oct 15, 2021

View reviewed changes

markusthoemmes force-pushed the help-default branch 4 times, most recently from 159299f to af2278c Compare November 4, 2021 18:21

Keep error semantics intact

756dda2

Signed-off-by: Markus Thömmes <[email protected]>

markusthoemmes force-pushed the help-default branch from af2278c to 756dda2 Compare November 5, 2021 08:34

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2021

openshift-ci bot assigned jwhonce and mheon Nov 8, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2021

openshift-merge-robot merged commit abfec81 into containers:main Nov 8, 2021

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display help text on empty subcommand by default #11953

Display help text on empty subcommand by default #11953

markusthoemmes commented Oct 13, 2021

markusthoemmes commented Oct 13, 2021

rhatdan commented Oct 13, 2021

TomSweeneyRedHat commented Oct 13, 2021

TomSweeneyRedHat commented Oct 13, 2021

rhatdan commented Oct 14, 2021

markusthoemmes commented Oct 14, 2021 •

edited

Loading

markusthoemmes commented Oct 14, 2021

rhatdan commented Oct 14, 2021

markusthoemmes commented Oct 14, 2021

jwhonce left a comment

jwhonce Oct 15, 2021

rhatdan Oct 15, 2021

rhatdan Oct 15, 2021

markusthoemmes commented Nov 5, 2021

rhatdan commented Nov 5, 2021

openshift-ci bot commented Nov 5, 2021

rhatdan commented Nov 5, 2021

rhatdan commented Nov 6, 2021

markusthoemmes commented Nov 8, 2021

mheon commented Nov 8, 2021

	return cmd.Help()
	cmd.Help()
	return errors.Errorf("missing command '%[1]s COMMAND", cmd.CommandPath()

Display help text on empty subcommand by default #11953

Display help text on empty subcommand by default #11953

Conversation

markusthoemmes commented Oct 13, 2021

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

markusthoemmes commented Oct 13, 2021

rhatdan commented Oct 13, 2021

TomSweeneyRedHat commented Oct 13, 2021

TomSweeneyRedHat commented Oct 13, 2021

rhatdan commented Oct 14, 2021

markusthoemmes commented Oct 14, 2021 • edited Loading

markusthoemmes commented Oct 14, 2021

rhatdan commented Oct 14, 2021

markusthoemmes commented Oct 14, 2021

jwhonce left a comment

Choose a reason for hiding this comment

jwhonce Oct 15, 2021

Choose a reason for hiding this comment

rhatdan Oct 15, 2021

Choose a reason for hiding this comment

rhatdan Oct 15, 2021

Choose a reason for hiding this comment

markusthoemmes commented Nov 5, 2021

rhatdan commented Nov 5, 2021

openshift-ci bot commented Nov 5, 2021

rhatdan commented Nov 5, 2021

rhatdan commented Nov 6, 2021

markusthoemmes commented Nov 8, 2021

mheon commented Nov 8, 2021

markusthoemmes commented Oct 14, 2021 •

edited

Loading