From 6864a5547a774d19a7ccb9d50a7799b721fb66ef Mon Sep 17 00:00:00 2001 From: Ed Santiago Date: Tue, 23 Jun 2020 08:10:08 -0600 Subject: BATS tests: new too-many-arguments test ...plus a few others. And fixes to actual parsing. If a command's usage message includes '...' in the argument list, assume it can take unlimited arguments. Nothing we can check. For all others, though, the ALL-CAPS part on the right-hand side of the usage message will define an upper bound on the number of arguments accepted by the command. So in our 'podman --help' test, generate N+1 args and run that command. We expect a 125 exit status and a suitably helpful error message. Not all podman commands or subcommands were checking, so I fixed that. And, fixed some broken usage messages (all-caps FLAGS, and '[flags]' at the end of 'ARGS'). Add new checks to the help test to prevent those in the future. Plus a little refactoring/cleanup where necessary. Signed-off-by: Ed Santiago --- cmd/podman/containers/commit.go | 6 ++-- cmd/podman/containers/export.go | 2 +- cmd/podman/containers/inspect.go | 2 +- cmd/podman/containers/mount.go | 2 +- cmd/podman/containers/port.go | 3 ++ cmd/podman/containers/top.go | 2 +- cmd/podman/generate/systemd.go | 2 +- cmd/podman/images/build.go | 1 + cmd/podman/images/inspect.go | 2 +- cmd/podman/images/list.go | 2 +- cmd/podman/images/push.go | 19 ++++-------- cmd/podman/images/save.go | 4 +-- cmd/podman/images/trust_show.go | 1 + cmd/podman/inspect.go | 2 +- cmd/podman/manifest/create.go | 2 +- cmd/podman/networks/create.go | 12 +++----- cmd/podman/networks/inspect.go | 2 +- cmd/podman/pods/top.go | 2 +- cmd/podman/system/connection.go | 2 +- cmd/podman/system/unshare.go | 2 +- cmd/podman/validate/args.go | 7 +++-- test/system/015-help.bats | 63 +++++++++++++++++++++++++++++++++------- test/system/120-load.bats | 6 ---- 23 files changed, 90 insertions(+), 58 deletions(-) diff --git a/cmd/podman/containers/commit.go b/cmd/podman/containers/commit.go index b3c3d7626..79e2a32a7 100644 --- a/cmd/podman/containers/commit.go +++ b/cmd/podman/containers/commit.go @@ -22,7 +22,7 @@ var ( Short: "Create new image based on the changed container", Long: commitDescription, RunE: commit, - Args: cobra.MinimumNArgs(1), + Args: cobra.RangeArgs(1, 2), Example: `podman commit -q --message "committing container to image" reverent_golick image-committed podman commit -q --author "firstName lastName" reverent_golick image-committed podman commit -q --pause=false containerID image-committed @@ -30,7 +30,7 @@ var ( } containerCommitCommand = &cobra.Command{ - Args: cobra.MinimumNArgs(1), + Args: commitCommand.Args, Use: commitCommand.Use, Short: commitCommand.Short, Long: commitCommand.Long, @@ -82,7 +82,7 @@ func init() { func commit(cmd *cobra.Command, args []string) error { container := args[0] - if len(args) > 1 { + if len(args) == 2 { commitOptions.ImageName = args[1] } if !commitOptions.Quiet { diff --git a/cmd/podman/containers/export.go b/cmd/podman/containers/export.go index bbb6a6bc9..66e768ccb 100644 --- a/cmd/podman/containers/export.go +++ b/cmd/podman/containers/export.go @@ -28,7 +28,7 @@ var ( } containerExportCommand = &cobra.Command{ - Args: cobra.MinimumNArgs(1), + Args: cobra.ExactArgs(1), Use: exportCommand.Use, Short: exportCommand.Short, Long: exportCommand.Long, diff --git a/cmd/podman/containers/inspect.go b/cmd/podman/containers/inspect.go index 8556ebe83..e49fcc2e0 100644 --- a/cmd/podman/containers/inspect.go +++ b/cmd/podman/containers/inspect.go @@ -10,7 +10,7 @@ import ( var ( // podman container _inspect_ inspectCmd = &cobra.Command{ - Use: "inspect [flags] CONTAINER", + Use: "inspect [flags] CONTAINER [CONTAINER...]", Short: "Display the configuration of a container", Long: `Displays the low-level information on a container identified by name or ID.`, RunE: inspectExec, diff --git a/cmd/podman/containers/mount.go b/cmd/podman/containers/mount.go index 7f15616de..5c73cceaa 100644 --- a/cmd/podman/containers/mount.go +++ b/cmd/podman/containers/mount.go @@ -23,7 +23,7 @@ var ( ` mountCommand = &cobra.Command{ - Use: "mount [flags] [CONTAINER]", + Use: "mount [flags] [CONTAINER...]", Short: "Mount a working container's root filesystem", Long: mountDescription, RunE: mount, diff --git a/cmd/podman/containers/port.go b/cmd/podman/containers/port.go index d058a6aaf..115adc2a7 100644 --- a/cmd/podman/containers/port.go +++ b/cmd/podman/containers/port.go @@ -89,6 +89,9 @@ func port(cmd *cobra.Command, args []string) error { container = args[0] } port := "" + if len(args) > 2 { + return errors.Errorf("`port` accepts at most 2 arguments") + } if len(args) > 1 && !portOpts.Latest { port = args[1] } diff --git a/cmd/podman/containers/top.go b/cmd/podman/containers/top.go index d2b11ec77..afab12a14 100644 --- a/cmd/podman/containers/top.go +++ b/cmd/podman/containers/top.go @@ -25,7 +25,7 @@ var ( topOptions = entities.TopOptions{} topCommand = &cobra.Command{ - Use: "top [flags] CONTAINER [FORMAT-DESCRIPTORS|ARGS]", + Use: "top [flags] CONTAINER [FORMAT-DESCRIPTORS|ARGS...]", Short: "Display the running processes of a container", Long: topDescription, RunE: top, diff --git a/cmd/podman/generate/systemd.go b/cmd/podman/generate/systemd.go index e4fdd8690..b4ab0f9df 100644 --- a/cmd/podman/generate/systemd.go +++ b/cmd/podman/generate/systemd.go @@ -20,7 +20,7 @@ var ( Short: "Generate systemd units.", Long: systemdDescription, RunE: systemd, - Args: cobra.MinimumNArgs(1), + Args: cobra.ExactArgs(1), Example: `podman generate systemd CTR podman generate systemd --new --time 10 CTR podman generate systemd --files --name POD`, diff --git a/cmd/podman/images/build.go b/cmd/podman/images/build.go index 23bfcab79..dfde896a1 100644 --- a/cmd/podman/images/build.go +++ b/cmd/podman/images/build.go @@ -45,6 +45,7 @@ var ( Long: buildDescription, TraverseChildren: true, RunE: build, + Args: cobra.MaximumNArgs(1), Example: `podman build . podman build --creds=username:password -t imageName -f Containerfile.simple . podman build --layers --force-rm --tag imageName .`, diff --git a/cmd/podman/images/inspect.go b/cmd/podman/images/inspect.go index f6a10ba44..a1a9e91eb 100644 --- a/cmd/podman/images/inspect.go +++ b/cmd/podman/images/inspect.go @@ -10,7 +10,7 @@ import ( var ( // Command: podman image _inspect_ inspectCmd = &cobra.Command{ - Use: "inspect [flags] IMAGE", + Use: "inspect [flags] IMAGE [IMAGE...]", Short: "Display the configuration of an image", Long: `Displays the low-level information of an image identified by name or ID.`, RunE: inspectExec, diff --git a/cmd/podman/images/list.go b/cmd/podman/images/list.go index b7a8b8911..de7cca40d 100644 --- a/cmd/podman/images/list.go +++ b/cmd/podman/images/list.go @@ -32,7 +32,7 @@ type listFlagType struct { var ( // Command: podman image _list_ listCmd = &cobra.Command{ - Use: "list [FLAGS] [IMAGE]", + Use: "list [flags] [IMAGE]", Aliases: []string{"ls"}, Args: cobra.MaximumNArgs(1), Short: "List images in local storage", diff --git a/cmd/podman/images/push.go b/cmd/podman/images/push.go index a1614dc7a..7af2d9343 100644 --- a/cmd/podman/images/push.go +++ b/cmd/podman/images/push.go @@ -29,10 +29,11 @@ var ( // Command: podman push pushCmd = &cobra.Command{ - Use: "push [flags] SOURCE DESTINATION", + Use: "push [flags] SOURCE [DESTINATION]", Short: "Push an image to a specified destination", Long: pushDescription, RunE: imagePush, + Args: cobra.RangeArgs(1, 2), Example: `podman push imageID docker://registry.example.com/repository:tag podman push imageID oci-archive:/path/to/layout:image:tag`, } @@ -45,6 +46,7 @@ var ( Short: pushCmd.Short, Long: pushCmd.Long, RunE: pushCmd.RunE, + Args: pushCmd.Args, Example: `podman image push imageID docker://registry.example.com/repository:tag podman image push imageID oci-archive:/path/to/layout:image:tag`, } @@ -96,19 +98,8 @@ func pushFlags(flags *pflag.FlagSet) { // imagePush is implement the command for pushing images. func imagePush(cmd *cobra.Command, args []string) error { - var source, destination string - switch len(args) { - case 1: - source = args[0] - destination = args[0] - case 2: - source = args[0] - destination = args[1] - case 0: - fallthrough - default: - return errors.New("push requires at least one image name, or optionally a second to specify a different destination") - } + source := args[0] + destination := args[len(args)-1] // TLS verification in c/image is controlled via a `types.OptionalBool` // which allows for distinguishing among set-true, set-false, unspecified diff --git a/cmd/podman/images/save.go b/cmd/podman/images/save.go index 56953e41c..9b03c1383 100644 --- a/cmd/podman/images/save.go +++ b/cmd/podman/images/save.go @@ -23,8 +23,8 @@ var ( saveDescription = `Save an image to docker-archive or oci-archive on the local machine. Default is docker-archive.` saveCommand = &cobra.Command{ - Use: "save [flags] IMAGE", - Short: "Save image to an archive", + Use: "save [flags] IMAGE [IMAGE...]", + Short: "Save image(s) to an archive", Long: saveDescription, RunE: save, Args: func(cmd *cobra.Command, args []string) error { diff --git a/cmd/podman/images/trust_show.go b/cmd/podman/images/trust_show.go index 23ee6c709..dbaa800a4 100644 --- a/cmd/podman/images/trust_show.go +++ b/cmd/podman/images/trust_show.go @@ -18,6 +18,7 @@ var ( Short: "Display trust policy for the system", Long: showTrustDescription, RunE: showTrust, + Args: cobra.MaximumNArgs(1), Example: "", } ) diff --git a/cmd/podman/inspect.go b/cmd/podman/inspect.go index a5fdaedc2..6c4607d88 100644 --- a/cmd/podman/inspect.go +++ b/cmd/podman/inspect.go @@ -10,7 +10,7 @@ import ( var ( // Command: podman _inspect_ Object_ID inspectCmd = &cobra.Command{ - Use: "inspect [flags] {CONTAINER_ID | IMAGE_ID}", + Use: "inspect [flags] {CONTAINER_ID | IMAGE_ID} [...]", Short: "Display the configuration of object denoted by ID", Long: "Displays the low-level information on an object identified by name or ID", TraverseChildren: true, diff --git a/cmd/podman/manifest/create.go b/cmd/podman/manifest/create.go index 9c0097b90..2ab1fccea 100644 --- a/cmd/podman/manifest/create.go +++ b/cmd/podman/manifest/create.go @@ -20,7 +20,7 @@ var ( Example: `podman manifest create mylist:v1.11 podman manifest create mylist:v1.11 arch-specific-image-to-add podman manifest create --all mylist:v1.11 transport:tagged-image-to-add`, - Args: cobra.MinimumNArgs(1), + Args: cobra.RangeArgs(1, 2), } ) diff --git a/cmd/podman/networks/create.go b/cmd/podman/networks/create.go index 5d28c7140..2d29beddd 100644 --- a/cmd/podman/networks/create.go +++ b/cmd/podman/networks/create.go @@ -8,7 +8,6 @@ import ( "github.com/containers/libpod/libpod/define" "github.com/containers/libpod/pkg/domain/entities" "github.com/containers/libpod/pkg/network" - "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/spf13/pflag" ) @@ -20,6 +19,7 @@ var ( Short: "network create", Long: networkCreateDescription, RunE: networkCreate, + Args: cobra.MaximumNArgs(1), Example: `podman network create podman1`, Annotations: map[string]string{ registry.ParentNSRequired: "", @@ -62,14 +62,10 @@ func networkCreate(cmd *cobra.Command, args []string) error { if err := network.IsSupportedDriver(networkCreateOptions.Driver); err != nil { return err } - if len(args) > 1 { - return errors.Errorf("only one network can be created at a time") - } - if len(args) > 0 && !define.NameRegex.MatchString(args[0]) { - return define.RegexError - } - if len(args) > 0 { + if !define.NameRegex.MatchString(args[0]) { + return define.RegexError + } name = args[0] } response, err := registry.ContainerEngine().NetworkCreate(registry.Context(), name, networkCreateOptions) diff --git a/cmd/podman/networks/inspect.go b/cmd/podman/networks/inspect.go index 1b2e89909..31269e836 100644 --- a/cmd/podman/networks/inspect.go +++ b/cmd/podman/networks/inspect.go @@ -16,7 +16,7 @@ import ( var ( networkinspectDescription = `Inspect network` networkinspectCommand = &cobra.Command{ - Use: "inspect NETWORK [NETWORK...] [flags] ", + Use: "inspect [flags] NETWORK [NETWORK...]", Short: "network inspect", Long: networkinspectDescription, RunE: networkInspect, diff --git a/cmd/podman/pods/top.go b/cmd/podman/pods/top.go index ba1efb638..8df00f92a 100644 --- a/cmd/podman/pods/top.go +++ b/cmd/podman/pods/top.go @@ -22,7 +22,7 @@ var ( topOptions = entities.PodTopOptions{} topCommand = &cobra.Command{ - Use: "top [flags] POD [FORMAT-DESCRIPTORS|ARGS]", + Use: "top [flags] POD [FORMAT-DESCRIPTORS|ARGS...]", Short: "Display the running processes of containers in a pod", Long: topDescription, RunE: top, diff --git a/cmd/podman/system/connection.go b/cmd/podman/system/connection.go index d8c709d6e..f19af2ccf 100644 --- a/cmd/podman/system/connection.go +++ b/cmd/podman/system/connection.go @@ -29,7 +29,7 @@ var ( return nil } connectionCmd = &cobra.Command{ - Use: "connection [flags] destination", + Use: "connection [flags] DESTINATION", Args: cobra.ExactArgs(1), Long: `Store ssh destination information in podman configuration. "destination" is of the form [user@]hostname or diff --git a/cmd/podman/system/unshare.go b/cmd/podman/system/unshare.go index 7db5d36d2..e5d41e06d 100644 --- a/cmd/podman/system/unshare.go +++ b/cmd/podman/system/unshare.go @@ -13,7 +13,7 @@ import ( var ( unshareDescription = "Runs a command in a modified user namespace." unshareCommand = &cobra.Command{ - Use: "unshare [flags] [COMMAND [ARG]]", + Use: "unshare [flags] [COMMAND [ARG ...]]", Short: "Run a command in a modified user namespace", Long: unshareDescription, RunE: unshare, diff --git a/cmd/podman/validate/args.go b/cmd/podman/validate/args.go index 69240798f..d170447ee 100644 --- a/cmd/podman/validate/args.go +++ b/cmd/podman/validate/args.go @@ -25,8 +25,11 @@ func SubCommandExists(cmd *cobra.Command, args []string) error { // IDOrLatestArgs used to validate a nameOrId was provided or the "--latest" flag func IDOrLatestArgs(cmd *cobra.Command, args []string) error { - if len(args) > 1 || (len(args) == 0 && !cmd.Flag("latest").Changed) { - return fmt.Errorf("`%s` requires a name, id or the \"--latest\" flag", cmd.CommandPath()) + if len(args) > 1 { + return fmt.Errorf("`%s` accepts at most one argument", cmd.CommandPath()) + } + if len(args) == 0 && !cmd.Flag("latest").Changed { + return fmt.Errorf("`%s` requires a name, id, or the \"--latest\" flag", cmd.CommandPath()) } return nil } diff --git a/test/system/015-help.bats b/test/system/015-help.bats index 42d3bd3ec..14af8e1a4 100644 --- a/test/system/015-help.bats +++ b/test/system/015-help.bats @@ -25,7 +25,7 @@ function podman_commands() { function check_help() { local count=0 - local subcommands_found=0 + local -A found for cmd in $(podman_commands "$@"); do # Human-readable podman command string, with multiple spaces collapsed @@ -44,24 +44,37 @@ function check_help() { # If usage ends in '[command]', recurse into subcommands if expr "$usage" : '.*\[command\]$' >/dev/null; then - subcommands_found=$(expr $subcommands_found + 1) + found[subcommands]=1 check_help "$@" $cmd continue fi - # If usage ends in '[flag]', command takes no more arguments. - # Confirm that by running with 'invalid-arg' and expecting failure. - if expr "$usage" : '.*\[flags\]$' >/dev/null; then + # We had someone write upper-case '[FLAGS]' once. Prevent it. + if expr "$usage" : '.*\[FLAG' >/dev/null; then + die "'flags' string must be lower-case in usage: $usage" + fi + + # We had someone do 'podman foo ARG [flags]' one time. Yeah, no. + if expr "$usage" : '.*[A-Z].*\[flag' >/dev/null; then + die "'flags' must precede arguments in usage: $usage" + fi + + # If usage lists no arguments (strings in ALL CAPS), confirm + # by running with 'invalid-arg' and expecting failure. + if ! expr "$usage" : '.*[A-Z]' >/dev/null; then if [ "$cmd" != "help" ]; then dprint "$command_string invalid-arg" run_podman 125 "$@" $cmd invalid-arg is "$output" "Error: .* takes no arguments" \ "'$command_string' with extra (invalid) arguments" fi + found[takes_no_args]=1 fi - # If usage has required arguments, try running without them - if expr "$usage" : '.*\[flags\] [A-Z]' >/dev/null; then + # If usage has required arguments, try running without them. + # The expression here is 'first capital letter is not in [BRACKETS]'. + # It is intended to handle 'podman foo [flags] ARG' but not ' [ARG]'. + if expr "$usage" : '[^A-Z]\+ [A-Z]' >/dev/null; then # Exceptions: these commands don't work rootless if is_rootless; then # "pause is not supported for rootless containers" @@ -80,6 +93,31 @@ function check_help() { run_podman 125 "$@" $cmd $n_args arguments" + + found[fixed_args]=1 + fi fi count=$(expr $count + 1) @@ -101,9 +139,14 @@ function check_help() { [ $count -gt 0 ] || \ die "Internal error: no commands found in 'podman help $@' list" - # At least the top level must have some subcommands - if [ -z "$*" -a $subcommands_found -eq 0 ]; then - die "Internal error: did not find any podman subcommands" + # Sanity check: make sure the special loops above triggered at least once. + # (We've had situations where a typo makes the conditional never run) + if [ -z "$*" ]; then + for i in subcommands required_args takes_no_args fixed_args; do + if [[ -z ${found[$i]} ]]; then + die "Internal error: '$i' subtest did not trigger" + fi + done fi } diff --git a/test/system/120-load.bats b/test/system/120-load.bats index f290c1888..afa5ab473 100644 --- a/test/system/120-load.bats +++ b/test/system/120-load.bats @@ -107,10 +107,4 @@ verify_iid_and_name() { "Diagnostic from 'podman load' without redirection or -i" } -@test "podman load - at most 1 arg(s)" { - run_podman 125 load 1 2 3 - is "$output" \ - "Error: accepts at most 1 arg(s), received 3" -} - # vim: filetype=sh -- cgit v1.2.3-54-g00ecf