Skip to content
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

[WIP] Issue #72 Add update command #813

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@dharmit
Copy link
Collaborator

dharmit commented Apr 24, 2017

Addresses issue #72

minishift update prompts user to update minishift if a newer version
is available.

downloadBinary := binaryName + "-" + runtime.GOOS + "-" + runtime.GOARCH
latestVersion, err := getLatestVersionFromGitHub(githubOwner, githubRepo)
if err != nil {
glog.Errorln(err)

This comment has been minimized.

@LalatenduMohanty

LalatenduMohanty Apr 24, 2017

Member

We are now using atexit.ExitWithMessage method instead of glog.Errorln().

This comment has been minimized.

@hferentschik
@@ -90,11 +95,13 @@ automatically update from %s%s to %s%s now? [y/N] `,

if strings.ToLower(confirm) == "y" {

This comment has been minimized.

@LalatenduMohanty

LalatenduMohanty Apr 24, 2017

Member

It should accept both y and Y.

This comment has been minimized.

@praveenkumar

praveenkumar Apr 25, 2017

Contributor

I think that what's ToLower will do.

This comment has been minimized.

@coolbrg

coolbrg Apr 28, 2017

Member

confirm == 'Y' || strings.ToLower(confirm) should be there.

fmt.Println("Skipping auto-update.")
fmt.Println("Skipping update.")
} else {
fmt.Println("Already using latest version.")

This comment has been minimized.

@LalatenduMohanty

LalatenduMohanty Apr 24, 2017

Member

I think we should mention the version too. so that user will know which is the latest version here.

This comment has been minimized.

@LalatenduMohanty

LalatenduMohanty Apr 24, 2017

Member

s/Already using latest version./Local minishift is at latest version : $version/

This comment has been minimized.

@hferentschik
}

// Write tar file to disk
err = ioutil.WriteFile("/tmp/minishift.tgz", tarballBytes, 0644)

This comment has been minimized.

@LalatenduMohanty

LalatenduMohanty Apr 24, 2017

Member

Please use something similar to tempDir, err := ioutil.TempDir("", "minishift") to create the temp directory and then use it for the purpose.

This comment has been minimized.

@hferentschik

hferentschik Apr 24, 2017

Member

-1 how is this working for Windows? There are several things wrong here. The use of the /tmp directory as well as hard coding the name. You will need to look at what you are actually downloading here.

This comment has been minimized.

@hferentschik

hferentschik Apr 24, 2017

Member

In fact, I think we should use the tmp directory of the MINISHIFT_HOME to do all this, e.g. _ ~/.minishift/tmp/_

downloadBinary := binaryName + "-" + runtime.GOOS + "-" + runtime.GOARCH
latestVersion, err := getLatestVersionFromGitHub(githubOwner, githubRepo)
if err != nil {
glog.Errorln(err)

This comment has been minimized.

@hferentschik
return
}

downloadTarball := fmt.Sprintf("%s-%s-%s-%s.tgz", binaryName, latestVersion, runtime.GOOS, runtime.GOARCH)

This comment has been minimized.

@hferentschik

hferentschik Apr 24, 2017

Member

We are not only downloading tarballs. Windows archive format is zip

@@ -90,11 +95,13 @@ automatically update from %s%s to %s%s now? [y/N] `,

if strings.ToLower(confirm) == "y" {
fmt.Printf("Updating to version %s\n", latestVersion)
updateBinary(latestVersion, downloadBinary, updateLinkPrefix, downloadLinkFormat)
updateBinary(latestVersion, downloadTarball, updateLinkPrefix, downloadLinkFormat)

This comment has been minimized.

@hferentschik

hferentschik Apr 24, 2017

Member

-1 to variable name. We are dealing with archives in general. We have tar as well as zip format.

fmt.Println("Skipping auto-update.")
fmt.Println("Skipping update.")
} else {
fmt.Println("Already using latest version.")

This comment has been minimized.

@hferentschik
}

// Write tar file to disk
err = ioutil.WriteFile("/tmp/minishift.tgz", tarballBytes, 0644)

This comment has been minimized.

@hferentschik

hferentschik Apr 24, 2017

Member

-1 how is this working for Windows? There are several things wrong here. The use of the /tmp directory as well as hard coding the name. You will need to look at what you are actually downloading here.

return hash.Sum([]byte{}), nil
}

