backend/cmd/headlamp: Fix issue in cluster looking for kube config #2323
Conversation
Fixes #1826 Signed-off-by: René Dudfield <[email protected]>
|
Backend Code coverage changed from 60.2% to 60.3%. Change: .1% 😃. |
Signed-off-by: René Dudfield <[email protected]>
|
Backend Code coverage changed from 60.1% to 60.3%. Change: .2% 😃. |
There was a problem hiding this comment.
Left a comment in the 1st patch that then I realize you had already implemented in the 2nd patch.
I think the 2nd patch is the right approach. Let's keep the possibility of mixing loading clusters from different places, but I don't think we need the new todo comments.
backend/cmd/headlamp.go
Outdated
| if err != nil { | ||
| logger.Log(logger.LevelError, nil, err, "loading kubeconfig") | ||
| } | ||
| if !config.useInCluster { |
There was a problem hiding this comment.
Many users have asked how they could add more clusters when headlamp is running in a cluster. I find the mixed use-case a bit cornerish, but don't see a reason why we should restrict mixing loading kubeconfigs with in-cluster. So maybe what we need to do is not log issues about loading kubeconfigs if we are running in-cluster.
| err := kubeconfig.LoadAndStoreKubeConfigs(config.kubeConfigStore, kubeConfigPath, kubeconfig.KubeConfig) | ||
| if err != nil { | ||
| if config.useInCluster { | ||
| logger.Log(logger.LevelInfo, nil, err, "loading kubeconfig") |
There was a problem hiding this comment.
maybe we could define the log into a variable and then only have one logger.Log call? I guess the code would be more easily understood.
|
There's a bit more discussion since in this issue:
|
Either way, I think we need to keep supporting this. By default we don't look for a kubeconfig if |
Fixes #1826
I'm not sure what's best? The first commit disables it.
There is a use case of using kube config in cluster here: #1826 (comment)
But are the other statements needed in-cluster too?
I guess if we do support kube config loading in cluster then they should be warnings and not error logs?