From c769c7789eb9aeb3575a1413876e584d9089f279 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 16 Jul 2018 22:57:02 +0200 Subject: [PATCH] Introduce imageOptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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č --- cmd/skopeo/copy.go | 24 +++++++++++------ cmd/skopeo/delete.go | 13 +++++++--- cmd/skopeo/inspect.go | 15 +++++++---- cmd/skopeo/layers.go | 12 ++++++--- cmd/skopeo/utils.go | 32 ++++++++++++++++------- cmd/skopeo/utils_test.go | 56 +++++++++++++++++++++++++++------------- 6 files changed, 104 insertions(+), 48 deletions(-) diff --git a/cmd/skopeo/copy.go b/cmd/skopeo/copy.go index 81f6f35d..fdff7c1b 100644 --- a/cmd/skopeo/copy.go +++ b/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 } diff --git a/cmd/skopeo/delete.go b/cmd/skopeo/delete.go index e6e173f9..5a9de127 100644 --- a/cmd/skopeo/delete.go +++ b/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 } diff --git a/cmd/skopeo/inspect.go b/cmd/skopeo/inspect.go index 15a38ccf..d35b4a75 100644 --- a/cmd/skopeo/inspect.go +++ b/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 } diff --git a/cmd/skopeo/layers.go b/cmd/skopeo/layers.go index f503af6e..da04b625 100644 --- a/cmd/skopeo/layers.go +++ b/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 } diff --git a/cmd/skopeo/utils.go b/cmd/skopeo/utils.go index d6fa5375..79b0a182 100644 --- a/cmd/skopeo/utils.go +++ b/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 } diff --git a/cmd/skopeo/utils_test.go b/cmd/skopeo/utils_test.go index 04b49913..44e61d93 100644 --- a/cmd/skopeo/utils_test.go +++ b/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) }