switch buildah manifest annotations flags to use StringArrayVar

[Taywee]

This prevents splitting on commas, which previously made it challenging to include an actual comma in an annotation.

What type of PR is this?

/kind bug

What this PR does / why we need it:

Annotations flags shouldn't split on commas.

How to verify it

$ ./bin/buildah manifest create --annotation=foo=bar '--annotation=alpha=beta, gamma, and delta again' localhost/this/test:version2
9ea39caf64ab94401e16b6de5574e03720584cc12624dcd67b45660f0ed5cd5b

$ ./bin/buildah manifest inspect localhost/this/test:version2
{
    "schemaVersion": 2,
    "mediaType": "application/vnd.oci.image.index.v1+json",
    "manifests": [],
    "annotations": {
        "alpha": "beta, gamma, and delta again",
        "foo": "bar"
    }
}

Which issue(s) this PR fixes:

Fixes #6793

Does this PR introduce a user-facing change?

For people working around this issue by wrapping in quotes, this will cause the quotes to start appearing in the key and value:

$ buildah manifest create --annotation=foo=bar '--annotation="alpha=beta, gamma, and delta"' localhost/this/test:version3
1bd72d935fd0d2d6900a04ed10c84bdfea1f72f1565ea04bd871786772778872

$ buildah manifest inspect localhost/this/test:version3
{
    "schemaVersion": 2,
    "mediaType": "application/vnd.oci.image.index.v1+json",
    "manifests": [],
    "annotations": {
        "alpha": "beta, gamma, and delta",
        "foo": "bar"
    }
}

$ ./bin/buildah manifest create --annotation=foo=bar '--annotation="alpha=beta, gamma, and delta"' localhost/this/test:version4
e6a029efbdb496b9348f00c5a60987a22c3387ac34a9e602969dd333a6e63ccd

$ ./bin/buildah manifest inspect localhost/this/test:version4
{
    "schemaVersion": 2,
    "mediaType": "application/vnd.oci.image.index.v1+json",
    "manifests": [],
    "annotations": {
        "\"alpha": "beta, gamma, and delta\"",
        "foo": "bar"
    }
}

`buildah manifest create --annotation` and `buildah manifest annotate --annotation` now handle annotation values the same way that `buildah bud` does, not splitting on commas, and treating surrounding quotation marks as literal characters.

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[Honny1]

Change LGTM, but please create tests for this.

[Luap99]

IMO this needs some larger discussion, it is clearly a breaking change.

Overall I agree that this makes sense, I think StringSliceVar() is almost always a mistake. On the podman side I wrote this https://github.com/containers/podman/tree/main/cmd/podman#adding-cli-flags

But spot fixing one single call here when I see a ton of uses here does not seem good to me. I switched the podman flags over as part of podman 5, containers/podman#20945 containers/podman#20976

@nalind how do you feel about this? I think this is a structural problem which should be fixed as such and not one by one. But would such a breaking change acceptable? Buildah already consumes several breaking changes such as the config rewrite but I am not sure you want to bump to a v2? Or just do this change without bumping a major?

    0 cmd/buildah/addcopy.go:84                           84 flags.StringSliceVar(&opts.decryptionKeys, "decryption-key", nil, "key needed to decrypt a pulled image")
    1 cmd/buildah/addcopy.go:88                           88 flags.StringSliceVar(&opts.excludes, "exclude", nil, "exclude pattern when copying files")
    2 cmd/buildah/commit.go:108                          108 flags.StringSliceVar(&opts.encryptionKeys, "encryption-key", nil, "key with the encryption protocol to use needed to encrypt the i>
    3 cmd/buildah/commit.go:196                          196 flags.StringSliceVar(&opts.unsetenvs, "unsetenv", nil, "unset env from final image")
    4 cmd/buildah/commit.go:198                          198 flags.StringSliceVar(&opts.unsetAnnotation, "unsetannotation", nil, "unset annotation when inheriting annotations from base image")
    5 cmd/buildah/config.go:95                            95 flags.StringSliceVar(&opts.onbuild, "onbuild", []string{}, "add onbuild command to be run on images based on this image. Only supp>
    6 cmd/buildah/config.go:99                            99 flags.StringSliceVarP(&opts.ports, "port", "p", []string{}, "add `port` to expose when running containers based on image (default >
    7 cmd/buildah/config.go:104                          104 flags.StringSliceVarP(&opts.volume, "volume", "v", []string{}, "add default `volume` path to be created for containers based on im>
    8 cmd/buildah/config.go:106                          106 flags.StringSliceVar(&opts.unsetLabels, "unsetlabel", nil, "remove image configuration label")
    9 cmd/buildah/config.go:107                          107 flags.StringSliceVar(&opts.unsetAnnotations, "unsetannotation", nil, "remove image configuration annotation")
   10 cmd/buildah/images.go:97                            97 flags.StringSliceVarP(&opts.filter, "filter", "f", []string{}, "filter output based on conditions provided")
   11 cmd/buildah/main.go:120                            120 rootCmd.PersistentFlags().StringSliceVar(&globalFlagResults.StorageOpts, "storage-opt", defaultStoreDriverOptions, "storage driver>
   12 cmd/buildah/main.go:121                            121 rootCmd.PersistentFlags().StringSliceVar(&globalFlagResults.UserNSUID, "userns-uid-map", []string{}, "default `ctrID:hostID:length>
   13 cmd/buildah/main.go:122                            122 rootCmd.PersistentFlags().StringSliceVar(&globalFlagResults.UserNSGID, "userns-gid-map", []string{}, "default `ctrID:hostID:length>
   14 cmd/buildah/manifest.go:114                        114 flags.StringSliceVar(&manifestCreateOpts.annotations, "annotation", nil, "set an `annotation` for the image index")
   15 cmd/buildah/manifest.go:152                        152 flags.StringSliceVar(&manifestAddOpts.artifactAnnotations, "artifact-annotation", nil, "artifact annotation")
   16 cmd/buildah/manifest.go:160                        160 flags.StringSliceVar(&manifestAddOpts.features, "features", nil, "override the `features` of the specified image")
   17 cmd/buildah/manifest.go:161                        161 flags.StringSliceVar(&manifestAddOpts.osFeatures, "os-features", nil, "override the OS `features` of the specified image")
   18 cmd/buildah/manifest.go:162                        162 flags.StringSliceVar(&manifestAddOpts.annotations, "annotation", nil, "set an `annotation` for the specified image or artifact")
   19 cmd/buildah/manifest.go:214                        214 flags.StringSliceVar(&manifestAnnotateOpts.features, "features", nil, "override the `features` of the specified image")
   20 cmd/buildah/manifest.go:215                        215 flags.StringSliceVar(&manifestAnnotateOpts.osFeatures, "os-features", nil, "override the os `features` of the specified image")
   21 cmd/buildah/manifest.go:216                        216 flags.StringSliceVar(&manifestAnnotateOpts.annotations, "annotation", nil, "set an `annotation` for the specified image")
   22 cmd/buildah/pull.go:68                              68 flags.StringSliceVar(&opts.decryptionKeys, "decryption-key", nil, "key needed to decrypt the image")
   23 cmd/buildah/push.go:101                            101 flags.StringSliceVar(&opts.encryptionKeys, "encryption-key", nil, "key with the encryption protocol to use needed to encrypt the i>
   24 cmd/buildah/run.go:70                               70 flags.StringSliceVar(&opts.capAdd, "cap-add", []string{}, "add the specified capability (default [])")
   25 cmd/buildah/run.go:71                               71 flags.StringSliceVar(&opts.capDrop, "cap-drop", []string{}, "drop the specified capability (default [])")
   26 cmd/buildah/run.go:81                               81 flags.StringSliceVar(&opts.runtimeFlag, "runtime-flag", []string{}, "add global flags for the container runtime")
   27 cmd/buildah/unshare.go:38                           38 flags.StringSliceVarP(&unshareMounts, "mount", "m", []string{}, "mount the specified containers (default [])")
   28 pkg/cli/common.go:175                              175 usernsFlags.StringSliceVar(&flags.GroupAdd, "group-add", nil, "add additional groups to the primary container process. 'keep-group>
   29 pkg/cli/common.go:177                              177 usernsFlags.StringSliceVar(&flags.UserNSUIDMap, "userns-uid-map", []string{}, "`containerUID:hostUID:length` UID mapping to use in>
   30 pkg/cli/common.go:178                              178 usernsFlags.StringSliceVar(&flags.UserNSGIDMap, "userns-gid-map", []string{}, "`containerGID:hostGID:length` GID mapping to use in>
   31 pkg/cli/common.go:258                              258 fs.StringSliceVarP(&flags.File, "file", "f", []string{}, "`pathname or URL` of a Dockerfile")
   32 pkg/cli/common.go:306                              306 fs.StringSliceVar(&flags.RuntimeFlags, "runtime-flag", []string{}, "add global flags for the container runtime")
   33 pkg/cli/common.go:339                              339 fs.StringSliceVar(&flags.UnsetEnvs, "unsetenv", nil, "unset environment variable from final image")
   34 pkg/cli/common.go:340                              340 fs.StringSliceVar(&flags.UnsetLabels, "unsetlabel", nil, "unset label when inheriting labels from base image")
   35 pkg/cli/common.go:341                              341 fs.StringSliceVar(&flags.UnsetAnnotations, "unsetannotation", nil, "unset annotation when inheriting annotations from base image")
   36 pkg/cli/common.go:414                              414 fs.StringSliceVar(&flags.AddHost, "add-host", []string{}, "add a custom host-to-IP mapping (`host:ip`) (default [])")
   37 pkg/cli/common.go:419                              419 fs.StringSliceVar(&flags.CapAdd, "cap-add", []string{}, "add the specified capability when running (default [])")
   38 pkg/cli/common.go:420                              420 fs.StringSliceVar(&flags.CapDrop, "cap-drop", []string{}, "drop the specified capability when running (default [])")
   39 pkg/cli/common.go:429                              429 fs.StringSliceVar(&flags.DecryptionKeys, "decryption-key", nil, "key needed to decrypt the image")
   40 pkg/cli/common.go:431                              431 fs.StringSliceVar(&flags.DNSSearch, "dns-search", defaultContainerConfig.Containers.DNSSearches.Get(), "set custom DNS search doma>
   41 pkg/cli/common.go:432                              432 fs.StringSliceVar(&flags.DNSServers, "dns", defaultContainerConfig.Containers.DNSServers.Get(), "set custom DNS servers or disable>
   42 pkg/cli/common.go:433                              433 fs.StringSliceVar(&flags.DNSOptions, "dns-option", defaultContainerConfig.Containers.DNSOptions.Get(), "set custom DNS options")
   43 pkg/cli/common.go:446                              446 fs.StringSliceVar(&flags.Ulimit, "ulimit", defaultContainerConfig.Containers.DefaultUlimits.Get(), "ulimit options")
   44 tests/copy/copy.go:147                             147 rootCmd.PersistentFlags().StringSliceVar(&storeOptions.GraphDriverOptions, "storage-opt", nil, "storage option")

[Taywee]

@Luap99 I agree that a more structural fix is probably better, but most of those uses are things that commas could not appear in anyway. Some, like capAdd and capDrop, are arguably useful. A full audit of what really really shouldn't be StringSliceVar is probably warranted.

@Honny1 I'll see if I can manage that; I haven't worked with bats before.

[Luap99]

Some, like capAdd and capDrop, are arguably useful. A full audit of what really really shouldn't be StringSliceVar is probably warranted.

which is what I described in https://github.com/containers/podman/tree/main/cmd/podman#adding-cli-flags

[Taywee]

which is what I described in https://github.com/containers/podman/tree/main/cmd/podman#adding-cli-flags

Oof, my mistake; I didn't read it all the way through.

@Honny1 I've augmented tests now to check for this particular case.

[nalind]

Barring a few exceptions (I'm mainly thinking of --platform, where the man page calls it out specifically, and maybe --cap-add and --cap-drop), are we creating an expectation that flags accept multiple values separated by commas when they treat multiple occurrences of the flag as indicating multiple values? If not, then it's a bug, we should fix it and move on.