func removeFiles() {

This comment has been minimized.

@hferentschik

hferentschik Apr 24, 2017

Member

-1, if you use a tmp directory you can clean up via a defer

}

// Write tar file to disk
err = ioutil.WriteFile("/tmp/minishift.tgz", tarballBytes, 0644)

This comment has been minimized.

@hferentschik

hferentschik Apr 24, 2017

Member

In fact, I think we should use the tmp directory of the MINISHIFT_HOME to do all this, e.g. _ ~/.minishift/tmp/_

@coolbrg

This comment has been minimized.

Copy link
Member

coolbrg commented Apr 26, 2017

Some few unit tests too.

@hferentschik

This comment has been minimized.

Copy link
Member

hferentschik commented Apr 26, 2017

I think we need to discuss also, how this is supposed to work. There are several gaps here.

@dharmit dharmit force-pushed the dharmit:fix-72 branch 3 times, most recently from 95110d3 to 5b59cf0 Apr 26, 2017

@dharmit

This comment has been minimized.

Copy link
Collaborator Author

dharmit commented Apr 26, 2017

@hferentschik @budhrg I've added code to cover the scenario of a Windows system. However, not able to test it since I don't have a Windows system around.

Would like to cover any possible gaps that are left uncovered in this PR.

@dharmit

This comment has been minimized.

Copy link
Collaborator Author

dharmit commented Apr 26, 2017

how this is supposed to work

Based on what I have added through this PR and what I've understood by looking at the code:

  • when a user does minishift update, we check for latest version on GitHub.
  • If a newer version of minishift is available, we download the archive (tarball for Linux & Mac, zip for Windows). The way we force minishift to think that it's running on older version is by modifying Makefile.
  • Then we compare the checksum of downloaded archive with the checksum we downloaded before downloading the archive itself.
  • If the checksums match, we extract the content of the archive to a tempdir which we delete with help of defer call. (thank you @hferentschik)
  • We then apply the update which replaces the minishift binary on disk (~/go/bin/minishift on Linux dev environment) with the binary from archive.
  • Earlier we used to run minishift update after downloading the new binary. But that failed because the new archive (1.0.0-rc.2 at the time of opening this PR) didn't have update switch. As a result minishift update would error out with no such command message. Me and @LalatenduMohanty debugged this but couldn't find any other possible explanation. So I modified it to print the version instead i.e. minishift version.

Some few unit tests too.

Sure. I'd need some help on that however.

@hferentschik

This comment has been minimized.

Copy link
Member

hferentschik commented Apr 26, 2017

I'll add some comments on #72

}

// Create a temporary directory to store archive contents
dir, err := ioutil.TempDir("", "minishift")

This comment has been minimized.

@hferentschik

hferentschik Apr 26, 2017

Member

We should use the Minishift tmp dir.

