-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix help commands to show short and long description. #2564
Conversation
cmd/podman/run.go
Outdated
@@ -39,6 +39,8 @@ var ( | |||
|
|||
func init() { | |||
runCommand.Command = _runCommand | |||
runCommand.SetHelpTemplate(HelpTemplate()) | |||
runCommand.SetHelpTemplate(HelpTemplate()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
cmd/podman/common.go
Outdated
|
||
Description: | ||
|
||
{{.Long}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the rest of the output, would you consider (1) removing the empty line between Description:
and the text, and (2) indenting {{.Long}}
two spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: well, that's harder than I thought: multi-line descriptions (import, info, inspect, ...) would need to be manually indented. I see no clean way to deal with this but hope someone can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think similar fun here as with the examples. They'd have to be manually indented. I think "as is" is probably the best case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove the line for now.
Top biting this one, gonna rekick. |
bot, retest this please. |
One probable small change, otherwise LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; only minor comments.
I see a lot of 'The container name or ID can be used.'; are those really necessary?
grep -L
suggests that healthcheck_run.go
might be another candidate for a Description.
cmd/podman/exec.go
Outdated
podman exec | ||
|
||
Run a command in a running container | ||
execDescription = `Execute the specified command inside of a running container. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for 'of'; just 'inside a container' is best. Also, extra space character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmd/podman/cp.go
Outdated
@@ -27,8 +27,9 @@ import ( | |||
var ( | |||
cpCommand cliconfig.CpValues | |||
|
|||
cpDescription = "Copy files/folders between a container and the local filesystem" | |||
_cpCommand = &cobra.Command{ | |||
cpDescription = `The podman cp utility copies the contents of SRC_PATH to the DEST_PATH. You can copy from the container's file system to the local machine or the reverse, from the local filesystem to the container. If - is specified for either the SRC_PATH or DEST_PATH, you can also stream a tar archive from STDIN or to STDOUT. The CONTAINER can be a running or stopped container. The SRC_PATH or DEST_PATH can be a file or directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention I see for the most part is to document as 'Action blah blah'; It feels jarring to see this one as 'The podman cp utility ...'. How about just 'Copy the contents'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed to `Commant copies ...
cmd/podman/inspect.go
Outdated
_inspectCommand = &cobra.Command{ | ||
inspectDescription = `This displays the low-level information on containers and images identified by name or ID. | ||
|
||
By default, this will render all results in a JSON array. If the container and image have the same name, this command returns the container JSON.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this isn't part of your commit, but why not change that to something more readable such as 'If given a name that matches both a container and an image, this command inspects the container.'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmd/podman/logs.go
Outdated
@@ -15,8 +15,10 @@ import ( | |||
|
|||
var ( | |||
logsCommand cliconfig.LogsValues | |||
logsDescription = "The podman logs command batch-retrieves whatever logs are present for a container at the time of execution. This does not guarantee execution" + | |||
"order when combined with podman run (i.e. your run may not have generated any logs at the time you execute podman logs" | |||
logsDescription = `The podman logs command batch-retrieves whatever logs are present for a container at the time of execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same principle as cp
above: 'Retrieves whatever logs ...'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmd/podman/rm.go
Outdated
Podman rm will remove one or more containers from the host. | ||
The container name or ID can be used. This does not remove images. | ||
Running containers will not be removed without the -f option. | ||
rmDescription = fmt.Sprintf(`Podman rm will remove one or more containers from the host. The container name or ID can be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another unnecessary Podman; just 'Remove one or more...' might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmd/podman/varlink.go
Outdated
|
||
run varlink interface | ||
Tools speaking valink protocol can remotely manage pods, containers and images. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo (valink)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
0a5c7bf
to
034b879
Compare
cmd/podman/exists.go
Outdated
` | ||
containerExistsDescription = ` | ||
podman container exists | ||
containerExistsDescription = `If an container exists in local storage, podman container exists exits with 0, otherwise exit code will be 1.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a container, not an; likewise for pod
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified the comment.
cmd/podman/inspect.go
Outdated
_inspectCommand = &cobra.Command{ | ||
inspectDescription = `This displays the low-level information on containers and images identified by name or ID. | ||
|
||
If given a namee that matches both a container and an image, this command inspects the container. By default, this will render all results in a JSON array.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: namee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmd/podman/pod_unpause.go
Outdated
@@ -12,8 +12,10 @@ import ( | |||
|
|||
var ( | |||
podUnpauseCommand cliconfig.PodUnpauseValues | |||
podUnpauseDescription = `Unpauses one or more pods. The pod name or ID can be used.` | |||
_podUnpauseCommand = &cobra.Command{ | |||
podUnpauseDescription = `The podman unpause command will unpause "paused" all containers assigned to the pod. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was that meant to be will unpause all "paused" containers
? I'm not sure how to parse it otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmd/podman/top.go
Outdated
the latest container. | ||
%s | ||
`, getDescriptorString()) | ||
topDescription = fmt.Sprintf(`Similare to system top command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Similare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmd/podman/volume_prune.go
Outdated
@@ -16,12 +16,9 @@ import ( | |||
|
|||
var ( | |||
volumePruneCommand cliconfig.VolumePruneValues | |||
volumePruneDescription = ` | |||
podman volume prune | |||
volumePruneDescription = `Command prompts for confirmation if not using the --force flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer says what this command does ("Remove all unused volumes"). Was that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Cleanup lots of help information to look good when displayed. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
[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 |
/lgtm Thank you for your patience with my nits |
Cleanup lots of help information to look good when displayed.
Signed-off-by: Daniel J Walsh dwalsh@redhat.com
Examples added by @TomSweeneyRedHat
Podman build help after:
Podman diff help before:
Podman diff help after: