Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Export stack commands #1519

Merged
merged 1 commit into from
Jan 23, 2019
Merged

Export stack commands #1519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions cli/command/stack/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ type commonOptions struct {
orchestrator command.Orchestrator
}

func (o *commonOptions) Orchestrator() command.Orchestrator {
if o == nil {
return command.OrchestratorSwarm
}
return o.orchestrator
}

// NewStackCommand returns a cobra command for `stack` subcommands
func NewStackCommand(dockerCli command.Cli) *cobra.Command {
var opts commonOptions
Expand Down
19 changes: 19 additions & 0 deletions cli/command/stack/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import (
"fmt"
"strings"
"unicode"

"github.com/docker/cli/cli/command"
"github.com/docker/cli/cli/command/stack/kubernetes"
"github.com/spf13/pflag"
)

// validateStackName checks if the provided string is a valid stack name (namespace).
Expand All @@ -29,3 +33,18 @@ func validateStackNames(namespaces []string) error {
func quotesOrWhitespace(r rune) bool {
return unicode.IsSpace(r) || r == '"' || r == '\''
}

func runOrchestratedCommand(dockerCli command.Cli, flags *pflag.FlagSet, commonOrchestrator command.Orchestrator, swarmCmd func() error, kubernetesCmd func(*kubernetes.KubeCli) error) 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 kubernetesCmd(kli)
default:
return swarmCmd()
}
}
17 changes: 4 additions & 13 deletions cli/command/stack/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func newDeployCommand(dockerCli command.Cli, common *commonOptions) *cobra.Comma
if err != nil {
return err
}
return RunDeploy(dockerCli, cmd.Flags(), config, commonOrchestrator, opts)
return RunDeploy(dockerCli, cmd.Flags(), config, common.Orchestrator(), opts)
},
}

Expand All @@ -75,16 +75,7 @@ func newDeployCommand(dockerCli command.Cli, common *commonOptions) *cobra.Comma

// 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 errors.Wrap(err, "unable to deploy to Kubernetes")
}
return kubernetes.RunDeploy(kli, opts, config)
default:
return swarm.RunDeploy(dockerCli, opts, config)
}
return runOrchestratedCommand(dockerCli, flags, commonOrchestrator,
func() error { return swarm.RunDeploy(dockerCli, opts, config) },
func(kli *kubernetes.KubeCli) error { return kubernetes.RunDeploy(kli, opts, config) })
}
5 changes: 3 additions & 2 deletions cli/command/stack/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func newListCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command
Short: "List stacks",
Args: cli.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
return runList(cmd, dockerCli, opts, common.orchestrator)
return RunList(cmd, dockerCli, opts, common.orchestrator)
},
}

Expand All @@ -35,7 +35,8 @@ func newListCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command
return cmd
}

func runList(cmd *cobra.Command, dockerCli command.Cli, opts options.List, orchestrator command.Orchestrator) error {
// RunList performs a stack list against the specified orchestrator
func RunList(cmd *cobra.Command, dockerCli command.Cli, opts options.List, orchestrator command.Orchestrator) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only concern I have with exporting all of these is that;

  • These functions now become a public "API", with no interface defined for them
    • this can complicate making changes in the CLI (they were kept non-exported, as their signature/interface was not "stable")
  • Some of these functions actually have a "UX" component built-in (e.g. they print confirmation messages on stdout/stderr, have an option to print formatted output); is that really suitable for external consumption?
    • calling these commands would become almost equivalent to exec.Cmd("docker stack ls")

For the underlying code on the other hand, we have various interfaces defined; if those are lacking functionality that's needed, should we review those interfaces, and make that the API to use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the underlying code on the other hand, we have various interfaces defined; if those are lacking functionality that's needed, should we review those interfaces, and make that the API to use?

I guess you can see these functions as a Facade, a simple API endpoint for a simple use, but if you have a more advanced use case, you can still rely on the underlying interfaces (like if you only want to talk to Swarm and don't care about Kubernetes). Otherwise if you want the same behavior, you need to replicate this complicated switch kubernetes/swarm.

Some of these functions actually have a "UX" component built-in (e.g. they print confirmation messages on stdout/stderr, have an option to print formatted output); is that really suitable for external consumption?
calling these commands would become almost equivalent to exec.Cmd("docker stack ls")

That is exactly the purpose of this PR: someone who vendors the docker/cli can benefit the same look-and-feel (and behavior) as the "real" docker-cli binary, without relying on the binary itself.

These functions now become a public "API", with no interface defined for them
this can complicate making changes in the CLI (they were kept non-exported, as their signature/interface was not "stable")

I think these signatures are simple and somehow stable. I don't have a crystal ball but my 2 cts is that they won't be modified before a long time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I do agree with @thaJeztah and also @silvin-lubecki 😅 🙃

I think our client API is too low-level, and we need to export those because of that 😅
We may define better APIs in the future (that would not have cobra.Command or comand.Cli in the method signature) for example.

I think it's the less worse we can do for now 👼 so, still LGTM 👼

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was discussing with @chris-crone to add a note somewhere (perhaps the readme) to indicate that these functions, even though they're exported, should not be considered a stable API (as in; their signature can change, and shouldn't be relied on)

We could also add notes to the GoDoc of these, similar to https://golang.org/pkg/testing/#RunTests

An internal function but exported because it is cross-package; part of the implementation of the "go test" command.

But that would be a bit more work to do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was discussing with @chris-crone to add a note somewhere (perhaps the readme) to indicate that these functions, even though they're exported, should not be considered a stable API (as in; their signature can change, and shouldn't be relied on)

