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.

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

Better stack list experience with Kubernetes and Swarm #1031

Merged
merged 3 commits into from May 15, 2018

Conversation

Projects
None yet
6 participants
@mat007
Contributor

mat007 commented Apr 30, 2018

- What I did

Made it easier to work with both swarm and kubernetes stacks combined, e.g.

  • added support in stack ls for the value all in the --orchestrator flag which triggers the display of both swarm and kubernetes stacks
  • allowed multiple --namespace for docker stack ls

- How I did it

  1. The stack lists are now merged after having been queried for each orchestrator and before sorting them and applying the formatting
  2. The namespace flag was moved from the stack command to all sub-commands thus allowing for the stack ls sub-command to accept several occurrences and loop over them

- How to verify it

# List both swarm and kubernetes with namespace
$ docker stack ls --orchestrator=all --namespace=tutu
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
dtc                 5                   Kubernetes          tutu
wall-e              2                   Swarm

# List stacks for both swarm and kubernetes in empty namespace
$ docker stack ls --orchestrator=all --namespace=pipo
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
wall-e              2                   Swarm

# List stacks for both swarm and kubernetes (all namespaces implied)
$ docker stack ls --orchestrator=all
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
abc                 2                   Kubernetes          molo
dtc                 5                   Kubernetes          tutu
wall-e              2                   Swarm

# List stacks for kubernetes in several namespaces
$ ./build/docker.exe stack ls --orchestrator=kubernetes --namespace=tutu,tutu,pipo,molo
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
abc                 2                   Kubernetes          molo
dtc                 5                   Kubernetes          tutu

- Description for the changelog

  • List stacks for both Swarm and Kubernetes with --orchestrator=all in docker stack ls
  • Allow several occurrences of --namespace for Kubernetes with docker stack ls

- A picture of a cute animal (not mandatory but encouraged)

image

@mat007

This comment has been minimized.

Contributor

mat007 commented Apr 30, 2018

comes after #991, only the 5 last commits are new.

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch from 3590518 to e1aaf21 Apr 30, 2018

@mat007 mat007 changed the title from docker stack ls --orchestrator=all to Better stack list experience with Kubernetes and Swarm Apr 30, 2018

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch 4 times, most recently from df0eaaf to 25a31b2 Apr 30, 2018

@@ -27,9 +27,6 @@ func NewStackCommand(dockerCli command.Cli) *cobra.Command {
newServicesCommand(dockerCli),
)
flags := cmd.PersistentFlags()
flags.String("namespace", "default", "Kubernetes namespace to use")
flags.SetAnnotation("namespace", "kubernetes", nil)
flags.SetAnnotation("namespace", "experimentalCLI", nil)

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

This should stay

This comment has been minimized.

@mat007

mat007 May 3, 2018

Contributor

This was moved into each sub-command, so still there in a way.

@@ -45,5 +51,6 @@ func newDeployCommand(dockerCli command.Cli) *cobra.Command {
`Query the registry to resolve image digest and supported platforms ("`+swarm.ResolveImageAlways+`"|"`+swarm.ResolveImageChanged+`"|"`+swarm.ResolveImageNever+`")`)
flags.SetAnnotation("resolve-image", "version", []string{"1.30"})
flags.SetAnnotation("resolve-image", "swarm", nil)
kubernetes.InstallFlags(flags)

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

Why making these flags per stack subcommand instead of putting the on the stack command and make the subcommand inherit ?

This comment has been minimized.

@mat007

mat007 May 3, 2018

Contributor

Because stack ls now needs a different behaviour than other stack sub-commands for --namespace and we cannot set it once with type string and another time with []string (e.g. several namespaces).
So the drawback is a bit of code duplication setting the flags for all sub-commands, which was factorized into kubernetes.InstallFlags

},
}
flags := cmd.Flags()
flags.StringVar(&opts.Format, "format", "", "Pretty-print stacks using a Go template")
flags.StringSliceVar(&opts.Namespaces, "namespace", []string{"default"}, "Kubernetes namespaces to use")

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

Will setting a default here will override the default namespace defined in the context ?

This comment has been minimized.

@mat007

mat007 May 3, 2018

Contributor

