Include latest release version #23
Conversation
cmd/ax/main.go
Outdated
| addAlertCommand = alertCommand.Command("add", "Add new alert") | ||
| version = "dev" | ||
| version = "v0.3.0" |
There was a problem hiding this comment.
So... do we change version manually after releases? Seems fiddly. cc @zefhemel
There was a problem hiding this comment.
That doesn't seem like a good idea
There was a problem hiding this comment.
Reverted to "dev"
cmd/ax/upgrade.go
Outdated
|
|
||
| release, _, err := client.Repositories.GetLatestRelease(context.Background(), "egnyte", "ax") | ||
| if err != nil { | ||
| fmt.Printf("Couldn't get a release information") |
There was a problem hiding this comment.
"Couldn't fetch release information"
cmd/ax/main.go
Outdated
| @@ -23,6 +23,7 @@ var ( | |||
| alertCommand = kingpin.Command("alert", "Be alerted when logs match a query") | |||
| alertDCommand = kingpin.Command("alertd", "Be alerted when logs match a query") | |||
| versionCommand = kingpin.Command("version", "Show the ax version") | |||
| upgrade = kingpin.Command("upgrade", "Verify if there is a new version available") | |||
There was a problem hiding this comment.
Upgrade Ax if a new version is available
cmd/ax/main.go
Outdated
| case "upgrade": | ||
| err := upgradeVersion() | ||
| if err != nil { | ||
| fmt.Println("Upgrade failed.") |
There was a problem hiding this comment.
Maybe you want to actually print the error?
cmd/ax/upgrade.go
Outdated
| func getLatestReleaseData() *github.RepositoryRelease { | ||
| client := github.NewClient(nil) | ||
|
|
||
| release, _, err := client.Repositories.GetLatestRelease(context.Background(), "egnyte", "ax") |
There was a problem hiding this comment.
Would be nice to define egnyte and ax as top-level constants.
cmd/ax/upgrade.go
Outdated
|
|
||
| release, _, err := client.Repositories.GetLatestRelease(context.Background(), "egnyte", "ax") | ||
| if err != nil { | ||
| fmt.Printf("Couldn't get a release information") |
There was a problem hiding this comment.
I would really print the error message here too (%v)
cmd/ax/upgrade.go
Outdated
| for _, element := range release.Assets { | ||
| assetLink := element.GetBrowserDownloadurl("") | ||
| if strings.Contains(assetLink, runtime.GOOS) && strings.Contains(assetLink, runtime.GOARCH) { | ||
| assetDownloadLink = assetLink |
There was a problem hiding this comment.
I'd just instantly return the value rather than keeping it in a variable.
cmd/ax/upgrade.go
Outdated
| return release.GetTagName() | ||
| } | ||
|
|
||
| func downloadFile(filepath string, url string) error { |
cmd/ax/upgrade.go
Outdated
| } | ||
|
|
||
| func upgradeVersion() error { | ||
| if latestVersion := getLatestReleaseTag(); version != latestVersion { |
There was a problem hiding this comment.
I'd probably do this to avoid a giant indented block:
latestVersion := getLatestReleaseTag()
if version == latestVersion {
fmt.Println("Latest version is already installed.")
return
}
// Upgrade case here
There was a problem hiding this comment.
I was considering it as I would the same in Python 😂 Im going to improve that.
cmd/ax/upgrade.go
Outdated
| return err | ||
| } | ||
| defer os.RemoveAll(extractionPath) | ||
| } else { |
There was a problem hiding this comment.
Same here, I'd just exit early (test negative case) to avoid giant if blocks.
cmd/ax/upgrade.go
Outdated
| if strings.ToLower(text) == "yes" || strings.ToLower(text) == "y" { | ||
| latestAssetURL := getLatestAssetLink() | ||
| downloadPath := "/tmp/egnyte_ax/ax.tar.gz" | ||
| extractionPath := "/tmp/egnyte_ax/" |
There was a problem hiding this comment.
https://golang.org/pkg/io/ioutil/#TempDir and TempFile ?
cmd/ax/upgrade.go
Outdated
|
|
||
| // switch old binary with a new one | ||
| currentBinaryPath, _ := osext.Executable() | ||
| err = os.Rename(extractionPath+"ax", currentBinaryPath) |
There was a problem hiding this comment.
Right, I suppose if you use TempFile and TempDir this may not work (rename won't work between different partitions). But you could copy the file and delete it afterwards.
cmd/ax/upgrade.go
Outdated
|
|
||
| func getLatestAssetLink() string { | ||
| release := getLatestReleaseData() | ||
| var assetDownloadLink string |
There was a problem hiding this comment.
Looks like this means that assetDownloadLink could be an empty string.
cmd/ax/upgrade.go
Outdated
| if strings.ToLower(text) == "yes" || strings.ToLower(text) == "y" { | ||
| latestAssetURL := getLatestAssetLink() | ||
| downloadPath := "/tmp/egnyte_ax/ax.tar.gz" | ||
| extractionPath := "/tmp/egnyte_ax/" |
cmd/ax/upgrade.go
Outdated
| if latestVersion := getLatestReleaseTag(); version != latestVersion { | ||
| fmt.Printf("New version detected. Current version: %s. Latest version: %s\n", version, latestVersion) | ||
| inputScanner := bufio.NewScanner(os.Stdin) | ||
| fmt.Print("Do you want to upgrade an Ax to its latest version? (yes/no): ") |
There was a problem hiding this comment.
"Do you want to upgrade Ax to the latest version?"
cmd/ax/upgrade.go
Outdated
| extractionPath := "/tmp/egnyte_ax/" | ||
|
|
||
| if _, err := os.Stat(extractionPath); os.IsNotExist(err) { | ||
| os.Mkdir(extractionPath, 0755) |
There was a problem hiding this comment.
Probably want to at least crash if this fails.
There was a problem hiding this comment.
Actually if you use TempDir this won't matter.
cmd/ax/upgrade.go
Outdated
| // currentBinaryPath, _ := osext.Executable() | ||
| // if err := os.Rename(extractionPath+"ax", currentBinaryPath); err != nil { | ||
| // return err | ||
| // } |
There was a problem hiding this comment.
So we don't actually perform the upgrade right now?
There was a problem hiding this comment.
I was testing something and missed this :( uncommented
|
Code review comments applied. |
fbc5df8 to
e2309c4
Compare
| if err != nil { | ||
| return err | ||
| } | ||
| downloadPath := fmt.Sprintf("%s/ax.tar.gz", extractionPath) |
There was a problem hiding this comment.
This could be still improved but the logic in the downloadFile has to be improved as well.
No description provided.