Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More cleanup for failures on missing commands. #2580

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Mar 7, 2019

Currently in podman if a user specifies a command that does not exist
the tool shows the help information. This patch changes it to show
information like:

$ ./bin/podman foobar
Error: unrecognized command 'podman foobar'
Try 'podman --help' for more information.
$ ./bin/podman volume foobar
Error: unrecognized command podman volume foobar
Try 'podman volume --help' for more information.
$ ./bin/podman container foobar
Error: unrecognized command podman container foobar
Try 'podman container --help' for more information.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M labels Mar 7, 2019
@rhatdan
Copy link
Member Author

rhatdan commented Mar 7, 2019

@rhatdan
Copy link
Member Author

rhatdan commented Mar 7, 2019

Closes:#2530

@edsantiago
Copy link
Collaborator

Okay... I have no idea why, but am throwing this out in case it's obvious to anyone: this change slightly breaks the Usage format:

before:
    $ podman system --help
    Usage:
        podman system [command]
after:
    Usage:
      podman system [flags]
      podman system [command]

This breaks all the scripts I've written that rely on '[command]' to indicate 'this is a podman command with further subcommands'. I can find a workaround, I'm sure, but first: does anyone understand why this is happening, and is there an easy fix for it? (my guess is that if {{.Runnable}} in common.go is now triggering, but what tdo I know).

Oh, and I think you missed pod.go. (Found by my recursion scripts)

@edsantiago
Copy link
Collaborator

Update: this edit to common.go resolves the double-usage-line issue without, I think, introducing any unexpected behavior (but I'm still checking):

--- a/cmd/podman/common.go
+++ b/cmd/podman/common.go
@@ -525,7 +525,7 @@ func scrubServer(server string) string {
 // This blocks the desplaying of the global options. The main podman
 // command should not use this.
 func UsageTemplate() string {
-       return `Usage:{{if .Runnable}}
+       return `Usage:{{if (and .Runnable (not .HasAvailableSubCommands))}}
   {{.UseLine}}{{end}}{{if .HasAvailableSubCommands}}
   {{.CommandPath}} [command]{{end}}{{if gt (len .Aliases) 0}}
 

@edsantiago
Copy link
Collaborator

This is really exciting. Thank you for finding a way to address this.

I hope you'll accept my refactored PR (and the warning comment therein)

@edsantiago
Copy link
Collaborator

@rhatdan I need to head out soon but just want to make sure you saw my refactoring PR? There's a lot of unnecessary duplication here, and also a serious index dereferencing bug (which can be fixed in only one place, if you agree with my PR approach)

@rhatdan
Copy link
Member Author

rhatdan commented Mar 7, 2019

Sure lets finish this up tomorrow, Hopefully your idea works.

@rhatdan
Copy link
Member Author

rhatdan commented Mar 8, 2019

/retest
bot, retest this please

cmd/podman/common.go Outdated Show resolved Hide resolved
Currently in podman if a user specifies a command that does not exist
the tool shows the help information.  This patch changes it to show
information like:

$ ./bin/podman foobar
Error: unrecognized command 'podman foobar'
Try 'podman --help' for more information.
$ ./bin/podman volume foobar
Error: unrecognized command `podman volume foobar`
Try 'podman volume --help' for more information.
$ ./bin/podman container foobar
Error: unrecognized command `podman container foobar`
Try 'podman container --help' for more information.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, 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:

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

@edsantiago
Copy link
Collaborator

/lgtm

I'd still encourage you to consider a more descriptive name than commandRunE, but otherwise this is really great and very much appreciated. Thank you.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2019
@openshift-merge-robot openshift-merge-robot merged commit 9e2cd7f into containers:master Mar 8, 2019
@github-actions 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 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants