Create an "options" structure for each command

This is a big diff, but it really only replaces a few global variables
with functions returning a structure.

The ultimate goal of this patch set is to replace option handling using

> cli.StringFlag{Name:"foo", ...}
> ...
> func somethingHandler(c *cli.Context) error {
>     c.String("foo")
> }

where the declaration and usage are connected only using a string constant,
and it's difficult to notice that one or the other is missing or that the
types don't match, by

> struct somethingOptions {
>    foo string
> }
> ...
> cli.StringFlag{Name:"foo", Destination:&foo}
> ...
> func (opts *somethingOptions) run(c *cli.Context) error {
>     opts.foo
> }

As a first step, this commit ONLY introduces the *Options structures,
but for now empty; nothing changes in the existing implementations.

So, we go from

> func somethingHandler(c *cli.Context error {...}
>
> var somethingCmd = cli.Command {
>     ...
>     Action: somethingHandler
> }

to

> type somethingOptions struct{
> } // empty for now
>
> func somethingCmd() cli.Command {
>     opts := somethingOptions{}
>     return cli.Command {
>         ... // unchanged
>         Action: opts.run
>     }
> }
>
> func (opts *somethingOptions) run(c *cli.context) error {...} // unchanged

Using the struct type has also made it possible to place the definition of
cli.Command in front of the actual command handler, so do that for better
readability.

In a few cases this also broke out an in-line lambda in the Action: field
into a separate opts.run method.  Again, nothing else has changed.

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

cmd/skopeo/copy.go

@@ -31,77 +31,12 @@ func contextsFromGlobalOptions(c *cli.Context) (*types.SystemContext, *types.Sys
 	return sourceCtx, destinationCtx, nil
 }
 
-func copyHandler(c *cli.Context) error {
+type copyOptions struct {
-	if len(c.Args()) != 2 {
-		cli.ShowCommandHelp(c, "copy")
-		return errors.New("Exactly two arguments expected")
 }
 
-	policyContext, err := getPolicyContext(c)
+func copyCmd() cli.Command {
-	if err != nil {
+	opts := copyOptions{}
-		return fmt.Errorf("Error loading trust policy: %v", err)
+	return cli.Command{
-	}
-	defer policyContext.Destroy()
-
-	srcRef, err := alltransports.ParseImageName(c.Args()[0])
-	if err != nil {
-		return fmt.Errorf("Invalid source name %s: %v", c.Args()[0], err)
-	}
-	destRef, err := alltransports.ParseImageName(c.Args()[1])
-	if err != nil {
-		return fmt.Errorf("Invalid destination name %s: %v", c.Args()[1], err)
-	}
-	signBy := c.String("sign-by")
-	removeSignatures := c.Bool("remove-signatures")
-
-	sourceCtx, destinationCtx, err := contextsFromGlobalOptions(c)
-	if err != nil {
-		return err
-	}
-
-	var manifestType string
-	if c.IsSet("format") {
-		switch c.String("format") {
-		case "oci":
-			manifestType = imgspecv1.MediaTypeImageManifest
-		case "v2s1":
-			manifestType = manifest.DockerV2Schema1SignedMediaType
-		case "v2s2":
-			manifestType = manifest.DockerV2Schema2MediaType
-		default:
-			return fmt.Errorf("unknown format %q. Choose on of the supported formats: 'oci', 'v2s1', or 'v2s2'", c.String("format"))
-		}
-	}
-
-	if c.IsSet("additional-tag") {
-		for _, image := range c.StringSlice("additional-tag") {
-			ref, err := reference.ParseNormalizedNamed(image)
-			if err != nil {
-				return fmt.Errorf("error parsing additional-tag '%s': %v", image, err)
-			}
-			namedTagged, isNamedTagged := ref.(reference.NamedTagged)
-			if !isNamedTagged {
-				return fmt.Errorf("additional-tag '%s' must be a tagged reference", image)
-			}
-			destinationCtx.DockerArchiveAdditionalTags = append(destinationCtx.DockerArchiveAdditionalTags, namedTagged)
-		}
-	}
-
-	ctx, cancel := commandTimeoutContextFromGlobalOptions(c)
-	defer cancel()
-
-	_, err = copy.Image(ctx, policyContext, destRef, srcRef, &copy.Options{
-		RemoveSignatures:      removeSignatures,
-		SignBy:                signBy,
-		ReportWriter:          os.Stdout,
-		SourceCtx:             sourceCtx,
-		DestinationCtx:        destinationCtx,
-		ForceManifestMIMEType: manifestType,
-	})
-	return err
-}
-
-var copyCmd = cli.Command{
 		Name:  "copy",
 		Usage: "Copy an IMAGE-NAME from one location to another",
 		Description: fmt.Sprintf(`
@@ -114,7 +49,7 @@ var copyCmd = cli.Command{
 	See skopeo(1) section "IMAGE NAMES" for the expected format
 	`, strings.Join(transports.ListNames(), ", ")),
 		ArgsUsage: "SOURCE-IMAGE DESTINATION-IMAGE",
-	Action:    copyHandler,
+		Action:    opts.run,
 		// FIXME: Do we need to namespace the GPG aspect?
 		Flags: []cli.Flag{
 			cli.StringSliceFlag{
@@ -196,3 +131,74 @@ var copyCmd = cli.Command{
 			},
 		},
 	}
+}
+
+func (opts *copyOptions) run(c *cli.Context) error {
+	if len(c.Args()) != 2 {
+		cli.ShowCommandHelp(c, "copy")
+		return errors.New("Exactly two arguments expected")
+	}
+
+	policyContext, err := getPolicyContext(c)
+	if err != nil {
+		return fmt.Errorf("Error loading trust policy: %v", err)
+	}
+	defer policyContext.Destroy()
+
+	srcRef, err := alltransports.ParseImageName(c.Args()[0])
+	if err != nil {
+		return fmt.Errorf("Invalid source name %s: %v", c.Args()[0], err)
+	}
+	destRef, err := alltransports.ParseImageName(c.Args()[1])
+	if err != nil {
+		return fmt.Errorf("Invalid destination name %s: %v", c.Args()[1], err)
+	}
+	signBy := c.String("sign-by")
+	removeSignatures := c.Bool("remove-signatures")
+
+	sourceCtx, destinationCtx, err := contextsFromGlobalOptions(c)
+	if err != nil {
+		return err
+	}
+
+	var manifestType string
+	if c.IsSet("format") {
+		switch c.String("format") {
+		case "oci":
+			manifestType = imgspecv1.MediaTypeImageManifest
+		case "v2s1":
+			manifestType = manifest.DockerV2Schema1SignedMediaType
+		case "v2s2":
+			manifestType = manifest.DockerV2Schema2MediaType
+		default:
+			return fmt.Errorf("unknown format %q. Choose on of the supported formats: 'oci', 'v2s1', or 'v2s2'", c.String("format"))
+		}
+	}
+
+	if c.IsSet("additional-tag") {
+		for _, image := range c.StringSlice("additional-tag") {
+			ref, err := reference.ParseNormalizedNamed(image)
+			if err != nil {
+				return fmt.Errorf("error parsing additional-tag '%s': %v", image, err)
+			}
+			namedTagged, isNamedTagged := ref.(reference.NamedTagged)
+			if !isNamedTagged {
+				return fmt.Errorf("additional-tag '%s' must be a tagged reference", image)
+			}
+			destinationCtx.DockerArchiveAdditionalTags = append(destinationCtx.DockerArchiveAdditionalTags, namedTagged)
+		}
+	}
+
+	ctx, cancel := commandTimeoutContextFromGlobalOptions(c)
+	defer cancel()
+
+	_, err = copy.Image(ctx, policyContext, destRef, srcRef, &copy.Options{
+		RemoveSignatures:      removeSignatures,
+		SignBy:                signBy,
+		ReportWriter:          os.Stdout,
+		SourceCtx:             sourceCtx,
+		DestinationCtx:        destinationCtx,
+		ForceManifestMIMEType: manifestType,
+	})
+	return err
+}

cmd/skopeo/delete.go

@@ -10,27 +10,12 @@ import (
 	"github.com/urfave/cli"
 )
 
-func deleteHandler(c *cli.Context) error {
+type deleteOptions struct {
-	if len(c.Args()) != 1 {
-		return errors.New("Usage: delete imageReference")
 }
 
-	ref, err := alltransports.ParseImageName(c.Args()[0])
+func deleteCmd() cli.Command {
-	if err != nil {
+	opts := deleteOptions{}
-		return fmt.Errorf("Invalid source name %s: %v", c.Args()[0], err)
+	return cli.Command{
-	}
-
-	sys, err := contextFromGlobalOptions(c, "")
-	if err != nil {
-		return err
-	}
-
-	ctx, cancel := commandTimeoutContextFromGlobalOptions(c)
-	defer cancel()
-	return ref.DeleteImage(ctx, sys)
-}
-
-var deleteCmd = cli.Command{
 		Name:  "delete",
 		Usage: "Delete image IMAGE-NAME",
 		Description: fmt.Sprintf(`
@@ -42,7 +27,7 @@ var deleteCmd = cli.Command{
 	See skopeo(1) section "IMAGE NAMES" for the expected format
 	`, strings.Join(transports.ListNames(), ", ")),
 		ArgsUsage: "IMAGE-NAME",
-	Action:    deleteHandler,
+		Action:    opts.run,
 		Flags: []cli.Flag{
 			cli.StringFlag{
 				Name:  "authfile",
@@ -64,3 +49,24 @@ var deleteCmd = cli.Command{
 			},
 		},
 	}
+}
+
+func (opts *deleteOptions) run(c *cli.Context) error {
+	if len(c.Args()) != 1 {
+		return errors.New("Usage: delete imageReference")
+	}
+
+	ref, err := alltransports.ParseImageName(c.Args()[0])
+	if err != nil {
+		return fmt.Errorf("Invalid source name %s: %v", c.Args()[0], err)
+	}
+
+	sys, err := contextFromGlobalOptions(c, "")
+	if err != nil {
+		return err
+	}
+
+	ctx, cancel := commandTimeoutContextFromGlobalOptions(c)
+	defer cancel()
+	return ref.DeleteImage(ctx, sys)
+}

cmd/skopeo/inspect.go

@@ -29,7 +29,12 @@ type inspectOutput struct {
 	Layers        []string
 }
 
-var inspectCmd = cli.Command{
+type inspectOptions struct {
+}
+
+func inspectCmd() cli.Command {
+	opts := inspectOptions{}
+	return cli.Command{
 		Name:  "inspect",
 		Usage: "Inspect image IMAGE-NAME",
 		Description: fmt.Sprintf(`
@@ -65,7 +70,11 @@ var inspectCmd = cli.Command{
 				Usage: "Use `USERNAME[:PASSWORD]` for accessing the registry",
 			},
 		},
-	Action: func(c *cli.Context) (retErr error) {
+		Action: opts.run,
+	}
+}
+
+func (opts *inspectOptions) run(c *cli.Context) (retErr error) {
 	ctx, cancel := commandTimeoutContextFromGlobalOptions(c)
 	defer cancel()
 
@@ -136,5 +145,4 @@ var inspectCmd = cli.Command{
 	}
 	fmt.Fprintln(c.App.Writer, string(out))
 	return nil
-	},
 }

cmd/skopeo/layers.go

@@ -15,12 +15,21 @@ import (
 	"github.com/urfave/cli"
 )
 
-var layersCmd = cli.Command{
+type layersOptions struct {
+}
+
+func layersCmd() cli.Command {
+	opts := &layersOptions{}
+	return cli.Command{
 		Name:      "layers",
 		Usage:     "Get layers of IMAGE-NAME",
 		ArgsUsage: "IMAGE-NAME [LAYER...]",
 		Hidden:    true,
-	Action: func(c *cli.Context) (retErr error) {
+		Action:    opts.run,
+	}
+}
+
+func (opts *layersOptions) run(c *cli.Context) (retErr error) {
 	fmt.Fprintln(os.Stderr, `DEPRECATED: skopeo layers is deprecated in favor of skopeo copy`)
 	if c.NArg() == 0 {
 		return errors.New("Usage: layers imageReference [layer...]")
@@ -124,5 +133,4 @@ var layersCmd = cli.Command{
 	}
 
 	return dest.Commit(ctx)
-	},
 }

cmd/skopeo/main.go

@@ -75,14 +75,14 @@ func createApp() *cli.App {
 		return nil
 	}
 	app.Commands = []cli.Command{
-		copyCmd,
+		copyCmd(),
-		inspectCmd,
+		inspectCmd(),
-		layersCmd,
+		layersCmd(),
-		deleteCmd,
+		deleteCmd(),
-		manifestDigestCmd,
+		manifestDigestCmd(),
-		standaloneSignCmd,
+		standaloneSignCmd(),
-		standaloneVerifyCmd,
+		standaloneVerifyCmd(),
-		untrustedSignatureDumpCmd,
+		untrustedSignatureDumpCmd(),
 	}
 	return app
 }

cmd/skopeo/manifest.go

@@ -9,7 +9,20 @@ import (
 	"github.com/urfave/cli"
 )
 
-func manifestDigest(context *cli.Context) error {
+type manifestDigestOptions struct {
+}
+
+func manifestDigestCmd() cli.Command {
+	opts := manifestDigestOptions{}
+	return cli.Command{
+		Name:      "manifest-digest",
+		Usage:     "Compute a manifest digest of a file",
+		ArgsUsage: "MANIFEST",
+		Action:    opts.run,
+	}
+}
+
+func (opts *manifestDigestOptions) run(context *cli.Context) error {
 	if len(context.Args()) != 1 {
 		return errors.New("Usage: skopeo manifest-digest manifest")
 	}
@@ -26,10 +39,3 @@ func manifestDigest(context *cli.Context) error {
 	fmt.Fprintf(context.App.Writer, "%s\n", digest)
 	return nil
 }
-
-var manifestDigestCmd = cli.Command{
-	Name:      "manifest-digest",
-	Usage:     "Compute a manifest digest of a file",
-	ArgsUsage: "MANIFEST",
-	Action:    manifestDigest,
-}

cmd/skopeo/signing.go

@@ -10,7 +10,26 @@ import (
 	"github.com/urfave/cli"
 )
 
-func standaloneSign(c *cli.Context) error {
+type standaloneSignOptions struct {
+}
+
+func standaloneSignCmd() cli.Command {
+	opts := standaloneSignOptions{}
+	return cli.Command{
+		Name:      "standalone-sign",
+		Usage:     "Create a signature using local files",
+		ArgsUsage: "MANIFEST DOCKER-REFERENCE KEY-FINGERPRINT",
+		Action:    opts.run,
+		Flags: []cli.Flag{
+			cli.StringFlag{
+				Name:  "output, o",
+				Usage: "output the signature to `SIGNATURE`",
+			},
+		},
+	}
+}
+
+func (opts *standaloneSignOptions) run(c *cli.Context) error {
 	outputFile := c.String("output")
 	if len(c.Args()) != 3 || outputFile == "" {
 		return errors.New("Usage: skopeo standalone-sign manifest docker-reference key-fingerprint -o signature")
@@ -40,20 +59,20 @@ func standaloneSign(c *cli.Context) error {
 	return nil
 }
 
-var standaloneSignCmd = cli.Command{
+type standaloneVerifyOptions struct {
-	Name:      "standalone-sign",
-	Usage:     "Create a signature using local files",
-	ArgsUsage: "MANIFEST DOCKER-REFERENCE KEY-FINGERPRINT",
-	Action:    standaloneSign,
-	Flags: []cli.Flag{
-		cli.StringFlag{
-			Name:  "output, o",
-			Usage: "output the signature to `SIGNATURE`",
-		},
-	},
 }
 
-func standaloneVerify(c *cli.Context) error {
+func standaloneVerifyCmd() cli.Command {
+	opts := standaloneVerifyOptions{}
+	return cli.Command{
+		Name:      "standalone-verify",
+		Usage:     "Verify a signature using local files",
+		ArgsUsage: "MANIFEST DOCKER-REFERENCE KEY-FINGERPRINT SIGNATURE",
+		Action:    opts.run,
+	}
+}
+
+func (opts *standaloneVerifyOptions) run(c *cli.Context) error {
 	if len(c.Args()) != 4 {
 		return errors.New("Usage: skopeo standalone-verify manifest docker-reference key-fingerprint signature")
 	}
@@ -85,14 +104,27 @@ func standaloneVerify(c *cli.Context) error {
 	return nil
 }
 
-var standaloneVerifyCmd = cli.Command{
+// WARNING: Do not use the contents of this for ANY security decisions,
-	Name:      "standalone-verify",
+// and be VERY CAREFUL about showing this information to humans in any way which suggest that these values “are probably” reliable.
-	Usage:     "Verify a signature using local files",
+// There is NO REASON to expect the values to be correct, or not intentionally misleading
-	ArgsUsage: "MANIFEST DOCKER-REFERENCE KEY-FINGERPRINT SIGNATURE",
+// (including things like “✅ Verified by $authority”)
-	Action:    standaloneVerify,
+//
+// The subcommand is undocumented, and it may be renamed or entirely disappear in the future.
+type untrustedSignatureDumpOptions struct {
 }
 
-func untrustedSignatureDump(c *cli.Context) error {
+func untrustedSignatureDumpCmd() cli.Command {
+	opts := untrustedSignatureDumpOptions{}
+	return cli.Command{
+		Name:      "untrusted-signature-dump-without-verification",
+		Usage:     "Dump contents of a signature WITHOUT VERIFYING IT",
+		ArgsUsage: "SIGNATURE",
+		Hidden:    true,
+		Action:    opts.run,
+	}
+}
+
+func (opts *untrustedSignatureDumpOptions) run(c *cli.Context) error {
 	if len(c.Args()) != 1 {
 		return errors.New("Usage: skopeo untrusted-signature-dump-without-verification signature")
 	}
@@ -114,17 +146,3 @@ func untrustedSignatureDump(c *cli.Context) error {
 	fmt.Fprintln(c.App.Writer, string(untrustedOut))
 	return nil
 }
-
-// WARNING: Do not use the contents of this for ANY security decisions,
-// and be VERY CAREFUL about showing this information to humans in any way which suggest that these values “are probably” reliable.
-// There is NO REASON to expect the values to be correct, or not intentionally misleading
-// (including things like “✅ Verified by $authority”)
-//
-// The subcommand is undocumented, and it may be renamed or entirely disappear in the future.
-var untrustedSignatureDumpCmd = cli.Command{
-	Name:      "untrusted-signature-dump-without-verification",
-	Usage:     "Dump contents of a signature WITHOUT VERIFYING IT",
-	ArgsUsage: "SIGNATURE",
-	Hidden:    true,
-	Action:    untrustedSignatureDump,
-}