From 67f0cf3ce3646436d17863a5574ec68c5390ffaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 4 Dec 2019 14:34:31 +0100 Subject: [PATCH] Improvements to update notifier - Only report an update available if the version number of the release is greater than the current version - Removes `command` dependency from `update` package; instead, pass current version as an argument - Remove `brew upgrade` instructions since we can't be certain that gh was installed via Homebrew in the first place. - Does not check for updates unless stderr is a tty - Preserve stderr color output even if stdout is not a tty - Fixes stderr color output on Windows --- go.mod | 1 + go.sum | 2 + main.go | 37 ++++++++---- test/fixtures/latestRelease.json | 4 -- update/update.go | 39 ++++++------- update/update_test.go | 99 +++++++++++++++++++++++++------- 6 files changed, 121 insertions(+), 61 deletions(-) delete mode 100644 test/fixtures/latestRelease.json diff --git a/go.mod b/go.mod index f4a0ea38b..c70fb9057 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.13 require ( github.com/AlecAivazis/survey/v2 v2.0.4 + github.com/hashicorp/go-version v1.2.0 github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 github.com/mattn/go-colorable v0.1.2 github.com/mattn/go-isatty v0.0.9 diff --git a/go.sum b/go.sum index b2f2e063e..442678710 100644 --- a/go.sum +++ b/go.sum @@ -13,6 +13,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo= +github.com/hashicorp/go-version v1.2.0 h1:3vNe/fWF5CBgRIguda1meWhsZHy3m8gCJ5wx+dIzX/E= +github.com/hashicorp/go-version v1.2.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= github.com/hinshun/vt10x v0.0.0-20180616224451-1954e6464174 h1:WlZsjVhE8Af9IcZDGgJGQpNflI3+MJSBhsgT5PCtzBQ= github.com/hinshun/vt10x v0.0.0-20180616224451-1954e6464174/go.mod h1:DqJ97dSdRW1W22yXSB90986pcOyQ7r45iio1KN2ez1A= diff --git a/main.go b/main.go index 7aab6377b..67f4b3e3c 100644 --- a/main.go +++ b/main.go @@ -7,13 +7,20 @@ import ( "github.com/github/gh-cli/command" "github.com/github/gh-cli/update" + "github.com/github/gh-cli/utils" + "github.com/mattn/go-isatty" + "github.com/mgutz/ansi" ) var updaterEnabled = "no" func main() { - updateMessageChan := make(chan *string) - go updateInBackground(updateMessageChan) + currentVersion := command.Version + updateMessageChan := make(chan *update.ReleaseInfo) + go func() { + rel, _ := checkForUpdate(currentVersion) + updateMessageChan <- rel + }() if cmd, err := command.RootCmd.ExecuteC(); err != nil { fmt.Fprintln(os.Stderr, err) @@ -24,23 +31,29 @@ func main() { os.Exit(1) } - updateMessage := <-updateMessageChan - if updateMessage != nil { - fmt.Fprintf(os.Stderr, *updateMessage) + newRelease := <-updateMessageChan + if newRelease != nil { + msg := fmt.Sprintf(`A new release of gh is available: %s → %s +Release notes: %s`, currentVersion, newRelease.Version, newRelease.URL) + stderr := utils.NewColorable(os.Stderr) + fmt.Fprintf(stderr, "\n\n%s\n\n", ansi.Color(msg, "cyan")) } } -func updateInBackground(updateMessageChan chan *string) { - if updaterEnabled != "yes" { - updateMessageChan <- nil - return +func shouldCheckForUpdate() bool { + errFd := os.Stderr.Fd() + return updaterEnabled == "yes" && (isatty.IsTerminal(errFd) || isatty.IsCygwinTerminal(errFd)) +} + +func checkForUpdate(currentVersion string) (*update.ReleaseInfo, error) { + if !shouldCheckForUpdate() { + return nil, nil } client, err := command.BasicClient() if err != nil { - updateMessageChan <- nil - return + return nil, err } - updateMessageChan <- update.UpdateMessage(client) + return update.CheckForUpdate(client, "github/homebrew-gh", currentVersion) } diff --git a/test/fixtures/latestRelease.json b/test/fixtures/latestRelease.json deleted file mode 100644 index feb26a258..000000000 --- a/test/fixtures/latestRelease.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "tag_name": "v1.0.0", - "html_url": "https://www.spacejam.com/archive/spacejam/movie/jam.htm" -} diff --git a/update/update.go b/update/update.go index 6a6e3d4e8..a1b9d49a0 100644 --- a/update/update.go +++ b/update/update.go @@ -2,42 +2,35 @@ package update import ( "fmt" - "os" "github.com/github/gh-cli/api" - "github.com/github/gh-cli/command" - "github.com/github/gh-cli/utils" + "github.com/hashicorp/go-version" ) -const nwo = "github/homebrew-gh" - -type releaseInfo struct { +// ReleaseInfo stores information about a release +type ReleaseInfo struct { Version string `json:"tag_name"` URL string `json:"html_url"` } -func UpdateMessage(client *api.Client) *string { - latestRelease, err := getLatestRelease(client) +// CheckForUpdate checks whether this software has had a newer relase on GitHub +func CheckForUpdate(client *api.Client, repo, currentVersion string) (*ReleaseInfo, error) { + latestRelease := ReleaseInfo{} + err := client.REST("GET", fmt.Sprintf("repos/%s/releases/latest", repo), nil, &latestRelease) if err != nil { - fmt.Fprintf(os.Stderr, "%+v", err) - return nil + return nil, err } - updateAvailable := latestRelease.Version != command.Version - if updateAvailable { - alertMsg := fmt.Sprintf(utils.Cyan(` -A new version of gh is available! %s → %s -Changelog: %s -Run 'brew upgrade gh' to upgrade to the latest version!`)+"\n\n", command.Version, latestRelease.Version, latestRelease.URL) - return &alertMsg + if versionGreaterThan(latestRelease.Version, currentVersion) { + return &latestRelease, nil } - return nil + return nil, nil } -func getLatestRelease(client *api.Client) (*releaseInfo, error) { - path := fmt.Sprintf("repos/%s/releases/latest", nwo) - var r releaseInfo - err := client.REST("GET", path, nil, &r) - return &r, err +func versionGreaterThan(v, w string) bool { + vv, ve := version.NewVersion(v) + vw, we := version.NewVersion(w) + + return ve == nil && we == nil && vv.GreaterThan(vw) } diff --git a/update/update_test.go b/update/update_test.go index 9d5a0b869..b5e8db7a4 100644 --- a/update/update_test.go +++ b/update/update_test.go @@ -1,34 +1,89 @@ package update import ( - "os" - "strings" + "bytes" + "fmt" "testing" "github.com/github/gh-cli/api" - "github.com/github/gh-cli/command" ) -func TestRunWhileCheckingForUpdate(t *testing.T) { - originalVersion := command.Version - command.Version = "v0.0.0" - defer func() { - command.Version = originalVersion - }() - - http := &api.FakeHTTP{} - jsonFile, _ := os.Open("../test/fixtures/latestRelease.json") - defer jsonFile.Close() - http.StubResponse(200, jsonFile) - - client := api.NewClient(api.ReplaceTripper(http)) - alertMsg := *UpdateMessage(client) - - if !strings.Contains(alertMsg, command.Version) { - t.Errorf("expected: \"%v\" to contain \"%v\"", alertMsg, command.Version) +func TestCheckForUpdate(t *testing.T) { + scenarios := []struct { + Name string + CurrentVersion string + LatestVersion string + LatestURL string + ExpectsResult bool + }{ + { + Name: "latest is newer", + CurrentVersion: "v0.0.1", + LatestVersion: "v1.0.0", + LatestURL: "https://www.spacejam.com/archive/spacejam/movie/jam.htm", + ExpectsResult: true, + }, + { + Name: "current is prerelease", + CurrentVersion: "v1.0.0-pre.1", + LatestVersion: "v1.0.0", + LatestURL: "https://www.spacejam.com/archive/spacejam/movie/jam.htm", + ExpectsResult: true, + }, + { + Name: "latest is current", + CurrentVersion: "v1.0.0", + LatestVersion: "v1.0.0", + LatestURL: "https://www.spacejam.com/archive/spacejam/movie/jam.htm", + ExpectsResult: false, + }, + { + Name: "latest is older", + CurrentVersion: "v0.10.0-pre.1", + LatestVersion: "v0.9.0", + LatestURL: "https://www.spacejam.com/archive/spacejam/movie/jam.htm", + ExpectsResult: false, + }, } - if !strings.Contains(alertMsg, "v1.0.0") { - t.Errorf("expected: \"%v\" to contain \"%v\"", alertMsg, "v1.0.0") + for _, s := range scenarios { + t.Run(s.Name, func(t *testing.T) { + http := &api.FakeHTTP{} + client := api.NewClient(api.ReplaceTripper(http)) + http.StubResponse(200, bytes.NewBufferString(fmt.Sprintf(`{ + "tag_name": "%s", + "html_url": "%s" + }`, s.LatestVersion, s.LatestURL))) + + rel, err := CheckForUpdate(client, "OWNER/REPO", s.CurrentVersion) + if err != nil { + t.Fatal(err) + } + + if len(http.Requests) != 1 { + t.Fatalf("expected 1 HTTP request, got %d", len(http.Requests)) + } + requestPath := http.Requests[0].URL.Path + if requestPath != "/repos/OWNER/REPO/releases/latest" { + t.Errorf("HTTP path: %q", requestPath) + } + + if !s.ExpectsResult { + if rel != nil { + t.Fatal("expected no new release") + } + return + } + if rel == nil { + t.Fatal("expected to report new release") + } + + if rel.Version != s.LatestVersion { + t.Errorf("Version: %q", rel.Version) + } + if rel.URL != s.LatestURL { + t.Errorf("URL: %q", rel.URL) + } + }) } }