Introduce imageOptions

This is similar to the previous *Options structures, but this one
will support differing sets of options, in particular for the
copy source/destination.

The way the return values of imageFlags() are integrated into
creation of a cli.Command forces fakeContext() in tests to do
very ugly filtering to have a working *imageOptions available
without having a copyCmd() cooperate to give it to us.  Rather
than extend copyCmd(), we do the filtering, because the reliance
on copyCmd() will go away after all flags are migrated, and so
will the filtering and fakeContext() complexity.

Finally, rename contextFromGlobalOptions to not lie about only
caring about global options.

This only introduces the infrastructure, all flags continue
to be handled in the old way.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

cmd/skopeo/copy.go

@@ -16,14 +16,14 @@ import (
 	"github.com/urfave/cli"
 )
 
-// contextsFromGlobalOptions returns source and destionation types.SystemContext depending on c.
-func contextsFromGlobalOptions(c *cli.Context, global *globalOptions) (*types.SystemContext, *types.SystemContext, error) {
-	sourceCtx, err := contextFromGlobalOptions(c, global, "src-")
+// contextsFromCopyOptions returns source and destionation types.SystemContext depending on c.
+func contextsFromCopyOptions(c *cli.Context, opts *copyOptions) (*types.SystemContext, *types.SystemContext, error) {
+	sourceCtx, err := contextFromImageOptions(c, opts.srcImage, "src-")
 	if err != nil {
 		return nil, nil, err
 	}
 
-	destinationCtx, err := contextFromGlobalOptions(c, global, "dest-")
+	destinationCtx, err := contextFromImageOptions(c, opts.destImage, "dest-")
 	if err != nil {
 		return nil, nil, err
 	}
@@ -33,6 +33,8 @@ func contextsFromGlobalOptions(c *cli.Context, global *globalOptions) (*types.Sy
 
 type copyOptions struct {
 	global            *globalOptions
+	srcImage          *imageOptions
+	destImage         *imageOptions
 	additionalTags    cli.StringSlice // For docker-archive: destinations, in addition to the name:tag specified as destination, also add these
 	removeSignatures  bool            // Do not copy signatures from the source image
 	signByFingerprint string          // Sign the image using a GPG key with the specified fingerprint
@@ -40,7 +42,13 @@ type copyOptions struct {
 }
 
 func copyCmd(global *globalOptions) cli.Command {
-	opts := copyOptions{global: global}
+	srcFlags, srcOpts := imageFlags(global, "src-")
+	destFlags, destOpts := imageFlags(global, "dest-")
+	opts := copyOptions{global: global,
+		srcImage:  srcOpts,
+		destImage: destOpts,
+	}
+
 	return cli.Command{
 		Name:  "copy",
 		Usage: "Copy an IMAGE-NAME from one location to another",
@@ -56,7 +64,7 @@ func copyCmd(global *globalOptions) cli.Command {
 		ArgsUsage: "SOURCE-IMAGE DESTINATION-IMAGE",
 		Action:    opts.run,
 		// FIXME: Do we need to namespace the GPG aspect?
-		Flags: []cli.Flag{
+		Flags: append(append([]cli.Flag{
 			cli.StringSliceFlag{
 				Name:  "additional-tag",
 				Usage: "additional tags (supports docker-archive)",
@@ -138,7 +146,7 @@ func copyCmd(global *globalOptions) cli.Command {
 				Value: "",
 				Usage: "use docker daemon host at `HOST` (docker-daemon destinations only)",
 			},
-		},
+		}, srcFlags...), destFlags...),
 	}
 }
 
@@ -163,7 +171,7 @@ func (opts *copyOptions) run(c *cli.Context) error {
 		return fmt.Errorf("Invalid destination name %s: %v", c.Args()[1], err)
 	}
 
-	sourceCtx, destinationCtx, err := contextsFromGlobalOptions(c, opts.global)
+	sourceCtx, destinationCtx, err := contextsFromCopyOptions(c, opts)
 	if err != nil {
 		return err
 	}

cmd/skopeo/delete.go

@@ -12,10 +12,15 @@ import (
 
 type deleteOptions struct {
 	global *globalOptions
+	image  *imageOptions
 }
 
 func deleteCmd(global *globalOptions) cli.Command {
-	opts := deleteOptions{global: global}
+	imageFlags, imageOpts := imageFlags(global, "")
+	opts := deleteOptions{
+		global: global,
+		image:  imageOpts,
+	}
 	return cli.Command{
 		Name:  "delete",
 		Usage: "Delete image IMAGE-NAME",
@@ -29,7 +34,7 @@ func deleteCmd(global *globalOptions) cli.Command {
 	`, strings.Join(transports.ListNames(), ", ")),
 		ArgsUsage: "IMAGE-NAME",
 		Action:    opts.run,
-		Flags: []cli.Flag{
+		Flags: append([]cli.Flag{
 			cli.StringFlag{
 				Name:  "authfile",
 				Usage: "path of the authentication file. Default is ${XDG_RUNTIME_DIR}/containers/auth.json",
@@ -48,7 +53,7 @@ func deleteCmd(global *globalOptions) cli.Command {
 				Name:  "tls-verify",
 				Usage: "require HTTPS and verify certificates when talking to container registries (defaults to true)",
 			},
-		},
+		}, imageFlags...),
 	}
 }
 
@@ -62,7 +67,7 @@ func (opts *deleteOptions) run(c *cli.Context) error {
 		return fmt.Errorf("Invalid source name %s: %v", c.Args()[0], err)
 	}
 
-	sys, err := contextFromGlobalOptions(c, opts.global, "")
+	sys, err := contextFromImageOptions(c, opts.image, "")
 	if err != nil {
 		return err
 	}

cmd/skopeo/inspect.go

@@ -31,11 +31,16 @@ type inspectOutput struct {
 
 type inspectOptions struct {
 	global *globalOptions
+	image  *imageOptions
 	raw    bool // Output the raw manifest instead of parsing information about the image
 }
 
 func inspectCmd(global *globalOptions) cli.Command {
-	opts := inspectOptions{global: global}
+	imageFlags, imageOpts := imageFlags(global, "")
+	opts := inspectOptions{
+		global: global,
+		image:  imageOpts,
+	}
 	return cli.Command{
 		Name:  "inspect",
 		Usage: "Inspect image IMAGE-NAME",
@@ -48,7 +53,7 @@ func inspectCmd(global *globalOptions) cli.Command {
 	See skopeo(1) section "IMAGE NAMES" for the expected format
 	`, strings.Join(transports.ListNames(), ", ")),
 		ArgsUsage: "IMAGE-NAME",
-		Flags: []cli.Flag{
+		Flags: append([]cli.Flag{
 			cli.StringFlag{
 				Name:  "authfile",
 				Usage: "path of the authentication file. Default is ${XDG_RUNTIME_DIR}/containers/auth.json",
@@ -72,7 +77,7 @@ func inspectCmd(global *globalOptions) cli.Command {
 				Value: "",
 				Usage: "Use `USERNAME[:PASSWORD]` for accessing the registry",
 			},
-		},
+		}, imageFlags...),
 		Action: opts.run,
 	}
 }
@@ -81,7 +86,7 @@ func (opts *inspectOptions) run(c *cli.Context) (retErr error) {
 	ctx, cancel := opts.global.commandTimeoutContext()
 	defer cancel()
 
-	img, err := parseImage(ctx, c, opts.global)
+	img, err := parseImage(ctx, c, opts.image)
 	if err != nil {
 		return err
 	}
@@ -127,7 +132,7 @@ func (opts *inspectOptions) run(c *cli.Context) (retErr error) {
 		outputData.Name = dockerRef.Name()
 	}
 	if img.Reference().Transport() == docker.Transport {
-		sys, err := contextFromGlobalOptions(c, opts.global, "")
+		sys, err := contextFromImageOptions(c, opts.image, "")
 		if err != nil {
 			return err
 		}

cmd/skopeo/layers.go

@@ -17,16 +17,22 @@ import (
 
 type layersOptions struct {
 	global *globalOptions
+	image  *imageOptions
 }
 
 func layersCmd(global *globalOptions) cli.Command {
-	opts := &layersOptions{global: global}
+	imageFlags, imageOpts := imageFlags(global, "")
+	opts := layersOptions{
+		global: global,
+		image:  imageOpts,
+	}
 	return cli.Command{
 		Name:      "layers",
 		Usage:     "Get layers of IMAGE-NAME",
 		ArgsUsage: "IMAGE-NAME [LAYER...]",
 		Hidden:    true,
 		Action:    opts.run,
+		Flags:     imageFlags,
 	}
 }
 
@@ -39,12 +45,12 @@ func (opts *layersOptions) run(c *cli.Context) (retErr error) {
 	ctx, cancel := opts.global.commandTimeoutContext()
 	defer cancel()
 
-	sys, err := contextFromGlobalOptions(c, opts.global, "")
+	sys, err := contextFromImageOptions(c, opts.image, "")
 	if err != nil {
 		return err
 	}
 	cache := blobinfocache.DefaultCache(sys)
-	rawSource, err := parseImageSource(ctx, c, opts.global, c.Args()[0])
+	rawSource, err := parseImageSource(ctx, c, opts.image, c.Args()[0])
 	if err != nil {
 		return err
 	}

cmd/skopeo/utils.go

@@ -10,11 +10,23 @@ import (
 	"github.com/urfave/cli"
 )
 
-func contextFromGlobalOptions(c *cli.Context, global *globalOptions, flagPrefix string) (*types.SystemContext, error) {
+// imageOptions collects CLI flags which are the same across subcommands, but may be different for each image
+// (e.g. may differ between the source and destination of a copy)
+type imageOptions struct {
+	global *globalOptions // May be shared across several imageOptions instances.
+}
+
+// imageFlags prepares a collection of CLI flags writing into imageOptions, and the managed imageOptions structure.
+func imageFlags(global *globalOptions, flagPrefix string) ([]cli.Flag, *imageOptions) {
+	opts := imageOptions{global: global}
+	return []cli.Flag{}, &opts
+}
+
+func contextFromImageOptions(c *cli.Context, opts *imageOptions, flagPrefix string) (*types.SystemContext, error) {
 	ctx := &types.SystemContext{
-		RegistriesDirPath:                 global.registriesDirPath,
-		ArchitectureChoice:                global.overrideArch,
-		OSChoice:                          global.overrideOS,
+		RegistriesDirPath:                 opts.global.registriesDirPath,
+		ArchitectureChoice:                opts.global.overrideArch,
+		OSChoice:                          opts.global.overrideOS,
 		DockerCertPath:                    c.String(flagPrefix + "cert-dir"),
 		OSTreeTmpDirPath:                  c.String(flagPrefix + "ostree-tmp-dir"),
 		OCISharedBlobDirPath:              c.String(flagPrefix + "shared-blob-dir"),
@@ -25,8 +37,8 @@ func contextFromGlobalOptions(c *cli.Context, global *globalOptions, flagPrefix
 		DockerDaemonInsecureSkipTLSVerify: !c.BoolT(flagPrefix + "tls-verify"),
 	}
 	// DEPRECATED: We support this for backward compatibility, but override it if a per-image flag is provided.
-	if global.tlsVerify.present {
-		ctx.DockerInsecureSkipTLSVerify = types.NewOptionalBool(!global.tlsVerify.value)
+	if opts.global.tlsVerify.present {
+		ctx.DockerInsecureSkipTLSVerify = types.NewOptionalBool(!opts.global.tlsVerify.value)
 	}
 	if c.IsSet(flagPrefix + "tls-verify") {
 		ctx.DockerInsecureSkipTLSVerify = types.NewOptionalBool(!c.BoolT(flagPrefix + "tls-verify"))
@@ -68,13 +80,13 @@ func getDockerAuth(creds string) (*types.DockerAuthConfig, error) {
 
 // parseImage converts image URL-like string to an initialized handler for that image.
 // The caller must call .Close() on the returned ImageCloser.
-func parseImage(ctx context.Context, c *cli.Context, global *globalOptions) (types.ImageCloser, error) {
+func parseImage(ctx context.Context, c *cli.Context, opts *imageOptions) (types.ImageCloser, error) {
 	imgName := c.Args().First()
 	ref, err := alltransports.ParseImageName(imgName)
 	if err != nil {
 		return nil, err
 	}
-	sys, err := contextFromGlobalOptions(c, global, "")
+	sys, err := contextFromImageOptions(c, opts, "")
 	if err != nil {
 		return nil, err
 	}
@@ -83,12 +95,12 @@ func parseImage(ctx context.Context, c *cli.Context, global *globalOptions) (typ
 
 // parseImageSource converts image URL-like string to an ImageSource.
 // The caller must call .Close() on the returned ImageSource.
-func parseImageSource(ctx context.Context, c *cli.Context, global *globalOptions, name string) (types.ImageSource, error) {
+func parseImageSource(ctx context.Context, c *cli.Context, opts *imageOptions, name string) (types.ImageSource, error) {
 	ref, err := alltransports.ParseImageName(name)
 	if err != nil {
 		return nil, err
 	}
-	sys, err := contextFromGlobalOptions(c, global, "")
+	sys, err := contextFromImageOptions(c, opts, "")
 	if err != nil {
 		return nil, err
 	}

cmd/skopeo/utils_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"flag"
+	"strings"
 	"testing"
 
 	"github.com/containers/image/types"
@@ -10,10 +11,10 @@ import (
 	"github.com/urfave/cli"
 )
 
-// fakeContext creates inputs for contextFromGlobalOptions.
+// fakeContext creates inputs for contextFromImageOptions.
 // NOTE: This is QUITE FAKE; none of the urfave/cli normalization and the like happens.
-func fakeContext(t *testing.T, cmdName string, globalFlags []string, cmdFlags []string) (*cli.Context, *globalOptions) {
-	app, global := createApp()
+func fakeContext(t *testing.T, cmdName string, flagPrefix string, globalFlags []string, cmdFlags []string) (*cli.Context, *imageOptions) {
+	app, globalOpts := createApp()
 
 	globalSet := flag.NewFlagSet(app.Name, flag.ContinueOnError)
 	for _, f := range app.Flags {
@@ -25,35 +26,54 @@ func fakeContext(t *testing.T, cmdName string, globalFlags []string, cmdFlags []
 
 	cmd := app.Command(cmdName)
 	require.NotNil(t, cmd)
-	cmdSet := flag.NewFlagSet(cmd.Name, flag.ContinueOnError)
-	for _, f := range cmd.Flags {
-		f.Apply(cmdSet)
+
+	imageFlags, imageOpts := imageFlags(globalOpts, flagPrefix)
+	appliedFlags := map[string]struct{}{}
+	// Ugly: cmd.Flags includes imageFlags as well.  For now, we need cmd.Flags to apply here
+	// to be able to test the non-Destination: flags, but we must not apply the same flag name twice.
+	// So, primarily use imageFlags (so that Destination: is used as expected), and then follow up with
+	// the remaining flags from cmd.Flags (so that cli.Context.String() etc. works).
+	// This is horribly ugly, but all of this will disappear within this PR.
+	firstName := func(f cli.Flag) string { // We even need to recognize "dest-creds,dcreds".  This will disappear as well.
+		return strings.Split(f.GetName(), ",")[0]
 	}
+	cmdSet := flag.NewFlagSet(cmd.Name, flag.ContinueOnError)
+	for _, f := range imageFlags {
+		f.Apply(cmdSet)
+		appliedFlags[firstName(f)] = struct{}{}
+	}
+	for _, f := range cmd.Flags {
+		if _, ok := appliedFlags[firstName(f)]; !ok {
+			f.Apply(cmdSet)
+			appliedFlags[firstName(f)] = struct{}{}
+		}
+	}
+
 	err = cmdSet.Parse(cmdFlags)
 	require.NoError(t, err)
-	return cli.NewContext(app, cmdSet, globalCtx), global
+	return cli.NewContext(app, cmdSet, globalCtx), imageOpts
 }
 
-func TestContextFromGlobalOptions(t *testing.T) {
+func TestContextFromImageOptions(t *testing.T) {
 	// FIXME: All of this only tests (skopeo copy --dest)
 	// FIXME FIXME: Apparently BoolT values are set to false if the flag is not declared for the specific subcommand!!
 
 	// Default state
-	c, global := fakeContext(t, "copy", []string{}, []string{})
-	res, err := contextFromGlobalOptions(c, global, "dest-")
+	c, opts := fakeContext(t, "copy", "dest-", []string{}, []string{})
+	res, err := contextFromImageOptions(c, opts, "dest-")
 	require.NoError(t, err)
 	assert.Equal(t, &types.SystemContext{}, res)
 
 	// Explicitly set everything to default, except for when the default is “not present”
-	c, global = fakeContext(t, "copy", []string{}, []string{
+	c, opts = fakeContext(t, "copy", "dest-", []string{}, []string{
 		"--dest-compress=false",
 	})
-	res, err = contextFromGlobalOptions(c, global, "dest-")
+	res, err = contextFromImageOptions(c, opts, "dest-")
 	require.NoError(t, err)
 	assert.Equal(t, &types.SystemContext{}, res)
 
 	// Set everything to non-default values.
-	c, global = fakeContext(t, "copy", []string{
+	c, opts = fakeContext(t, "copy", "dest-", []string{
 		"--registries.d", "/srv/registries.d",
 		"--override-arch", "overridden-arch",
 		"--override-os", "overridden-os",
@@ -67,7 +87,7 @@ func TestContextFromGlobalOptions(t *testing.T) {
 		"--dest-tls-verify=false",
 		"--dest-creds", "creds-user:creds-password",
 	})
-	res, err = contextFromGlobalOptions(c, global, "dest-")
+	res, err = contextFromImageOptions(c, opts, "dest-")
 	require.NoError(t, err)
 	assert.Equal(t, &types.SystemContext{
 		RegistriesDirPath:                 "/srv/registries.d",
@@ -109,15 +129,15 @@ func TestContextFromGlobalOptions(t *testing.T) {
 		if c.cmd != "" {
 			cmdFlags = append(cmdFlags, "--dest-tls-verify="+c.cmd)
 		}
-		ctx, global := fakeContext(t, "copy", globalFlags, cmdFlags)
-		res, err = contextFromGlobalOptions(ctx, global, "dest-")
+		ctx, opts := fakeContext(t, "copy", "dest-", globalFlags, cmdFlags)
+		res, err = contextFromImageOptions(ctx, opts, "dest-")
 		require.NoError(t, err)
 		assert.Equal(t, c.expectedDocker, res.DockerInsecureSkipTLSVerify, "%#v", c)
 		assert.Equal(t, c.expectedDockerDaemon, res.DockerDaemonInsecureSkipTLSVerify, "%#v", c)
 	}
 
 	// Invalid option values
-	c, global = fakeContext(t, "copy", []string{}, []string{"--dest-creds", ""})
-	_, err = contextFromGlobalOptions(c, global, "dest-")
+	c, opts = fakeContext(t, "copy", "dest-", []string{}, []string{"--dest-creds", ""})
+	_, err = contextFromImageOptions(c, opts, "dest-")
 	assert.Error(t, err)
 }