Hmm yes, I suppose it will, which is likely a bug.
However I don't think this has been introduced by this PR as the default for --namespace was already default.
Could this be fixed in a follow up?

This comment has been minimized.

@mat007

mat007 May 3, 2018

Contributor

Actually there is code in kubernetes.WrapCli to deal with that and it seems to be working as expected

> docker stack ls  --orchestrator=kubernetes
stacks.compose.docker.com is forbidden: User "uu" cannot list stacks.compose.docker.com in the namespace "default": access denied
> kubectl config set-context ucp_34.209.72.126:6443_uu --namespace=tutu
Context "ucp_34.209.72.126:6443_uu" modified.
> docker stack ls  --orchestrator=kubernetes
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
dtc                 5                   Kubernetes          tutu
flags.SetAnnotation("namespace", "kubernetes", nil)
flags.SetAnnotation("namespace", "experimentalCLI", nil)
flags.BoolVarP(&opts.AllNamespaces, "all-namespaces", "", false, "List stacks among all Kubernetes namespaces")
flags.SetAnnotation("all-namespaces", "kubernetes", nil)

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

all-namespaces should probably be marked as experimentalCLI too

This comment has been minimized.

@mat007

mat007 May 3, 2018

Contributor

Yes, I'll add that, thanks.

Output: dockerCli.Out(),
Format: formatter.NewStackFormat(format),
}
sort.Slice(stacks,

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

sort.Slice(stacks, func(i, j int) bool { …

This comment has been minimized.

@mat007

mat007 May 3, 2018

Contributor

OK

)}, nil
},
})
cli.SetClientInfo(func() command.ClientInfo { return command.ClientInfo{Orchestrator: "swarm"} })

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

Same here

@@ -86,6 +91,7 @@ func TestListWithoutFormat(t *testing.T) {
)}, nil
},
})
cli.SetClientInfo(func() command.ClientInfo { return command.ClientInfo{Orchestrator: "swarm"} })

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

and here …

@@ -139,6 +145,7 @@ func TestListOrder(t *testing.T) {
return uc.swarmServices, nil
},
})
cli.SetClientInfo(func() command.ClientInfo { return command.ClientInfo{Orchestrator: "swarm"} })

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

… and here

