Merge pull request #20945 from Luap99/string-array

cli: use StringArray over StringSlice Part 1

cmd/podman/README.md

@@ -109,3 +109,11 @@ The complete set can be found in the `validate` package, here are some examples:
     created := validate.ChoiceValue(&opts.Sort, "command", "created", "id", "image", "names", "runningfor", "size", "status")
     flags.Var(created, "sort", "Sort output by: "+created.Choices())
     ```
+
+## Adding CLI flags
+
+When adding adding a new cli option that accepts a string array, there are two options to choose from: `StringSlice()` and `StringArray()`.
+They differ slightly in their behavior: `StringSlice()` allows the values to be comma separated so `--opt v1,v2 --opt v3` results in
+`[]string{"v1", "v2", "v3"}`, while `StringArray()`  would result in `[]string{"v1,v2", "v3"}`. Thus it is impossible to use values with comma in `StringSlice()`, which makes it unsuitable for flags that accept arbitrary values such as file paths as example. Also, because `StringSlice()` uses the csv lib to parse the values, it has special escaping rules for things like quotes, see https://github.com/containers/podman/issues/20064 for an example of how complicated things can get because of this.
+Thus use `StringSlice()` only when the option accepts predefined values that do not contain special characters, for example `--cap-add` and `--cap-drop` are a good example for this. Using `--cap-add NET_ADMIN,NET_RAW` is equal to `--cap-add NET_ADMIN --cap-add NET_RAW` so it is better suited to save some typing for users.
+When in doubt always choose `StringArray()` over `StringSlice()`.

cmd/podman/common/create.go

@@ -33,7 +33,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 
 	if mode == entities.CreateMode { // regular create flags
 		annotationFlagName := "annotation"
-		createFlags.StringSliceVar(
+		createFlags.StringArrayVar(
 			&cf.Annotation,
 			annotationFlagName, []string{},
 			"Add annotations to container (key=value)",
@@ -147,7 +147,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 		}
 
 		envFileFlagName := "env-file"
-		createFlags.StringSliceVar(
+		createFlags.StringArrayVar(
 			&cf.EnvFile,
 			envFileFlagName, []string{},
 			"Read in a file of environment variables",
@@ -282,7 +282,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 		_ = cmd.RegisterFlagCompletionFunc(logDriverFlagName, AutocompleteLogDriver)
 
 		logOptFlagName := "log-opt"
-		createFlags.StringSliceVar(
+		createFlags.StringArrayVar(
 			&cf.LogOptions,
 			logOptFlagName, []string{},
 			"Logging driver options",
@@ -595,7 +595,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 		_ = cmd.RegisterFlagCompletionFunc(pidFileFlagName, completion.AutocompleteDefault)
 
 		chrootDirsFlagName := "chrootdirs"
-		createFlags.StringSliceVar(
+		createFlags.StringArrayVar(
 			&cf.ChrootDirs,
 			chrootDirsFlagName, []string{},
 			"Chroot directories inside the container",
@@ -611,7 +611,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 		_ = cmd.RegisterFlagCompletionFunc(groupEntryName, completion.AutocompleteNone)
 
 		decryptionKeysFlagName := "decryption-key"
-		createFlags.StringSliceVar(
+		createFlags.StringArrayVar(
 			&cf.DecryptionKeys,
 			decryptionKeysFlagName, []string{},
 			"Key needed to decrypt the image (e.g. /path/to/key.pem)",
@@ -772,7 +772,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 		_ = cmd.RegisterFlagCompletionFunc(labelFlagName, completion.AutocompleteNone)
 
 		labelFileFlagName := "label-file"
-		createFlags.StringSliceVar(
+		createFlags.StringArrayVar(
 			&cf.LabelFile,
 			labelFileFlagName, []string{},
 			"Read in a line delimited file of labels",
@@ -814,7 +814,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 		_ = cmd.RegisterFlagCompletionFunc(volumeFlagName, AutocompleteVolumeFlag)
 
 		deviceFlagName := "device"
-		createFlags.StringSliceVar(
+		createFlags.StringArrayVar(
 			&cf.Devices,
 			deviceFlagName, devices(),
 			"Add a host device to the container",
@@ -898,15 +898,15 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 	}
 	if mode == entities.CreateMode || mode == entities.UpdateMode {
 		deviceReadIopsFlagName := "device-read-iops"
-		createFlags.StringSliceVar(
+		createFlags.StringArrayVar(
 			&cf.DeviceReadIOPs,
 			deviceReadIopsFlagName, []string{},
 			"Limit read rate (IO per second) from a device (e.g. --device-read-iops=/dev/sda:1000)",
 		)
 		_ = cmd.RegisterFlagCompletionFunc(deviceReadIopsFlagName, completion.AutocompleteDefault)
 
 		deviceWriteIopsFlagName := "device-write-iops"
-		createFlags.StringSliceVar(
+		createFlags.StringArrayVar(
 			&cf.DeviceWriteIOPs,
 			deviceWriteIopsFlagName, []string{},
 			"Limit write rate (IO per second) to a device (e.g. --device-write-iops=/dev/sda:1000)",
@@ -970,15 +970,15 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 	_ = cmd.RegisterFlagCompletionFunc(memorySwapFlagName, completion.AutocompleteNone)
 
 	deviceReadBpsFlagName := "device-read-bps"
-	createFlags.StringSliceVar(
+	createFlags.StringArrayVar(
 		&cf.DeviceReadBPs,
 		deviceReadBpsFlagName, []string{},
 		"Limit read rate (bytes per second) from a device (e.g. --device-read-bps=/dev/sda:1mb)",
 	)
 	_ = cmd.RegisterFlagCompletionFunc(deviceReadBpsFlagName, completion.AutocompleteDefault)
 
 	deviceWriteBpsFlagName := "device-write-bps"
-	createFlags.StringSliceVar(
+	createFlags.StringArrayVar(
 		&cf.DeviceWriteBPs,
 		deviceWriteBpsFlagName, []string{},
 		"Limit write rate (bytes per second) to a device (e.g. --device-write-bps=/dev/sda:1mb)",
@@ -994,7 +994,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 	_ = cmd.RegisterFlagCompletionFunc(blkioWeightFlagName, completion.AutocompleteNone)
 
 	blkioWeightDeviceFlagName := "blkio-weight-device"
-	createFlags.StringSliceVar(
+	createFlags.StringArrayVar(
 		&cf.BlkIOWeightDevice,
 		blkioWeightDeviceFlagName, []string{},
 		"Block IO weight (relative device weight, format: `DEVICE_NAME:WEIGHT`)",

cmd/podman/containers/exec.go

@@ -68,7 +68,7 @@ func execFlags(cmd *cobra.Command) {
 	_ = cmd.RegisterFlagCompletionFunc(envFlagName, completion.AutocompleteNone)
 
 	envFileFlagName := "env-file"
-	flags.StringSliceVar(&envFile, envFileFlagName, []string{}, "Read in a file of environment variables")
+	flags.StringArrayVar(&envFile, envFileFlagName, []string{}, "Read in a file of environment variables")
 	_ = cmd.RegisterFlagCompletionFunc(envFileFlagName, completion.AutocompleteDefault)
 
 	flags.BoolVarP(&execOpts.Interactive, "interactive", "i", false, "Keep STDIN open even if not attached")

cmd/podman/registry/config.go

@@ -72,7 +72,7 @@ func containersConfModules() ([]string, error) {
 	fs.ParseErrorsWhitelist.UnknownFlags = true
 	fs.Usage = func() {}
 	fs.SetInterspersed(false)
-	fs.StringSliceVar(&modules, "module", nil, "")
+	fs.StringArrayVar(&modules, "module", nil, "")
 	fs.BoolP("help", "h", false, "") // Need a fake help flag to avoid the `pflag: help requested` error
 	return modules, fs.Parse(os.Args[index:])
 }

cmd/podman/root.go

@@ -507,7 +507,7 @@ func rootFlags(cmd *cobra.Command, podmanConfig *entities.PodmanConfig) {
 		// as a flag here to a) make sure that rootflags are aware of
 		// this flag and b) to have shell completions.
 		moduleFlagName := "module"
-		lFlags.StringSlice(moduleFlagName, nil, "Load the containers.conf(5) module")
+		lFlags.StringArray(moduleFlagName, nil, "Load the containers.conf(5) module")
 		_ = cmd.RegisterFlagCompletionFunc(moduleFlagName, common.AutocompleteContainersConfModules)
 
 		// A *hidden* flag to change the database backend.
@@ -541,7 +541,7 @@ func rootFlags(cmd *cobra.Command, podmanConfig *entities.PodmanConfig) {
 		_ = cmd.RegisterFlagCompletionFunc(eventsBackendFlagName, common.AutocompleteEventBackend)
 
 		hooksDirFlagName := "hooks-dir"
-		pFlags.StringSliceVar(&podmanConfig.HooksDir, hooksDirFlagName, podmanConfig.ContainersConfDefaultsRO.Engine.HooksDir.Get(), "Set the OCI hooks directory path (may be set multiple times)")
+		pFlags.StringArrayVar(&podmanConfig.HooksDir, hooksDirFlagName, podmanConfig.ContainersConfDefaultsRO.Engine.HooksDir.Get(), "Set the OCI hooks directory path (may be set multiple times)")
 		_ = cmd.RegisterFlagCompletionFunc(hooksDirFlagName, completion.AutocompleteDefault)
 
 		pFlags.IntVar(&podmanConfig.MaxWorks, "max-workers", (runtime.NumCPU()*3)+1, "The maximum number of workers for parallel operations")

test/e2e/create_test.go

@@ -101,15 +101,15 @@ var _ = Describe("Podman create", func() {
 	})
 
 	It("podman create adds annotation", func() {
-		session := podmanTest.Podman([]string{"create", "--annotation", "HELLO=WORLD", "--name", "annotate_test", ALPINE, "ls"})
+		session := podmanTest.Podman([]string{"create", "--annotation", "HELLO=WORLD,WithComma", "--name", "annotate_test", ALPINE, "ls"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
 		Expect(podmanTest.NumberOfContainers()).To(Equal(1))
 
 		check := podmanTest.Podman([]string{"inspect", "annotate_test"})
 		check.WaitWithDefaultTimeout()
 		data := check.InspectContainerToJSON()
-		Expect(data[0].Config.Annotations).To(HaveKeyWithValue("HELLO", "WORLD"))
+		Expect(data[0].Config.Annotations).To(HaveKeyWithValue("HELLO", "WORLD,WithComma"))
 	})
 
 	It("podman create --entrypoint command", func() {
@@ -717,7 +717,7 @@ var _ = Describe("Podman create", func() {
 	})
 
 	It("podman create --chrootdirs functionality test", func() {
-		session := podmanTest.Podman([]string{"create", "-t", "--chrootdirs", "/var/local/qwerty", ALPINE, "/bin/cat"})
+		session := podmanTest.Podman([]string{"create", "-t", "--chrootdirs", "/var/local/qwerty,withcomma", ALPINE, "/bin/cat"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
 		ctrID := session.OutputToString()
@@ -726,7 +726,7 @@ var _ = Describe("Podman create", func() {
 		setup.WaitWithDefaultTimeout()
 		Expect(setup).Should(ExitCleanly())
 
-		setup = podmanTest.Podman([]string{"exec", ctrID, "cmp", "/etc/resolv.conf", "/var/local/qwerty/etc/resolv.conf"})
+		setup = podmanTest.Podman([]string{"exec", ctrID, "cmp", "/etc/resolv.conf", "/var/local/qwerty,withcomma/etc/resolv.conf"})
 		setup.WaitWithDefaultTimeout()
 		Expect(setup).Should(ExitCleanly())
 	})

test/e2e/logs_test.go

@@ -536,7 +536,7 @@ var _ = Describe("Podman logs", func() {
 
 	It("using journald for container with container tag", func() {
 		SkipIfJournaldUnavailable()
-		logc := podmanTest.Podman([]string{"run", "--log-driver", "journald", "--log-opt=tag={{.ImageName}}", "-d", ALPINE, "sh", "-c", "echo podman; sleep 0.1; echo podman; sleep 0.1; echo podman"})
+		logc := podmanTest.Podman([]string{"run", "--log-driver", "journald", "--log-opt=tag={{.ImageName}},withcomma", "-d", ALPINE, "sh", "-c", "echo podman; sleep 0.1; echo podman; sleep 0.1; echo podman"})
 		logc.WaitWithDefaultTimeout()
 		Expect(logc).To(ExitCleanly())
 		cid := logc.OutputToString()
@@ -549,7 +549,7 @@ var _ = Describe("Podman logs", func() {
 			cmd := exec.Command("journalctl", "--no-pager", "-o", "json", "--output-fields=CONTAINER_TAG", fmt.Sprintf("CONTAINER_ID_FULL=%s", cid))
 			out, err := cmd.CombinedOutput()
 			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(string(out)).To(ContainSubstring("alpine"))
+			g.Expect(string(out)).To(ContainSubstring(ALPINE + ",withcomma"))
 		}).Should(Succeed())
 	})
 

test/e2e/run_test.go

@@ -916,7 +916,7 @@ USER bin`, BB)
 
 	It("podman test hooks", func() {
 		SkipIfRemote("--hooks-dir does not work with remote")
-		hooksDir := tempdir + "/hooks"
+		hooksDir := tempdir + "/hooks,withcomma"
 		err := os.Mkdir(hooksDir, 0755)
 		Expect(err).ToNot(HaveOccurred())
 		hookJSONPath := filepath.Join(hooksDir, "checkhooks.json")

test/system/300-cli-parsing.bats

@@ -100,7 +100,7 @@ function _check_env {
 }
 
 
-@test "podman run --env-file" {
+@test "podman run/exec --env-file" {
     declare -A expect=(
         [simple]="abc"
         [special]="bcd#e!f|g hij=lmnop"
@@ -116,7 +116,7 @@ function _check_env {
 
     # Write two files, so we confirm that podman can accept multiple values
     # and that the second will override the first
-    local envfile1="$PODMAN_TMPDIR/envfile-in-1"
+    local envfile1="$PODMAN_TMPDIR/envfile-in-1,withcomma"
     local envfile2="$PODMAN_TMPDIR/envfile-in-2"
     cat >$envfile1 <<EOF
 infile2=this is set in env-file-1 and should be overridden in env-file-2
@@ -158,6 +158,19 @@ EOF
     expect[weird*na#me!]=$weirdname
 
     _check_env $resultsfile
+
+    # Now check the same with podman exec
+    run_podman run -d --name testctr        \
+            -v "$resultsfile:/envresults:Z" \
+            $IMAGE top
+
+    run_podman exec --env-file $envfile1 \
+            --env-file $envfile2 testctr \
+            sh -c 'env -0 >/envresults'
+
+    _check_env $resultsfile
+
+    run_podman rm -f -t0 testctr
 }
 
 # Obscure feature: '--env FOO*' will pass all env starting with FOO
@@ -196,7 +209,7 @@ EOF
     fi
 
     # Same, with --env-file
-    local envfile="$PODMAN_TMPDIR/envfile-in-1"
+    local envfile="$PODMAN_TMPDIR/envfile-in-1,withcomma"
     cat >$envfile <<EOF
 $prefix*
 NOT*DEFINED
@@ -213,4 +226,40 @@ EOF
 }
 
 
+@test "podman create --label-file" {
+    declare -A expect=(
+        [simple]="abc"
+        [special]="bcd#e!f|g hij=lmnop"
+        [withquotes]='"withquotes"'
+        [withsinglequotes]="'withsingle'"
+    )
+
+    # Write two files, so we confirm that podman can accept multiple values
+    # and that the second will override the first
+    local labelfile1="$PODMAN_TMPDIR/label-file1,withcomma"
+    local labelfile2="$PODMAN_TMPDIR/label-file2"
+
+        cat >$labelfile1 <<EOF
+simple=value1
+
+# Comments ignored
+EOF
+
+    for v in "${!expect[@]}"; do
+        echo "$v=${expect[$v]}" >>$labelfile2
+    done
+
+    run_podman create --rm --name testctr --label-file $labelfile1  \
+               --label-file $labelfile2 $IMAGE
+
+    for v in "${!expect[@]}"; do
+        run_podman inspect testctr --format "{{index .Config.Labels \"$v\"}}"
+        assert "$output" == "${expect[$v]}" "label $v"
+    done
+
+    run_podman rm testctr
+}
+
+
+
 # vim: filetype=sh

test/system/800-config.bats

@@ -104,8 +104,8 @@ See 'podman create --help'" "--module must be specified before the command"
 
     run_podman rm -f $cid
 
-    # Nonexistent module path
-    nonesuch=${PODMAN_TMPDIR}/nonexistent
+    # Nonexistent module path with comma
+    nonesuch=${PODMAN_TMPDIR}/nonexistent,withcomma
     run_podman 1 --module=$nonesuch sdfsdfdsf
     is "$output" "Failed to obtain podman configuration: could not resolve module \"$nonesuch\": stat $nonesuch: no such file or directory" \
        "--module=ENOENT"