-
Notifications
You must be signed in to change notification settings - Fork 288
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
Add support for KUBECONTEXT environment variable and --kubecontext option into botkube CLI #1457
Conversation
Ensure Current context is set into HelmCfg.
e1660b5
to
2532596
Compare
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.
Works well! 🚀
Please see my minor comments and run the following commands to pass the CI checks:
make gen-docs-cli
make lint-fix
Then I'll approve and merge it 👍 Thank you for your contribution!
internal/kubex/config.go
Outdated
// 3. load from rawConfig | ||
if len(kubecontext) == 0 { | ||
kubecontext = rawConfig.CurrentContext | ||
} | ||
|
||
return &ConfigWithMeta{ | ||
K8s: clientConfig, | ||
CurrentContext: rawConfig.CurrentContext, | ||
CurrentContext: kubecontext, |
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.
This change is not needed, right? As you pass the &clientcmd.ConfigOverrides{CurrentContext: kubecontext}
, it is already properly set? So that means we can revert this to:
func transform(c clientcmd.ClientConfig) (*ConfigWithMeta, error) {
rawConfig, err := c.RawConfig()
if err != nil {
return nil, fmt.Errorf("while getting raw config: %v", err)
}
clientConfig, err := c.ClientConfig()
if err != nil {
return nil, fmt.Errorf("while getting client config: %v", err)
}
return &ConfigWithMeta{
K8s: clientConfig,
CurrentContext: rawConfig.CurrentContext,
}, 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, I also feel like it was redundant.
I was expecting the same but I don't know why the value of c.RawConfig().CurrentContext
is not equal to the value set by configOverrides := &clientcmd.ConfigOverrides{CurrentContext: kubecontext}
by the caller of transform
.
The value of c.RawConfig().CurrentContext
remains the current context in my ~/.kube/config
, it is not override yet?!
I also tried to configure only ConfigWithMeta.CurrentContext
in the transform
function and gave up the overrides in LoadRestConfigWithMetaInformation
. The ConfigWithMeta.CurrentContext
was set correctly but Helm was trying to connect to the cluster which was corresponding to the previously context value.
return transform(clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, nil))
}
func transform(c clientcmd.ClientConfig) (*ConfigWithMeta, error) {
rawConfig, err := c.RawConfig()
if err != nil {
return nil, fmt.Errorf("while getting raw config: %v", err)
}
clientConfig, err := c.ClientConfig()
if err != nil {
return nil, fmt.Errorf("while getting client config: %v", err)
}
// 1. --kubecontext flag
if kubecontext == "" {
// 2. KUBECONTEXT env
kubecontext = os.Getenv("KUBECONTEXT")
// 3. load from rawConfig
if len(kubecontext) == 0 {
kubecontext = rawConfig.CurrentContext
}
}
return &ConfigWithMeta{
K8s: clientConfig,
CurrentContext: kubecontext,
}, nil
}
I cannot get it work otherwise. I wish to understand but I may not have enough XP with Go. It just let me fell like a pointer issue or I miss understand how overrides work. I'm baffled!
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, I understand now 👍 Fair enough. I'm also not sure why the behavior is like that but I think we can skip that part to not spend too much time on it.
What do you think about such code then?
// ...
configOverrides := &clientcmd.ConfigOverrides{CurrentContext: loadKubecontext("")}
return transform(clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides))
}
func loadKubecontext(fallbackValue string) string {
// 1. --kubecontext flag
if kubecontext != "" {
return kubecontext
}
// 2. KUBECONTEXT env
kubeCtx := os.Getenv("KUBECONTEXT")
if kubeCtx != "" {
return kubeCtx
}
return fallbackValue
}
func transform(c clientcmd.ClientConfig) (*ConfigWithMeta, error) {
rawConfig, err := c.RawConfig()
if err != nil {
return nil, fmt.Errorf("while getting raw config: %v", err)
}
clientConfig, err := c.ClientConfig()
if err != nil {
return nil, fmt.Errorf("while getting client config: %v", err)
}
return &ConfigWithMeta{
K8s: clientConfig,
CurrentContext: loadKubecontext(rawConfig.CurrentContext),
}, nil
}
Does it look a bit cleaner? If so, then please apply the snippet 👍
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.
Good by me 👍 .
internal/kubex/config.go
Outdated
// 1. --kubecontext flag | ||
if kubecontext == "" { | ||
// 2. KUBECONTEXT env | ||
kubecontext = os.Getenv("KUBECONTEXT") | ||
} | ||
|
||
return transform(clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, nil)) | ||
configOverrides := &clientcmd.ConfigOverrides{CurrentContext: kubecontext} | ||
return transform(clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides)) | ||
} |
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.
What do you think about extracting the kubecontext logic to a separate function? E.g. something like
configOverrides := &clientcmd.ConfigOverrides{CurrentContext: loadKubecontext()}
return transform(clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides))
}
func loadKubecontext() string {
// 1. --kubecontext flag
if kubecontext != "" {
return kubecontext
}
// 2. KUBECONTEXT env
return os.Getenv("KUBECONTEXT")
}
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.
Sounds reasonable to me.
I will run the commands once the code logic is approved. |
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.
🚀 Thanks again for such valuable contribution!
I will edit later. I'm still testing.
Description
Changes proposed in this pull request:
Testing
Related issue(s)
Resolves #1456