Adds a kubecontext global flag #296
Conversation
pkg/client/factory.go
Outdated
| f.flags.StringVarP(&f.namespace, "namespace", "n", f.namespace, "The namespace in which Ark should operate") | ||
| f.flags.StringVar(&f.kubecontext, "kubecontext", "", "The context to use to talk to the Kubernetes apiserver. If unset defaults to whatever your current-context is (kubectl config current-context)") |
There was a problem hiding this comment.
@Bradamant3 pls review the text here
pkg/client/factory.go
Outdated
| f.flags.StringVarP(&f.namespace, "namespace", "n", f.namespace, "The namespace in which Ark should operate") | ||
| f.flags.StringVar(&f.kubecontext, "kubecontext", "", "The context to use to talk to the Kubernetes apiserver. If unset defaults to whatever your current-context is (kubectl config current-context)") | ||
| f.flags.StringVar(&f.kubeconfig, "kubeconfig", "", "Path to the kubeconfig file to use to talk to the Kubernetes apiserver. If unset, try the environment variable KUBECONFIG, as well as in-cluster configuration") |
There was a problem hiding this comment.
@blakebarnett any particular reason you moved this down?
There was a problem hiding this comment.
nope, I must have done that when I rebased, will fix.
pkg/client/client.go
Outdated
| func Config(kubeconfig, kubecontext, baseName string) (*rest.Config, error) { | ||
| loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() | ||
| loadingRules.ExplicitPath = kubeconfig | ||
| configOverrides := &clientcmd.ConfigOverrides{} |
There was a problem hiding this comment.
Since you're only setting 1 field in here, how about this?
configOverrides := &clientcmd.ConfigOverrides{CurrentContext: kubecontext}
|
@blakebarnett thanks for the PR! Would you please sign your commit, and also run |
blakebarnett
force-pushed
the
bdb/add_kubecontext_flag
branch
from
0147b73 to
8172d24
Compare
|
Ah I see, I need to actually update the docs to include, I'll take care of that in the next commit. |
|
@blakebarnett looking pretty good! Just run |
| @@ -68,6 +69,7 @@ func NewFactory(baseName string) Factory { | |||
|
|
|||
| f.flags.StringVar(&f.kubeconfig, "kubeconfig", "", "Path to the kubeconfig file to use to talk to the Kubernetes apiserver. If unset, try the environment variable KUBECONFIG, as well as in-cluster configuration") | |||
There was a problem hiding this comment.
This part of the message isn't clear:
"If unset, try the environment variable KUBECONFIG, as well as in-cluster configuration"
Are these meant to be alternatives to setting the path? Do you need both? Is either of them the default if not set? Or would you have to try one or the other for the command to succeed? In other words, does not specifying result in an error condition?
Or does the command look for a KUBECONFIG environment variable, and if not found then look for the path in the cluster config?
If it's the latter, the fix is fairly simple:
"If unset, tries the environment variable KUBECONFIG, and if not present, tries in-cluster configuration"
If not the latter, questions ^^ might help with clearer text.
There was a problem hiding this comment.
@Bradamant3 we can address the kubeconfig text in another issue. This PR is about kubecontext 😄
|
@blakebarnett FYI, we'll pick this up as soon as master is open for v0.8.0 later this week. Thanks for your patience! |
|
@blakebarnett we'd like to get this in sometime soon. Could you please squash & sign your commit? |
|
I signed it, I thought GH would auto-squash on merge? |
|
You currently have 4 commits, and each one must have the DCO stanza in it. I don't see it in any of them? |
blakebarnett
force-pushed
the
bdb/add_kubecontext_flag
branch
from
4153f5f to
a2b6075
Compare
Signed-off-by: Blake <blake.barnett@postmates.com>
blakebarnett
force-pushed
the
bdb/add_kubecontext_flag
branch
from
a2b6075 to
465c60b
Compare
|
sorry, never used GH's signing stuff, I think that's done it? |
|
DCO is good, thanks! |
|
Tested this, can confirm that it works as described. |
|
Ugh, helm uses |
|
We could probably support both |
|
Yeah, I did it this way because it already had |
Fixes #207