-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Kubernetes orchestrator all namespaces #1059
Kubernetes orchestrator all namespaces #1059
Conversation
Split from #1031, only the last commit is new. |
For reference, here is the current state of the discussion from #1031
|
So
Thoughts: we could;
I'm leaning towards 4. (or 3.), as it keeps the user in control;
|
cc @thaJeztah @vdemeester @mat007 I'm opened to any comment on the config file option 😄 |
Some details on the UX:
Updated
|
cli/config/configfile/file.go
Outdated
Proxies map[string]ProxyConfig `json:"proxies,omitempty"` | ||
Experimental string `json:"experimental,omitempty"` | ||
Orchestrator string `json:"orchestrator,omitempty"` | ||
StackListAllNamespaces string `json:"stackListAllNamespaces,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should start to using some nested
struct, like
{
"orchestrator": "kubernetes",
"stack": {
"list": {
"allNamespaces": "disabled",
"format": "…",
}
}
}
cc @thaJeztah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not, but maybe in a follow up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we only use this configuration for stacks, or also for docker service ls
? (wondering about the Stack
prefix.
Alternatively, if we want to use a boolean instead of the disabled
string, we could use {"singleNamespace": true}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so what about
{
"kubernetes": {
"singleNamespace": true
}
}
And by the way, singleNamespace makes you think you can only list one namespace, but you still can use multiple --namespace
in the docker stack ls
command, so it could be a little confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thaJeztah hum good point.
I tend to agree with @silvin-lubecki on the singleNamespace
name… but nothing really pop in my head for a correct name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point; I'd say:
- use
"allNamespaces": "disabled"
- also accept
"allNamespaces": "enabled"
for those that want to be explicit - if no option is set in the configuration,
"allNamespaces": "enabled"
is the default
SGTY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
Looks like there's a linting failure;
|
Indeed 😔 |
…or is all or Kubernetes * Add "kubernetes" struct in config file with "allNamespaces" option, to opt-out this behavior when set as "disabled" Signed-off-by: Mathieu Champlon <[email protected]>
Signed-off-by: Silvin Lubecki <[email protected]>
PTAL 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As a follow-up we need to update the CLI configuration documentation https://github.com/docker/cli/blob/master/docs/reference/commandline/cli.md#configuration-files
Unfortunately, that one is quite messy, and in need of a proper rewrite (with actual examples 😞)
return getStacksWithAllNamespaces(kubeCli, opts) | ||
} | ||
return getStacksWithNamespaces(kubeCli, opts, removeDuplicates(opts.Namespaces)) | ||
} | ||
|
||
func isAllNamespacesDisabled(kubeCliConfig *configfile.KubernetesConfig) bool { | ||
return kubeCliConfig == nil || kubeCliConfig != nil && kubeCliConfig.AllNamespaces != "disabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, one thing: do we want a strict check to only accept "disabled", "enabled" or nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, @silvin-lubecki will follow up 👼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I will!
Is this the only documentation we have about the CLI configuration? Nothing else on docs.docker.com? |
We may need some more in general (about working with multiple orchestrators); not sure if "orchestrator" was already documented in the configuration; also some flags/options are currently experimental, so we'll have to do a "sweep" after they leave experimental, so see if everything is there. Would be good to have a tracking issue in which we can add things that we think about (could be a checkbox-list style) |
Not yet in the documentation as it was behind the experimental feature flag. |
- What I did
I made
--orchestrator=all
behave as if also--all-namespaces
unless specific namespaces have been queried with--namespace
- How I did it
Opts.AllNamespaces gets set to true if requesting for all orchestrators without
specifying
a namespace- Description for the changelog
Setting
--orchestrator=all
also sets--all-namespaces
unless specific--namespace
are set.- A picture of a cute animal (not mandatory but encouraged)