feat: adding install method on minikube download #173
Conversation
Signed-off-by: axel7083 <[email protected]>
src/download.ts
Outdated
| try { | ||
| destFile = await installBinaryToSystem(destFile, 'minikube'); | ||
| } catch (err: unknown) { | ||
| console.debug('Cannot install system wide', err); |
There was a problem hiding this comment.
should it be reported at error level ?
also is it expected that we don't fail ? (report an error) it's like we're silently ignoring that we can't install it system wide ?
There was a problem hiding this comment.
but I would expect it's an issue if we can't
There was a problem hiding this comment.
I don't know what is the expected behaviour in the current main branch we have the doUpdate which try to install system wide
if it fails, just cannot update the binary, and need to restart
The status bar install seems to ask the user:
So my proposal, following your comment would be
- download in extension folder
- ask user to install system wide ?
- yes => try, if fails, create a notification error
- no => don't try to install system wide
There was a problem hiding this comment.
it seems a better way involving the user yes
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
| let destFile = await this.download(release); | ||
|
|
||
| const result = await window.showInformationMessage( | ||
| `minikube binary has been successfully downloaded.\n\nWould you like to install it system-wide for accessibility on the command line? This will require administrative privileges.`, |
There was a problem hiding this comment.
maybe add the version of minikube in the text
Following #172
When we
CliTool#registerInstallandCliTool#registerUpdatewe need a method to download and install the binary, instead of repeating the code between those two object, we can create a unique methodinstalltaking the artifact as argument. Why ? Because we have a bit of logic:And having this method avoid repeating the same lines.