if dockerCli.ClientInfo().HasKubernetes() {
switch {
case dockerCli.ClientInfo().HasAll():
return fmt.Errorf("Please specify an orchestrator (swarm|kubernetes)")

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

We may want to share this error somehow

This comment has been minimized.

@mat007

mat007 May 3, 2018

Contributor

OK!

@@ -54,7 +54,7 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command {
flags = cmd.Flags()
flags.BoolVarP(&opts.Version, "version", "v", false, "Print version information and quit")
flags.StringVar(&opts.ConfigDir, "config", cliconfig.Dir(), "Location of client config files")
opts.Common.InstallFlags(flags)
opts.Common.InstallFlags(flags, cmd.PersistentFlags())

This comment has been minimized.

@vdemeester

vdemeester May 3, 2018

Member

I don't get this change, this feels really weird to add the flag twice (on cmd.Flags() and cmd.PersistentFlags())

This comment has been minimized.

@mat007

mat007 May 3, 2018

Contributor

"orchestrator" is the only flag added to cmd.PersistentFlags() in InstallFlags, all the other flags are still in cmd.Flags()

This comment has been minimized.

@mat007

mat007 May 3, 2018

Contributor

That being said if you would rather have me remove that last commit, I'd be fine with that!

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

Slightly confused as well

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

OK, I'll just remove the related commit.

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch from 25a31b2 to cf208bb May 3, 2018

@mat007

This comment has been minimized.

Contributor

mat007 commented May 3, 2018

@vdemeester I fixed all comments except the ones under discussion about the flags e.g.

  • persistence of --orchestrator
  • multiple --namespace implementation

Would you kindly please take another look?

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch 2 times, most recently from ae08ebe to e366a21 May 3, 2018

@thaJeztah thaJeztah changed the title from Better stack list experience with Kubernetes and Swarm to [WIP] Better stack list experience with Kubernetes and Swarm May 8, 2018

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch from e366a21 to 718ebe7 May 9, 2018

@mat007 mat007 requested a review from thaJeztah as a code owner May 9, 2018

@thaJeztah

This adds a lot of complexity to the codebase, and I'm not sure if all is needed. I left some comments inline, but must admit some of the flow is a bit hard to follow

@@ -55,7 +55,7 @@ func (commonOpts *CommonOptions) InstallFlags(flags *pflag.FlagSet) {
flags.StringVarP(&commonOpts.LogLevel, "log-level", "l", "info", `Set the logging level ("debug"|"info"|"warn"|"error"|"fatal")`)
flags.BoolVar(&commonOpts.TLS, "tls", dockerTLS, "Use TLS; implied by --tlsverify")
flags.BoolVar(&commonOpts.TLSVerify, FlagTLSVerify, dockerTLSVerify, "Use TLS and verify the remote")
flags.StringVar(&commonOpts.Orchestrator, "orchestrator", "", "Which orchestrator to use with the docker cli (swarm|kubernetes) (default swarm) (experimental)")
flags.StringVar(&commonOpts.Orchestrator, "orchestrator", "", "Which orchestrator to use with the docker cli (swarm|kubernetes|all) (default swarm) (experimental)")

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

Looks like (default swarm) no longer matches reality (for example, having "orchestrator":"kubernetes" in my ~/.docker/config.json would set the default)

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

That's a good point. I'll remove the (default swarm).

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

Also this was there before but I don't thing with the docker cli brings anything and the other flag descriptions don't have it. I'll remove it too.

},
}
flags := cmd.Flags()
flags.StringVar(&opts.Format, "format", "", "Pretty-print stacks using a Go template")
flags.BoolVarP(&opts.AllNamespaces, "all-namespaces", "", false, "List stacks among all Kubernetes namespaces")
flags.SetAnnotation("all-namespaces", "kubernetes", nil)
flags.SetAnnotation("all-namespaces", "experimentalCLI", nil)

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

Looks like this change to all-namespaces should be done in the other PR (#991)?

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

You're right, too late though as #991 has been merged in the meanwhile…

@@ -28,6 +28,9 @@ func newListCommand(dockerCli command.Cli) *cobra.Command {
flags := cmd.Flags()
flags.StringVar(&opts.Format, "format", "", "Pretty-print stacks using a Go template")
flags.StringSliceVar(&opts.Namespaces, "namespace", []string{}, "Kubernetes namespaces to use")

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

Is there a reason we use two separate approaches for the namespace flag? Looks like we define it twice now: once through AddNamespaceFlag()

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

All docker stack sub-commands use AddNamespaceFlag and have a namespace flag defined as a StringVar except ls which has a namespace flag defined as a StringSliceVar.
This is because for all sub-commands but ls only one --namespace can be set whereas ls can have several, e.g. docker stack ls --namespace=tutu,default,bla or docker stack ls --namespace=tutu --namespace=default --namespace=bla.

This comment has been minimized.

@thaJeztah

thaJeztah May 11, 2018

Member

Wondering now if this should be a StringSliceVar or a StringVar; I think StringVar can be specified multiple times (--namespace=bla --namespace=foo), but does not support comma-separated values; we haven't documented using comma-separated values, and don't think we support it for other options (possibly --security-opt only)

(note that if a command only accepts a single namespace, we could still produce an error if multiple are given)

This comment has been minimized.

@mat007

mat007 May 11, 2018

Contributor

I'll take another look but from what I remember there was no way to make it look nice, for both cases, e.g.

$ docker stack ps  --help
…
Options:
…
      --namespace string    Kubernetes namespace to use

VS

$ ./build/docker.exe stack ls --help
…
Options:
…
      --namespace strings   Kubernetes namespaces to use

Also I don't see how a single StringVar can hold multiple --namespace values…

This comment has been minimized.

@mat007

mat007 May 12, 2018

Contributor

I think StringVar can be specified multiple times (--namespace=bla --namespace=foo), but does not support comma-separated values

In that case the latest value is silently selected (here foo), I suppose the flags are simply processed in order.
If we just let comma-separated values out of the question we could indeed emulate both behaviors with a StringSliceVar by using the last element when only one namespace is required.

(Incidentally passing --namespace=bla,foo to a StringVar flag yields a value of bla,foo…)

This comment has been minimized.

@mat007

mat007 May 12, 2018

Contributor

The best I could come up with by using []string on the stack command level is

$ ./build/docker.exe stack --help

Usage:  docker stack COMMAND

Manage Docker stacks

Options:
      --kubeconfig string   Kubernetes config file
      --namespace strings   Kubernetes namespace to use

This is wrong for all sub-commands as for most of them it says strings plural whereas only one is supported (and differs for instance from kubeconfig), and for docker stack ls which supports several namespaces the description should have namespaces

I don't see how to have types and descriptions accurate for all sub-commands without duplicating the flag.

}
mnms := map[string]struct{}{}
for _, nm := range nms {
mnms[nm] = struct{}{}

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

Looks like this is to get rid of duplicates? This feels a bit like premature optimisation, also I'd expected this to return a []string (matching the type before removing duplicates)

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

Yes this is to remove duplicates, again to match kubectl behavior. I'll change the return type to a []string.

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

…and refactor it to

func removeDuplicates(namespaces []string) []string {
	mnms := map[string]struct{}{}
	for _, nm := range namespaces {
		mnms[nm] = struct{}{}
	}
	namespaces = []string{}
	for nm := range mnms {
		namespaces = append(namespaces, nm)
	}
	return namespaces
}
if len(namespace) > 0 {
opts.Namespace = namespace[0]
} else if nm, err := flags.GetString("namespace"); err == nil {
opts.Namespace = nm

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

There's now opts.Namespaces (for stacks) and opts.Namespace, correct?

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

Yes, there first one is of type []string and the latter string.

ss, err := kubernetes.GetStacks(kli, opts)
if err != nil {
return err
if opts.AllNamespaces || len(mnms) == 0 {

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

Wouldn't len(opts.Namespaces) give the same here?

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

Oh yes, nice catch!

return ss, nil
}
func getNamespaces(cmd *cobra.Command) (map[string]struct{}, error) {

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

This could just take (flags *flag.FlagSet, or even namespaces []string, then call it with getNamespaces(cmd.Flags()), but see my other comments

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

This has actually been changed to receive a flags *flag.FlagSet in a follow-up PR, but I might as well do it there.

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

Hmm sorry I meant []string and passing it opts.Namespaces

@@ -23,20 +23,28 @@ type Options struct {
}
// NewOptions returns an Options initialized with command line flags
func NewOptions(flags *flag.FlagSet) Options {
func NewOptions(flags *flag.FlagSet, namespace ...string) Options {

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

Both flags and namespace basically come from cmd.Flags(); why pass both, if we have all information in flags?

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

The way Kubernetes works implies that listing the stacks (or anything else) must be made within the context of a namespace, thus as we want to merge the lists of stacks from different namespaces we create a new kubernetes.Options for each namespace when iterating over them for docker stack ls.
I'll refactor the code to separate the different behaviors in respect to namespaces.

opts.Namespace = namespace
if len(namespace) > 0 {
opts.Namespace = namespace[0]
} else if nm, err := flags.GetString("namespace"); err == nil {

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

in what situation would flags.GetString("namespace") give anything different than namespace[0]? If we only expect a single namespace to be passed, it should not take more than one

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

This is again due to the discrepancy between docker stack ls and the other docker stack sub-commands regarding the number of --namespace flags allowed.

@@ -54,7 +54,7 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command {
flags = cmd.Flags()
flags.BoolVarP(&opts.Version, "version", "v", false, "Print version information and quit")
flags.StringVar(&opts.ConfigDir, "config", cliconfig.Dir(), "Location of client config files")
opts.Common.InstallFlags(flags)
opts.Common.InstallFlags(flags, cmd.PersistentFlags())

This comment has been minimized.

@thaJeztah

thaJeztah May 9, 2018

Member

Slightly confused as well

@thaJeztah thaJeztah changed the title from [WIP] Better stack list experience with Kubernetes and Swarm to Better stack list experience with Kubernetes and Swarm May 9, 2018

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented May 9, 2018

This needs a rebase now

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch 2 times, most recently from c4e2c8b to e0dbcb3 May 10, 2018

@mat007

This comment has been minimized.

Contributor

mat007 commented May 10, 2018

@thaJeztah I believe the code to be much clearer now, thank you for your comments!
I merged the changes into 539654c as it was the commit which they all originated from.
Would you please take another look?

kopts := kubernetes.NewOptions(cmd.Flags())
if opts.AllNamespaces || len(opts.Namespaces) == 0 {
if dockerCli.ClientInfo().HasAll() {
opts.AllNamespaces = true

This comment has been minimized.

@wsong

wsong May 10, 2018

Contributor

So if you have orchestrator=all and then you do docker stack ls --namespace=foobar, this will still return all stacks in every namespace?

This comment has been minimized.

@mat007

mat007 May 10, 2018

Contributor

This branch is only executed if opts.AllNamespaces || len(opts.Namespaces) == 0 e.g. there is already --orchestrator=all or no --namespace.
Here is what this gives me:

$ ./build/docker.exe --orchestrator=all stack ls
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
blaaaaaa            1                   Kubernetes          default
blaaaaaaaaa         1                   Kubernetes          default
dtc                 5                   Kubernetes          tutu
dtc                 5                   Kubernetes          tata
dtcccccc            5                   Kubernetes          default

$ ./build/docker.exe --orchestrator=all stack ls --namespace=tata
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
dtc                 5                   Kubernetes          tata

This comment has been minimized.

@wsong

wsong May 10, 2018

Contributor

Ah, I see now.

@mat007

This comment has been minimized.

Contributor

mat007 commented May 11, 2018

This looks like what we want because

docker stack deploy --namespace=foo --namespace=bar -c docker-compose.yml --orchestrator=swarm foo
unknown flag: --orchestrator

The orchestrator flag must be right after docker (I removed the commit which made it persistent).

docker --orchestrator=swarm stack deploy --namespace=foo --namespace=bar -c docker-compose.yml foo
"--namespace" is only supported on a Docker cli with kubernetes features enabled

Passing --orchestrator=swarm tells docker to target that orchestrator for which the --namespace makes no sense, hence the error.

Or am I missing something?

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch 2 times, most recently from 54ac68a to 1f9c976 May 11, 2018

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented May 14, 2018

The orchestrator flag must be right after docker (I removed the commit which made it persistent).

hm, right; I'm in doubt if we should only accept it as a global flag. I know users can get very confused as to "where" to pass a flag; existing examples are, e.g. docker container run -e foo mysql --host=bar (where --host=bar is a mysql option, but -e is a docker option); adding global flags makes this even more confusing.

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch from 1f9c976 to 8bddaa0 May 14, 2018

@mat007

This comment has been minimized.

Contributor

mat007 commented May 14, 2018

@thaJeztah I brought the refactoring over from https://github.com/docker/cli/pull/1034/files/e0dbcb3b88f615d358fc5bed93ca3d14aa094d87..691eeff5a5db87be3fe5e0f1ca852024edf08f1c#diff-0ade8f01def39989ced9bb51bab08a69
Tell me if you'd like me to change the orchestrator flag, maybe we could move it after docker stack for instance? This could be addressed in another PR if needed though as this has not been touched in this one?

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch 3 times, most recently from 978099b to 5c32773 May 14, 2018

Fix typo in golden file name
Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch from 5c32773 to ef7be30 May 15, 2018

@mat007

This comment has been minimized.

Contributor

mat007 commented May 15, 2018

@vdemeester @thaJeztah I can remove the last commit if that helps moving forward with this PR (and either open a separate PR with it or just forget about it)?

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch from ef7be30 to 685e0dd May 15, 2018

@thaJeztah

Code generally LGTM, but perhaps we should indeed move the refactor out of this PR (t.b.h., I don't recall suggesting that change, but it could've been me, idk)

"github.com/docker/cli/cli"
"github.com/docker/cli/cli/command"
"github.com/spf13/cobra"
)
var errUnsupportedAllOrchestrator = fmt.Errorf("Please specify an orchestrator (swarm|kubernetes)")

This comment has been minimized.

@thaJeztah

thaJeztah May 15, 2018

Member

Thinking of rephrasing this (also in light of #770); perhaps;

No orchestrator specified. Use either "kubernetes" or "swarm"

(better suggestions welcome)

This comment has been minimized.

@mat007

mat007 May 15, 2018

Contributor

As an error message shouldn't it start with a lower case? I would probably avoid the dot too, e.g.

no orchestrator specified, use either "kubernetes" or "swarm"

or

no orchestrator specified: use either "kubernetes" or "swarm"

?

This comment has been minimized.

@thaJeztah

thaJeztah May 15, 2018

Member

I like this one:

no orchestrator specified: use either "kubernetes" or "swarm"
composeClient, err := kubeCli.composeClient()
if err != nil {
return nil, err
}
stackSvc, err := composeClient.Stacks(allNamespaces)
stackSvc, err := composeClient.Stacks(opts.AllNamespaces)

This comment has been minimized.

@thaJeztah

thaJeztah May 15, 2018

Member

Not for this PR, but we should consider passing an opts here, instead of a boolean (opts.AllNamespaces) to prevent future bloat in the signature

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented May 15, 2018

I still have reservations on the magic relation between --orchestrator=all and --all-namespaces (copying my earlier comment #1031 (comment) here, as it was now hidden);

I'm having trouble conflating these; this may become very confusing:

  • configuration has "orchestrator: all": docker stack ls shows all stacks (both swarm and k8s)
  • I want to view only the k8s stacks, so I add --orchestrator=kubernetes. Now I no longer see all my stacks, only those in the default namespace
  • confused

@chris-crone :

The thinking behind this was as follows: When you list stacks across orchestrators your intention is to see all the stacks running on your cluster. When you specify kubernetes as your orchestrator; you are doing something more specific and want similar behaviour to what kubectl gives you.

I can see how this might be confusing for your flow. There will be a NAMESPACE column which should help the user understand what has happened.

@chris-crone : Ah, see you're referring to the case when a user has set orchestrator to all in their config file. I don't think many users will do this as they will only be able to list resources. In order to manipulate resources, they will need to specify an orchestrator.

I don't think many users will do this as they will only be able to list resources.

I'm actually not sure: I can definitely see it being useful to just run any docker <foo> list or docker <foo> ps command, and see everything I have running, then use --filter <something> and/or --orchestrator=x to limit the list to what I'm looking for, or (when deploying / interacting with objects), pass --orchestrator=foo --namespace=bar as part of the steps to deploy.

Which brings me to two things to consider:

  • Add orchestrator and namespace as filter options (--filter orchestrator=swarm) (no high-priority, but for consistency)
  • When interacting with existing objects, only require --orchestrator and/or --namespace if the object refered to is ambiguous, for example, if there's only one stack with a given name, there would be no need to ask for --orchestrator.

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch from 685e0dd to 98224a1 May 15, 2018

@mat007

This comment has been minimized.

Contributor

mat007 commented May 15, 2018

I removed the commit with the tests refactoring and changed the error message when an orchestrator must be explicitly specified.

Support 'all' in orchestrator flag for docker stack ls
All other docker stack commands report an error when passed this value.

Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch from 98224a1 to 4c013ee May 15, 2018

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented May 15, 2018

@mat007 could you move the all orchestrators -> all namespaces changes to a separate PR to unblock this one?

It's three lines, so easy to review from a code-perspective, so if we get agreement on that one, we should be able to merge that one fast;

if dockerCli.ClientInfo().HasAll() {
	opts.AllNamespaces = true
}
Support multiple namespaces for docker stack ls
Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>

@mat007 mat007 force-pushed the mat007:kubernetes-orchestrator-all branch from 4c013ee to 84241cc May 15, 2018

@mat007

This comment has been minimized.

Contributor

mat007 commented May 15, 2018

@thaJeztah done!

@thaJeztah

LGTM

sorry for all the hence-and-forth 😅

@vdemeester

LGTM 🐯

@vdemeester vdemeester merged commit 5a5b880 into docker:master May 15, 2018

6 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 18.06.0 milestone May 15, 2018

@chris-crone

This comment has been minimized.

Contributor

chris-crone commented May 16, 2018

Just to keep track, the all orchestrator all namespace work is in #1059.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment