Try to load kubeconfig from wsl if present. #142
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
node-client/src/config.ts
Outdated
| @@ -205,6 +206,13 @@ export class KubeConfig { | |||
| return; | |||
| } | |||
| } | |||
| if (process.platform === 'win32' && shelljs.which('wsl.exe')) { | |||
| const result = shelljs.exec('wsl.exe cat $HOME/.kube/config', { silent: true }); | |||
There was a problem hiding this comment.
This doesn't feel safe to me, for a couple of reasons.
1 (minor practical concern). Invoking wsl.exe on my home machine took a while and was side-effectful (performed a file system upgrade) and produced additional output; unfortunately I couldn't test to see if this happened with wsl cat ... because it was a one-time upgrade and I had already blown my chance to reproduce it on an interactive prompt! grin
2 (major 'least surprise' concern). I am sceptical about preferring the WSL kubeconfig over the Windows kubeconfig if they are different. I have both WSL and Windows but 90% of the time I want the Windows kubeconfig. I may be abnormal but it seems like this could be surprising to an end user who when they run kubectl config at their (Windows) command prompt it tells them they have clusters A, B, C and D but this Electron app insists that they have D, E and F. If they are running an app inside WSL then it should use the WSL kubeconfig, otherwise I feel it should use the Windows kubeconfig.
If you strongly want this then let's provide an options object where a consumer of the library can specify { configSource: "windows" | "wsl" } or something. But even then I worry about app developers using the library and violating user expectations...
There was a problem hiding this comment.
Well, your windows kubeconfig should be caught and preferred by the ${HOME}/.kube/config lookup above this.
This only happens if there isn't a kubeconfig in your Windows home (and you're on Windows, and wsl is installed)
That seems mostly safe to me, but... There's a way I can implement this w/o using WSL, so I'll do that, since that will be safer, this was just cheaper/easier :)
There was a problem hiding this comment.
Bah, that's what I get for not looking at the full diff context. In that case yes, it behaves the way I'd expect and I'm good with it. Sorry for the false alarm.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
removed
the
lgtm
brendandburns
added
the
lgtm
|
self lgtm, since it was just a rebase. |
@itowlson