-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Relaxed requirements for version in kubeConfig #1200
Conversation
@caesarxuchao could you please take a look into this? |
Thanks @piosz. For the context: Heapster in Kubemark started crashlooping after recent version bump with error:
It seems that it's caused by the changes in kubeclient. |
@@ -120,9 +120,6 @@ func GetKubeClientConfig(uri *url.URL) (*kube_client.Config, error) { | |||
if len(kubeConfig.Host) == 0 { | |||
return nil, fmt.Errorf("invalid kubernetes master url specified") | |||
} | |||
if len(kubeConfig.GroupVersion.Version) == 0 { |
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'd say you should set kubeConfig.GroupVersion at line 111, that's the only path that could end up with a nil GroupVersion and cause the panic. I don't know why this problem is exposed now.
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 @caesarxuchao
To give you more context this started failing after bumping the Kubernetes version in #1158 from kubernetes/kubernetes@644d651 (~late Feb 2016) to kubernetes/kubernetes@4bb30e0 (~mid May 2016). After that NewNonInteractiveDeferredLoadingClientConfig function started returning empty version.
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.
Sorry I don't have time to investigate why NewNonInteractiveDeferredLoadingClientConfig() returns empty version. On the other hand, the version in the kubeconfig isn't useful currently, it will be given a default value anyway, so I can LGTM the PR. It's just that you might get surprises when we have more groups that have multiple versions and the default version isn't the one you want (currently only batch
have two versions).
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.
No worries. This is not high priority. Thanks for looking into it.
LGTM |
@gmarek