Will do that in a follow-up 👍

stacks := []*formatter.Stack{}
if orchestrator.HasSwarm() {
ss, err := swarm.GetStacks(dockerCli)
Expand Down
22 changes: 9 additions & 13 deletions cli/command/stack/ps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/docker/cli/cli/command/stack/swarm"
cliopts "github.com/docker/cli/opts"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)

func newPsCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command {
Expand All @@ -22,19 +23,7 @@ func newPsCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command {
if err := validateStackName(opts.Namespace); err != nil {
return err
}

switch {
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.RunPS(kli, opts)
default:
return swarm.RunPS(dockerCli, opts)
}
return RunPs(dockerCli, cmd.Flags(), common.Orchestrator(), opts)
},
}
flags := cmd.Flags()
Expand All @@ -46,3 +35,10 @@ func newPsCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command {
kubernetes.AddNamespaceFlag(flags)
return cmd
}

// RunPs performs a stack ps against the specified orchestrator
func RunPs(dockerCli command.Cli, flags *pflag.FlagSet, commonOrchestrator command.Orchestrator, opts options.PS) error {
return runOrchestratedCommand(dockerCli, flags, commonOrchestrator,
func() error { return swarm.RunPS(dockerCli, opts) },
func(kli *kubernetes.KubeCli) error { return kubernetes.RunPS(kli, opts) })
}
22 changes: 9 additions & 13 deletions cli/command/stack/remove.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/docker/cli/cli/command/stack/options"
"github.com/docker/cli/cli/command/stack/swarm"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)

func newRemoveCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command {
Expand All @@ -22,22 +23,17 @@ func newRemoveCommand(dockerCli command.Cli, common *commonOptions) *cobra.Comma
if err := validateStackNames(opts.Namespaces); err != nil {
return err
}

switch {
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.RunRemove(kli, opts)
default:
return swarm.RunRemove(dockerCli, opts)
}
return RunRemove(dockerCli, cmd.Flags(), common.Orchestrator(), opts)
},
}
flags := cmd.Flags()
kubernetes.AddNamespaceFlag(flags)
return cmd
}

// RunRemove performs a stack remove against the specified orchestrator
func RunRemove(dockerCli command.Cli, flags *pflag.FlagSet, commonOrchestrator command.Orchestrator, opts options.Remove) error {
return runOrchestratedCommand(dockerCli, flags, commonOrchestrator,
func() error { return swarm.RunRemove(dockerCli, opts) },
func(kli *kubernetes.KubeCli) error { return kubernetes.RunRemove(kli, opts) })
}
22 changes: 9 additions & 13 deletions cli/command/stack/services.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/docker/cli/cli/command/stack/swarm"
cliopts "github.com/docker/cli/opts"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)

func newServicesCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command {
Expand All @@ -22,19 +23,7 @@ func newServicesCommand(dockerCli command.Cli, common *commonOptions) *cobra.Com
if err := validateStackName(opts.Namespace); err != nil {
return err
}

switch {
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.RunServices(kli, opts)
default:
return swarm.RunServices(dockerCli, opts)
}
return RunServices(dockerCli, cmd.Flags(), common.Orchestrator(), opts)
},
}
flags := cmd.Flags()
Expand All @@ -44,3 +33,10 @@ func newServicesCommand(dockerCli command.Cli, common *commonOptions) *cobra.Com
kubernetes.AddNamespaceFlag(flags)
return cmd
}

// RunServices performs a stack services against the specified orchestrator
func RunServices(dockerCli command.Cli, flags *pflag.FlagSet, commonOrchestrator command.Orchestrator, opts options.Services) error {
return runOrchestratedCommand(dockerCli, flags, commonOrchestrator,
func() error { return swarm.RunServices(dockerCli, opts) },
func(kli *kubernetes.KubeCli) error { return kubernetes.RunServices(kli, opts) })
}