// Create a temporary directory to store archive contents
dir, err := ioutil.TempDir("", "minishift")
if err != nil {
atexit.ExitWithMessage(1, "Could not create a temporary directory to store archive contents")

This comment has been minimized.

@hferentschik

hferentschik Apr 26, 2017

Member

We should not be using atexit anywhere outside of the commands. If there is an error, it should be returned.

@hferentschik

This comment has been minimized.

Copy link
Member

hferentschik commented Apr 26, 2017

Some few unit tests too.

+1 I'd like to see this working in a unit test as well.


defer os.RemoveAll(dir) // clean up

binary = extractAndReplace(runtime.GOOS, dir, archiveBytes)

This comment has been minimized.

@LalatenduMohanty

LalatenduMohanty Apr 28, 2017

Member

I am not sure why we need to pass archiveBytes instead of updatedArchive. cc @hferentschik

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

It does not make much sense to me either. I recommend to have a look at

func DownloadOpenShiftReleaseBinary(binaryType OpenShiftBinaryType, osType minishiftos.OS, version, outputPath string) error {
. There we also deal with downloading various archive types and unpacking them.

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

Why _ extractAndReplace_, what does get replaced there?

This comment has been minimized.

@dharmit

dharmit May 11, 2017

Author Collaborator

I am not sure why we need to pass archiveBytes instead of updatedArchive

My understanding is that we need to work on [] byte for writing and unpacking the binary from tar/zip. So instead of having to do archiveBytes, err := ioutil.ReadAll(updatedArchive) again, I passed it to the function. Is that a bad idea or is their a better way to do it?

Why _ extractAndReplace_, what does get replaced there?

Agree with you. Renaming it to extractBinary since it's returning a binary. Does that sound OK?

binary := filepath.Join(extract, "minishift")

// Write archive file to disk
err := ioutil.WriteFile(downloadedArchive, archiveBytes, 0644)

This comment has been minimized.

@LalatenduMohanty

LalatenduMohanty Apr 28, 2017

Member

Something is wrong here. archiveBytes has []byte for .zip and .tgz, isn't it? But we are writing only minishift.tgz.

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

It not wrong, just fishy. The code just writes the file to minishift.tgz regardless. It then checks the OS and if on Windows just unzips it, regardless of the extension.

So why not download and safe the archive as is, with the proper filename. Then you decide whether to use tar or zip based on the filename.

Also, as has as I can tell, this method is supposed returns the path to the extracted minishift executable - binary := filepath.Join(extract, "minishift"). However, on Windows the binary is minishift.exe. I have not tried, but would be very surprised if this works. One needs to handle the download and naming explicitly.

This comment has been minimized.

@dharmit

dharmit May 11, 2017

Author Collaborator

Right. Quite a few glitches there! How about I check the OS first and perform operations based on that? If OS is Windows, save to a file with .zip extension else .tgz extension? Then extract data out of it and return the path to binary (with exe extension on Windows). Modifying the code for your feedback.

@hferentschik
Copy link
Member

hferentschik left a comment

Even though generally doing what it is supposed to do, there are several issues. The code dealing with Windows is not correct. Also, there are the parts for the automatic update check which should be removed.

Overall, the code should be restructured to have a clearer structure. This would also allow to potentially add some unit tests. As it stands now, things are very entangled and as a result also hard to test.

}

func init() {
RootCmd.AddCommand(updateCmd)

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

What's about we add a check flag as well, which just checks whether there is a new version. update alone will check and do the update.

This way we can split out the concern of checking whether a new version exists from MaybeUpdate.

This comment has been minimized.

@dharmit

dharmit May 11, 2017

Author Collaborator

Sounds great. How about taking it as a separate issue/PR outside this one? I'll send in a new one once this gets merged.

Or do you think that as an absolute must for this PR to move further?

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

As you like.

@@ -53,29 +58,37 @@ var (
lastUpdateCheckFilePath = constants.MakeMiniPath("last_update_check")

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

Actually this enables the the code around WantUpdateNotification and ReminderWaitPeriodInHours, even though it is not hooked in. IMO we should remove all this for now and just have an explicit update.

return err
}

var downloadArchive string

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

archiveName is better imo, downloadArchive gives the impression I am dealing with the actual archive already.

downloadArchive = fmt.Sprintf("%s-%s-%s-%s.zip", binaryName, latestVersion, runtime.GOOS, runtime.GOARCH)
} else {
downloadArchive = fmt.Sprintf("%s-%s-%s-%s.tgz", binaryName, latestVersion, runtime.GOOS, runtime.GOARCH)
}
updateLinkPrefix := "https://github.com/" + githubOwner + "/" + githubRepo + "/releases/tag/" + version.VersionPrefix
downloadLinkFormat := "https://github.com/" + githubOwner + "/" + githubRepo + "/releases/download/v%s/%s"

if !shouldCheckURLVersion(lastUpdatePath) {

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

This can go, if we don't deal with implicit update checks

downloadArchive = fmt.Sprintf("%s-%s-%s-%s.zip", binaryName, latestVersion, runtime.GOOS, runtime.GOARCH)
} else {
downloadArchive = fmt.Sprintf("%s-%s-%s-%s.tgz", binaryName, latestVersion, runtime.GOOS, runtime.GOARCH)
}
updateLinkPrefix := "https://github.com/" + githubOwner + "/" + githubRepo + "/releases/tag/" + version.VersionPrefix
downloadLinkFormat := "https://github.com/" + githubOwner + "/" + githubRepo + "/releases/download/v%s/%s"

if !shouldCheckURLVersion(lastUpdatePath) {
return

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

You cannot just return. In this case you would need to return nil, but as said, we might as well remove this for now.

os.Exit(1)
}
defer func() { _ = httpResp.Body.Close() }()

binary := httpResp.Body
updatedArchive := httpResp.Body

This comment has been minimized.

@hferentschik

// Create a temporary directory inside minishift directory to store archive contents

dir, err := ioutil.TempDir(constants.Minipath, "tmp/download")

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

Actually, I change my mind. If we determine the tmp directory so late in the game, it is better to use just ioutil.TempDir without constants.Minipath,. Using this constant so deep in the code is bad. If we want to use the Minishift tmp dir we need to pass the tmp dir as a parameter.


defer os.RemoveAll(dir) // clean up

binary = extractAndReplace(runtime.GOOS, dir, archiveBytes)

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

It does not make much sense to me either. I recommend to have a look at

func DownloadOpenShiftReleaseBinary(binaryType OpenShiftBinaryType, osType minishiftos.OS, version, outputPath string) error {
. There we also deal with downloading various archive types and unpacking them.


defer os.RemoveAll(dir) // clean up

binary = extractAndReplace(runtime.GOOS, dir, archiveBytes)

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

Why _ extractAndReplace_, what does get replaced there?

binary := filepath.Join(extract, "minishift")

// Write archive file to disk
err := ioutil.WriteFile(downloadedArchive, archiveBytes, 0644)

This comment has been minimized.

@hferentschik

hferentschik May 6, 2017

Member

It not wrong, just fishy. The code just writes the file to minishift.tgz regardless. It then checks the OS and if on Windows just unzips it, regardless of the extension.

So why not download and safe the archive as is, with the proper filename. Then you decide whether to use tar or zip based on the filename.

Also, as has as I can tell, this method is supposed returns the path to the extracted minishift executable - binary := filepath.Join(extract, "minishift"). However, on Windows the binary is minishift.exe. I have not tried, but would be very surprised if this works. One needs to handle the download and naming explicitly.

@hferentschik hferentschik changed the title Issue #72 Add update command [WIP] Issue #72 Add update command May 10, 2017

@dharmit dharmit force-pushed the dharmit:fix-72 branch from 40024df to b4771f6 May 10, 2017

@@ -26,3 +26,4 @@ var SupportedVMDrivers = [...]string{

const DefaultVMDriver = "xhyve"
const OC_BINARY_NAME = "oc"
const BINARY_NAME = "minishift"

This comment has been minimized.

@coolbrg

coolbrg May 10, 2017

Member

@dharmit , please remove this and other related changes as it is not being used now.

@dharmit dharmit force-pushed the dharmit:fix-72 branch 3 times, most recently from 8361363 to 4ef594e May 11, 2017

@hferentschik

This comment has been minimized.

Copy link
Member

hferentschik commented May 11, 2017

Could you squash your commits, effectively we are just dealing with updates to single file.

@hferentschik
Copy link
Member

hferentschik left a comment

Much, much better. It feels we are almost there now :-)

}

func init() {
RootCmd.AddCommand(updateCmd)

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

As you like.

)

func MaybeUpdateFromGithub(output io.Writer) {
func MaybeUpdateFromGithub(output io.Writer) error {
localVersion, err := version.GetSemverVersion()
if err != nil {
glog.Errorln(err)

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

let's remove the log and be consistent. once you log once you don't. Let's just return the error and let the caller deal with it

return err
}

if checkLatestVersion(localVersion, latestVersion) {

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

isNewerVersion or something like this. checkLatestVersion does not really make clear what it does

latestVersion, err := getLatestVersionFromGitHub(githubOwner, githubRepo)

archiveName := fmt.Sprintf("minishift-%s-%s-%s.%s", latestVersion, runtime.GOOS, runtime.GOARCH, extName)
// updateLinkPrefix := "https://github.com/" + githubOwner + "/" + githubRepo + "/releases/tag/" + version.VersionPrefix

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

I guess this needs to be clean up, right

downloadLinkFormat := "https://github.com/" + githubOwner + "/" + githubRepo + "/releases/download/v%s/%s"

fmt.Printf("Updating to version %s\n", latestVersion)
// err := updateBinary(latestVersion, archiveName, updateLinkPrefix, downloadLinkFormat)

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

same as above

}
return downloadedArchive, dir, nil

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

why do you need to return the full path to the archive + the directory. The latter is implicit, right? Even if you later on need it again, you can easily get it from downloadedArchive, right?

This comment has been minimized.

@dharmit

dharmit May 12, 2017

Author Collaborator

Returning the full path so that it can be used in other functions that are going to extract the binary or replace existing binary with updated one.

Agree with dir part. Can get rid of it.

updateBinary(latestVersion, downloadBinary, updateLinkPrefix, downloadLinkFormat)
return
}
if err := downloadAndVerifyChecksum(downloadedArchive, downloadLinkFormat, archiveName, latestVersion); err != nil {

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

This really should be part of downloadAndSaveArchive. If the checksum does not match, downloadAndSaveArchive_ should just throw an error. Verifying the checksum is integral part of downloadAndSaveArchive

func shouldCheckURLVersion(filePath string) bool {
if !viper.GetBool(config.WantUpdateNotification) {
return false
err = extractAndUpdateBinary(currentBinary, downloadedArchive, dir)

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

This still puts extraction and update together. Why not just extractBinary which in turn returns the path to the extracted binary. Then you pass this path to a updateBinary

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

currentBinary is an unused parameter.

}

return hex.DecodeString(strings.TrimSpace(string(b)))
// binaryBytes, err := ioutil.ReadAll(binaryFile)

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

I guess this needs to be clean up as well

// return err
// }

err = update.Apply(binaryFile, update.Options{

This comment has been minimized.

@hferentschik

hferentschik May 11, 2017

Member

this part to the end should be really in a separate method.

lastUpdateTime := getTimeFromFileIfExists(filePath)
if time.Since(lastUpdateTime).Hours() < viper.GetFloat64(config.ReminderWaitPeriodInHours) {
return false
return nil

This comment has been minimized.

@hferentschik

hferentschik May 15, 2017

Member

Is this temporary code?

if time.Since(lastUpdateTime).Hours() < viper.GetFloat64(config.ReminderWaitPeriodInHours) {
return false
return nil
if err != nil {

This comment has been minimized.

@hferentschik

hferentschik May 15, 2017

Member

This can never happen, given the err check from above

This comment has been minimized.

@dharmit

dharmit May 15, 2017

Author Collaborator

@hferentschik i think I missed removing it during the refactoring.

@dharmit dharmit force-pushed the dharmit:fix-72 branch from 7294c5d to 9bd23f5 May 15, 2017

}
if localVersion.Compare(latestVersion) < 0 {

This comment has been minimized.

@hferentschik

hferentschik May 15, 2017

Member

This got completely dropped now. I think we this interactive step. We should get this back. I'd put that in the update command and expose from update.go

  • CurrentVersion()
  • LatestVersion()
  • IsNewer(current, latest)
  • Update (basically current MaybeUpdate)

This way you can implement the actual update logic in the update command. Determine whether there is a newer version, if not, display that Minishift is up to date. ATM, there is no feedback whatsoever, if your binary is up to date.

If there is a newer binary, you ask for confirmation of the update. If update is confirmed, you make the call for updating the binary.


func updateBinaryFile(url string, checksum []byte) {
fmt.Println("Downloading updated binary")
fmt.Println("Downloading updated archive")

This comment has been minimized.

@hferentschik

hferentschik May 15, 2017

Member

"Downloading release" or fmt.Sprintf("Downloading %s, url)

u := fmt.Sprintf(downloadLinkFormat, v, downloadBinary+".sha256")
// Download and verify checksum

fmt.Println("Downloading updated archive checksum to validate updated archive")

This comment has been minimized.

@hferentschik

hferentschik May 15, 2017

Member

I don't think this message is relevant for the user


if runtime.GOOS == "windows" {
// Store archive in zip file on Windows
downloadedArchive = filepath.Join(dir, "minishift.zip")

This comment has been minimized.

@hferentschik

hferentschik May 15, 2017

Member

You have the true achieve name in the url, right? In archiveName resp. url. Why not getting it from there? This seems to be fragile and makes assumptions which might change.

This comment has been minimized.

@dharmit

dharmit May 16, 2017

Author Collaborator

Yes, we can use the last part of the url. That also helps use remove the runtime.GOOS check, I guess.

urlSplit := strings.Split(url, "/")

downloadedArchive = filepath.Join(dir, urlSplit[len(urlSplit)-1])
if err := writeArchiveToDisk(downloadedArchive, archiveBytes); err != nil {
    return "", err
}
// It returns a string containing path to the downloaded archive.
// It returns an error if any of the steps failed.
func downloadAndVerifyArchive(latestVersion semver.Version, archiveName, downloadLinkFormat string) (string, error) {
url := fmt.Sprintf(downloadLinkFormat, latestVersion, archiveName)

This comment has been minimized.

@hferentschik

hferentschik May 15, 2017

Member

why don't you not just pass the url into this method? This seems to be all that is really relevant, right?

httpResp, err := http.Get(url)
if err != nil {
glog.Errorf("Cannot download binary: %s", err)
os.Exit(1)
return "", errors.New("Cannot download archive")

This comment has been minimized.

@hferentschik

hferentschik May 15, 2017

Member

use the explicit URL, more context about the failure

return "", err
}

archiveBytes, err = ioutil.ReadFile(downloadedArchive)

This comment has been minimized.

@hferentschik

hferentschik May 15, 2017

Member

You already have the archive bytes, why re-read them?

@gbraad

This comment has been minimized.

Copy link
Member

gbraad commented May 15, 2017

@dharmit I believe the current issue with:

PS C:\work\src\github.com\minishift\minishift> C:\work\bin\minishift.exe update
Updating to version 1.0.1
Downloading updated archive
 3.27 MB / 3.43 MB [=======================================================================================================================================================>-------]  95.29% 0sD
ownloading updated archive checksum to validate updated archive
 65 B / 65 B [=====================================================================================================================================================================] 100.00% 0s

E0515 21:17:22.983876   11760 update.go:135] Failed to execute updated binary C:\work\bin\minishift.exe: not supported by windows

might be related to kicking the executable out from underneath the running process and replace it with a new executable. you are then trying to invoke it from the same execution context. I guess this can lead to a disallowed state. After the/at the time of the run, you can actually find a .minishift.exe.old and a minishift.exe which can both be executed from cmd.

@coolbrg

This comment has been minimized.

Copy link
Member

coolbrg commented May 15, 2017

E0515 21:17:22.983876 11760 update.go:135] Failed to execute updated binary C:\work\bin\minishift.exe: not supported by windows

I am having same error.

you are then trying to invoke it from the same execution context. I guess this can lead to a disallowed state.

It seems so.

@coolbrg

This comment has been minimized.

Copy link
Member

coolbrg commented May 15, 2017

@dharmit will look tomorrow sure

@hferentschik

This comment has been minimized.

Copy link
Member

hferentschik commented May 15, 2017

might be related to kicking the executable out from underneath the running process

+1, I am surprised it works so far, trying to call this executable which you just updated from within the same process it questionable. Looking at the go-update example, it does actually terminate after the update.

I know this was code which already existed, but I am not sure whether this was ever tested on Windows. I think we might be ok with removing it altogether and relying on the error reporting of go-update. Calling version on the new binary is nothing which is per se required as part of the update.

@coolbrg

This comment has been minimized.

Copy link
Member

coolbrg commented May 16, 2017

Update from my side,

Currently going through the comments in go-update issue inconshreveable/go-update#5.

https://github.com/inconshreveable/go-update hasn't been updated since a year.

My plan is to check the code mentioned in this comment inconshreveable/go-update#5 (comment) or do investigating in https://github.com/rcrowley/goagain/ to see if it works in our usecase.

@dharmit

This comment has been minimized.

Copy link
Collaborator Author

dharmit commented May 16, 2017

I know this was code which already existed, but I am not sure whether this was ever tested on Windows. I think we might be ok with removing it altogether and relying on the error reporting of go-update. Calling version on the new binary is nothing which is per se required as part of the update.

+1

I had kept the piece of code that executes something based off the newly downloaded binary because the old code did that. I'm wondering if it adds any value to execute minishift version after the update? Earlier, it was calling minishift update but that failed since we were downloading a new binary from GitHub which didn't have the update command.

@dharmit dharmit force-pushed the dharmit:fix-72 branch 4 times, most recently from 91c9fe3 to db9dd89 May 16, 2017

}

if update.IsNewerVersion(localVersion, latestVersion) {
fmt.Printf("A newer version of minishift is available.\nDo you want to update from %s to %s now? [y/N]: ", localVersion, latestVersion)

This comment has been minimized.

@coolbrg

coolbrg May 18, 2017

Member

Should be [y/n].

This comment has been minimized.

@dharmit

dharmit May 18, 2017

Author Collaborator

That indicates that if a user will hit return without entering a "y" or "N", default action is to not update the binary. That's how dnf update and apt-get update work as well. apt defaults to "Y" and dnf defaults to "N", IIRC.

return err
}

// env := os.Environ()

This comment has been minimized.

@coolbrg

coolbrg May 18, 2017

Member

Are we really not verifying the replaced binary now?

This comment has been minimized.

@dharmit

dharmit May 18, 2017

Author Collaborator

We're not verifying it by the way of executing minishift version because that doesn't work well on Windows. If you have suggestion(s) around some other way of doing it, I'm open to doing it.

This comment has been minimized.

@coolbrg

coolbrg May 18, 2017

Member

Ack @dharmit , look reasonable to me to go with it. Also, fine with depending on go-update operation as hardy mentioned. If replacing binary fails then, we have the error.

@dharmit dharmit force-pushed the dharmit:fix-72 branch from db9dd89 to 24e7c63 May 19, 2017

Issue #72 Add update command
`minishift update` prompts user to update minishift if a newer version
is available.

@dharmit dharmit force-pushed the dharmit:fix-72 branch from 24e7c63 to 90aad41 May 19, 2017

@coolbrg

This comment has been minimized.

Copy link
Member

coolbrg commented May 19, 2017

Closing this PR as it is superseded by the PR #930.

Thanks @dharmit for the minishift update implementation.

@coolbrg coolbrg closed this May 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.