Refactor `stack` command/package

- Handle `bundlefile` directly in the `top-level`
  command. `bundlefile` is still experimental and will be deprecated
  in future version — this should make be easier to remove it.
- Validate the `stack` name in all cases (i.e. whatever the
  orchestrator is used)
- Load the composefile ahead of choosing the orchestrator. This
  removes some slight duplication.
- Makes `RunDeploy` easier to use from outside packages (like
  `docker/app`) with a preloaded configuration.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

cli/command/stack/common.go

@@ -0,0 +1,31 @@
+package stack
+
+import (
+	"fmt"
+	"strings"
+	"unicode"
+)
+
+// validateStackName checks if the provided string is a valid stack name (namespace).
+// It currently only does a rudimentary check if the string is empty, or consists
+// of only whitespace and quoting characters.
+func validateStackName(namespace string) error {
+	v := strings.TrimFunc(namespace, quotesOrWhitespace)
+	if v == "" {
+		return fmt.Errorf("invalid stack name: %q", namespace)
+	}
+	return nil
+}
+
+func validateStackNames(namespaces []string) error {
+	for _, ns := range namespaces {
+		if err := validateStackName(ns); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func quotesOrWhitespace(r rune) bool {
+	return unicode.IsSpace(r) || r == '"' || r == '\''
+}

cli/command/stack/deploy.go

@@ -1,12 +1,18 @@
 package stack
 
 import (
+	"context"
+
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/command/stack/kubernetes"
+	"github.com/docker/cli/cli/command/stack/loader"
 	"github.com/docker/cli/cli/command/stack/options"
 	"github.com/docker/cli/cli/command/stack/swarm"
+	composetypes "github.com/docker/cli/cli/compose/types"
+	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 )
 
 func newDeployCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command {
@@ -19,20 +25,32 @@ func newDeployCommand(dockerCli command.Cli, common *commonOptions) *cobra.Comma
 		Args:    cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			opts.Namespace = args[0]
-			switch {
-			case common == nil: // Top level deploy commad
-				return swarm.RunDeploy(dockerCli, opts)
-			case common.orchestrator.HasAll():
-				return errUnsupportedAllOrchestrator
-			case common.orchestrator.HasKubernetes():
-				kli, err := kubernetes.WrapCli(dockerCli, kubernetes.NewOptions(cmd.Flags(), common.orchestrator))
-				if err != nil {
-					return err
-				}
-				return kubernetes.RunDeploy(kli, opts)
-			default:
-				return swarm.RunDeploy(dockerCli, opts)
+			if err := validateStackName(opts.Namespace); err != nil {
+				return err
 			}
+
+			commonOrchestrator := command.OrchestratorSwarm // default for top-level deploy command
+			if common != nil {
+				commonOrchestrator = common.orchestrator
+			}
+
+			switch {
+			case opts.Bundlefile == "" && len(opts.Composefiles) == 0:
+				return errors.Errorf("Please specify either a bundle file (with --bundle-file) or a Compose file (with --compose-file).")
+			case opts.Bundlefile != "" && len(opts.Composefiles) != 0:
+				return errors.Errorf("You cannot specify both a bundle file and a Compose file.")
+			case opts.Bundlefile != "":
+				if commonOrchestrator != command.OrchestratorSwarm {
+					return errors.Errorf("bundle files are not supported on another orchestrator than swarm.")
+				}
+				return swarm.DeployBundle(context.Background(), dockerCli, opts)
+			}
+
+			config, err := loader.LoadComposefile(dockerCli, opts)
+			if err != nil {
+				return err
+			}
+			return RunDeploy(dockerCli, cmd.Flags(), config, commonOrchestrator, opts)
 		},
 	}
 
@@ -54,3 +72,19 @@ func newDeployCommand(dockerCli command.Cli, common *commonOptions) *cobra.Comma
 	kubernetes.AddNamespaceFlag(flags)
 	return cmd
 }
+
+// RunDeploy performs a stack deploy against the specified orchestrator
+func RunDeploy(dockerCli command.Cli, flags *pflag.FlagSet, config *composetypes.Config, commonOrchestrator command.Orchestrator, opts options.Deploy) error {
+	switch {
+	case commonOrchestrator.HasAll():
+		return errUnsupportedAllOrchestrator
+	case commonOrchestrator.HasKubernetes():
+		kli, err := kubernetes.WrapCli(dockerCli, kubernetes.NewOptions(flags, commonOrchestrator))
+		if err != nil {
+			return err
+		}
+		return kubernetes.RunDeploy(kli, opts, config)
+	default:
+		return swarm.RunDeploy(dockerCli, opts, config)
+	}
+}

cli/command/stack/deploy_test.go

@@ -0,0 +1,17 @@
+package stack
+
+import (
+	"io/ioutil"
+	"testing"
+
+	"github.com/docker/cli/internal/test"
+	"gotest.tools/assert"
+)
+
+func TestDeployWithEmptyName(t *testing.T) {
+	cmd := newDeployCommand(test.NewFakeCli(&fakeClient{}), nil)
+	cmd.SetArgs([]string{"'   '"})
+	cmd.SetOutput(ioutil.Discard)
+
+	assert.ErrorContains(t, cmd.Execute(), `invalid stack name: "'   '"`)
+}

cli/command/stack/kubernetes/deploy.go

@@ -5,14 +5,14 @@ import (
 	"io"
 
 	"github.com/docker/cli/cli/command"
-	"github.com/docker/cli/cli/command/stack/loader"
 	"github.com/docker/cli/cli/command/stack/options"
+	composetypes "github.com/docker/cli/cli/compose/types"
 	"github.com/morikuni/aec"
 	"github.com/pkg/errors"
 )
 
 // RunDeploy is the kubernetes implementation of docker stack deploy
-func RunDeploy(dockerCli *KubeCli, opts options.Deploy) error {
+func RunDeploy(dockerCli *KubeCli, opts options.Deploy, cfg *composetypes.Config) error {
 	cmdOut := dockerCli.Out()
 	// Check arguments
 	if len(opts.Composefiles) == 0 {
@@ -29,11 +29,6 @@ func RunDeploy(dockerCli *KubeCli, opts options.Deploy) error {
 		return err
 	}
 
-	// Parse the compose file
-	cfg, err := loader.LoadComposefile(dockerCli, opts)
-	if err != nil {
-		return err
-	}
 	stack, err := stacks.FromCompose(dockerCli.Err(), opts.Namespace, cfg)
 	if err != nil {
 		return err

cli/command/stack/ps.go

@@ -19,6 +19,10 @@ func newPsCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command {
 		Args:  cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			opts.Namespace = args[0]
+			if err := validateStackName(opts.Namespace); err != nil {
+				return err
+			}
+
 			switch {
 			case common.orchestrator.HasAll():
 				return errUnsupportedAllOrchestrator

cli/command/stack/ps_test.go

@@ -51,6 +51,14 @@ func TestStackPsErrors(t *testing.T) {
 	}
 }
 
+func TestRunPSWithEmptyName(t *testing.T) {
+	cmd := newPsCommand(test.NewFakeCli(&fakeClient{}), &orchestrator)
+	cmd.SetArgs([]string{"'   '"})
+	cmd.SetOutput(ioutil.Discard)
+
+	assert.ErrorContains(t, cmd.Execute(), `invalid stack name: "'   '"`)
+}
+
 func TestStackPsEmptyStack(t *testing.T) {
 	fakeCli := test.NewFakeCli(&fakeClient{
 		taskListFunc: func(options types.TaskListOptions) ([]swarm.Task, error) {

cli/command/stack/remove.go

@@ -19,6 +19,10 @@ func newRemoveCommand(dockerCli command.Cli, common *commonOptions) *cobra.Comma
 		Args:    cli.RequiresMinArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			opts.Namespaces = args
+			if err := validateStackNames(opts.Namespaces); err != nil {
+				return err
+			}
+
 			switch {
 			case common.orchestrator.HasAll():
 				return errUnsupportedAllOrchestrator

cli/command/stack/remove_test.go

@@ -41,6 +41,14 @@ func fakeClientForRemoveStackTest(version string) *fakeClient {
 	}
 }
 
+func TestRemoveWithEmptyName(t *testing.T) {
+	cmd := newRemoveCommand(test.NewFakeCli(&fakeClient{}), &orchestrator)
+	cmd.SetArgs([]string{"good", "'   '", "alsogood"})
+	cmd.SetOutput(ioutil.Discard)
+
+	assert.ErrorContains(t, cmd.Execute(), `invalid stack name: "'   '"`)
+}
+
 func TestRemoveStackVersion124DoesNotRemoveConfigsOrSecrets(t *testing.T) {
 	client := fakeClientForRemoveStackTest("1.24")
 	cmd := newRemoveCommand(test.NewFakeCli(client), &orchestrator)

cli/command/stack/services.go

@@ -19,6 +19,10 @@ func newServicesCommand(dockerCli command.Cli, common *commonOptions) *cobra.Com
 		Args:  cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			opts.Namespace = args[0]
+			if err := validateStackName(opts.Namespace); err != nil {
+				return err
+			}
+
 			switch {
 			case common.orchestrator.HasAll():
 				return errUnsupportedAllOrchestrator

cli/command/stack/services_test.go

@@ -80,6 +80,14 @@ func TestStackServicesErrors(t *testing.T) {
 	}
 }
 
+func TestRunServicesWithEmptyName(t *testing.T) {
+	cmd := newServicesCommand(test.NewFakeCli(&fakeClient{}), &orchestrator)
+	cmd.SetArgs([]string{"'   '"})
+	cmd.SetOutput(ioutil.Discard)
+
+	assert.ErrorContains(t, cmd.Execute(), `invalid stack name: "'   '"`)
+}
+
 func TestStackServicesEmptyServiceList(t *testing.T) {
 	fakeCli := test.NewFakeCli(&fakeClient{
 		serviceListFunc: func(options types.ServiceListOptions) ([]swarm.Service, error) {

cli/command/stack/swarm/common.go

@@ -2,9 +2,6 @@ package swarm
 
 import (
 	"context"
-	"fmt"
-	"strings"
-	"unicode"
 
 	"github.com/docker/cli/cli/compose/convert"
 	"github.com/docker/cli/opts"
@@ -51,28 +48,3 @@ func getStackSecrets(ctx context.Context, apiclient client.APIClient, namespace
 func getStackConfigs(ctx context.Context, apiclient client.APIClient, namespace string) ([]swarm.Config, error) {
 	return apiclient.ConfigList(ctx, types.ConfigListOptions{Filters: getStackFilter(namespace)})
 }
-
-// validateStackName checks if the provided string is a valid stack name (namespace).
-//
-// It currently only does a rudimentary check if the string is empty, or consists
-// of only whitespace and quoting characters.
-func validateStackName(namespace string) error {
-	v := strings.TrimFunc(namespace, quotesOrWhitespace)
-	if len(v) == 0 {
-		return fmt.Errorf("invalid stack name: %q", namespace)
-	}
-	return nil
-}
-
-func validateStackNames(namespaces []string) error {
-	for _, ns := range namespaces {
-		if err := validateStackName(ns); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func quotesOrWhitespace(r rune) bool {
-	return unicode.IsSpace(r) || r == '"' || r == '\''
-}

cli/command/stack/swarm/deploy.go

@@ -7,6 +7,7 @@ import (
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/command/stack/options"
 	"github.com/docker/cli/cli/compose/convert"
+	composetypes "github.com/docker/cli/cli/compose/types"
 	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/api/types/versions"
 	"github.com/pkg/errors"
@@ -21,26 +22,14 @@ const (
 )
 
 // RunDeploy is the swarm implementation of docker stack deploy
-func RunDeploy(dockerCli command.Cli, opts options.Deploy) error {
+func RunDeploy(dockerCli command.Cli, opts options.Deploy, cfg *composetypes.Config) error {
 	ctx := context.Background()
 
-	if err := validateStackName(opts.Namespace); err != nil {
-		return err
-	}
 	if err := validateResolveImageFlag(dockerCli, &opts); err != nil {
 		return err
 	}
 
-	switch {
-	case opts.Bundlefile == "" && len(opts.Composefiles) == 0:
-		return errors.Errorf("Please specify either a bundle file (with --bundle-file) or a Compose file (with --compose-file).")
-	case opts.Bundlefile != "" && len(opts.Composefiles) != 0:
-		return errors.Errorf("You cannot specify both a bundle file and a Compose file.")
-	case opts.Bundlefile != "":
-		return deployBundle(ctx, dockerCli, opts)
-	default:
-		return deployCompose(ctx, dockerCli, opts)
-	}
+	return deployCompose(ctx, dockerCli, opts, cfg)
 }
 
 // validateResolveImageFlag validates the opts.resolveImage command line option

cli/command/stack/swarm/deploy_bundlefile.go

@@ -15,10 +15,8 @@ import (
 	"github.com/pkg/errors"
 )
 
-func deployBundle(ctx context.Context, dockerCli command.Cli, opts options.Deploy) error {
-	if err := validateStackName(opts.Namespace); err != nil {
-		return err
-	}
+// DeployBundle deploy a bundlefile (dab) on a swarm.
+func DeployBundle(ctx context.Context, dockerCli command.Cli, opts options.Deploy) error {
 	bundle, err := loadBundlefile(dockerCli.Err(), opts.Namespace, opts.Bundlefile)
 	if err != nil {
 		return err

cli/command/stack/swarm/deploy_composefile.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 
 	"github.com/docker/cli/cli/command"
-	"github.com/docker/cli/cli/command/stack/loader"
 	"github.com/docker/cli/cli/command/stack/options"
 	"github.com/docker/cli/cli/compose/convert"
 	composetypes "github.com/docker/cli/cli/compose/types"
@@ -17,15 +16,7 @@ import (
 	"github.com/pkg/errors"
 )
 
-func deployCompose(ctx context.Context, dockerCli command.Cli, opts options.Deploy) error {
-	if err := validateStackName(opts.Namespace); err != nil {
-		return err
-	}
-	config, err := loader.LoadComposefile(dockerCli, opts)
-	if err != nil {
-		return err
-	}
-
+func deployCompose(ctx context.Context, dockerCli command.Cli, opts options.Deploy, config *composetypes.Config) error {
 	if err := checkDaemonIsSwarmManager(ctx, dockerCli); err != nil {
 		return err
 	}

cli/command/stack/swarm/deploy_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"testing"
 
-	"github.com/docker/cli/cli/command/stack/options"
 	"github.com/docker/cli/cli/compose/convert"
 	"github.com/docker/cli/internal/test"
 	"github.com/docker/docker/api/types"
@@ -27,15 +26,6 @@ func TestPruneServices(t *testing.T) {
 	assert.Check(t, is.DeepEqual(buildObjectIDs([]string{objectName("foo", "remove")}), client.removedServices))
 }
 
-func TestDeployWithEmptyName(t *testing.T) {
-	ctx := context.Background()
-	client := &fakeClient{}
-	dockerCli := test.NewFakeCli(client)
-
-	err := deployCompose(ctx, dockerCli, options.Deploy{Namespace: "'   '", Prune: true})
-	assert.Check(t, is.Error(err, `invalid stack name: "'   '"`))
-}
-
 // TestServiceUpdateResolveImageChanged tests that the service's
 // image digest, and "ForceUpdate" is preserved if the image did not change in
 // the compose file

cli/command/stack/swarm/ps.go

@@ -13,10 +13,6 @@ import (
 
 // RunPS is the swarm implementation of docker stack ps
 func RunPS(dockerCli command.Cli, opts options.PS) error {
-	if err := validateStackName(opts.Namespace); err != nil {
-		return err
-	}
-
 	filter := getStackFilterFromOpt(opts.Namespace, opts.Filter)
 
 	ctx := context.Background()

cli/command/stack/swarm/ps_test.go

@@ -1,18 +0,0 @@
-package swarm
-
-import (
-	"testing"
-
-	"github.com/docker/cli/cli/command/stack/options"
-	"github.com/docker/cli/internal/test"
-	"gotest.tools/assert"
-	is "gotest.tools/assert/cmp"
-)
-
-func TestRunPSWithEmptyName(t *testing.T) {
-	client := &fakeClient{}
-	dockerCli := test.NewFakeCli(client)
-
-	err := RunPS(dockerCli, options.PS{Namespace: "'   '"})
-	assert.Check(t, is.Error(err, `invalid stack name: "'   '"`))
-}

cli/command/stack/swarm/remove.go

@@ -16,10 +16,6 @@ import (
 
 // RunRemove is the swarm implementation of docker stack remove
 func RunRemove(dockerCli command.Cli, opts options.Remove) error {
-	if err := validateStackNames(opts.Namespaces); err != nil {
-		return err
-	}
-
 	client := dockerCli.Client()
 	ctx := context.Background()
 

cli/command/stack/swarm/remove_test.go

@@ -1,18 +0,0 @@
-package swarm
-
-import (
-	"testing"
-
-	"github.com/docker/cli/cli/command/stack/options"
-	"github.com/docker/cli/internal/test"
-	"gotest.tools/assert"
-	is "gotest.tools/assert/cmp"
-)
-
-func TestRunRemoveWithEmptyName(t *testing.T) {
-	client := &fakeClient{}
-	dockerCli := test.NewFakeCli(client)
-
-	err := RunRemove(dockerCli, options.Remove{Namespaces: []string{"good", "'   '", "alsogood"}})
-	assert.Check(t, is.Error(err, `invalid stack name: "'   '"`))
-}

cli/command/stack/swarm/services.go

@@ -14,9 +14,6 @@ import (
 
 // RunServices is the swarm implementation of docker stack services
 func RunServices(dockerCli command.Cli, opts options.Services) error {
-	if err := validateStackName(opts.Namespace); err != nil {
-		return err
-	}
 	ctx := context.Background()
 	client := dockerCli.Client()
 

cli/command/stack/swarm/services_test.go

@@ -1,18 +0,0 @@
-package swarm
-
-import (
-	"testing"
-
-	"github.com/docker/cli/cli/command/stack/options"
-	"github.com/docker/cli/internal/test"
-	"gotest.tools/assert"
-	is "gotest.tools/assert/cmp"
-)
-
-func TestRunServicesWithEmptyName(t *testing.T) {
-	client := &fakeClient{}
-	dockerCli := test.NewFakeCli(client)
-
-	err := RunServices(dockerCli, options.Services{Namespace: "'   '"})
-	assert.Check(t, is.Error(err, `invalid stack name: "'   '"`))
-}