We now use mixed libraries for checking IsTerminal: golang.org/x/crypto/ssh/terminal and github.com/mattn/go-isatty. If I'm not mistaken, the latter is written in pure Go and works without CGO, but until CGO becomes a problem for us, maybe we can consider consistently using the former since it's more "official"?

I don't have a strong opinion on this, so I'll change the few places we use go-isatty to use the terminal lib instead.

I got rid of our dependency for go-isatty but it turns out that github.com/AlecAivazis/survey creates an indirect dependency. So we still have it in our go.mod file ¯_(ツ)_/¯

Is "lastest" a typo or a cute misspelling :3

This will print to stdout, which might mess up someone script if they're piping e.g. gh pr list to somewhere else other than to a terminal. Can we ensure that this goes to stderr, or—even better—that the caller to CheckForUpdate() can decide which output stream the message goes to?

I assumed that that the check on line 17 terminal.IsTerminal would handle this case. Wouldn't CheckForUpdate do an early return if gh was piped somewhere?

Can we get the release URL from the release API record (since we've just queried it) instead of manually constructing it?

It surprised me that this functionality is in the command package, since it doesn't seem to be a command at all. If we follow the philosophy that seems popular in the Go community that “oversplitting” is generally better than large packages, can we consider putting the updater functionality into its own package?

I'll move it into it's own package. At one point I was relying on private methods from command and that's why it was moved in there, but it's all self-contained now 👍🏼

Should we consider using our own api package to handle some of the HTTP stuff for us? We could add functionalty to make REST calls in addition to existing GraphQL. We can also use that for forking, which seems at the time only available via REST.

Additionally, I think we should try to use the user's API token (if available) and set the appropriate User-Agent string. This would ensure that these lookups are subject to a more permissive rate limit and that they are appropriately tracked on our end as CLI activity.

I started down this road, but quickly exited out for three main reasons:

1. This calls an api endpoint on a public repo so we can call it with a plain GET call
2. Making a generic REST endpoint based on this one call seemed like overkill because it wouldn't take advantage of much of the api package
3. Because we only plan on running this once a day I wasn't concerned about rate limiting.

Once we have another REST endpoint I think it makes sense to move to the api package and makes something more generic, but for now I felt like it was too much code before we need it.

I wonder if the version comparison should be more sophisticated, i.e. implement true semver comparison. If the current gh version was a prerelease, e.g. v1.0-beta1, and the latest stable version was v0.9, this will result in the message:

A new version of gh is available! v1.0-beta1 → v0.9

This was an effort to minimize the complexity of the code. For now I thought the simple approach is better because we have a very simple release pattern (we just increment the release number). I may be naive in thinking this pattern will last us for awhile, so if you think we should get this in before it becomes a problem I can change my mind about it.

Forgot to point out: love the async implementation of this! Great job 👏

I played around with a few variations of this. It still feels weird to me, and I still have worries about it accidentally forgetting to send a message and hanging the main thread. But ¯_(ツ)_/¯

We definitely want people to in general use brew upgrade gh (no github/gh/ prefix necessary) instead of reinstall

👍

Nit: perhaps configuration should be passed down by the caller, not decided from within the update package. A good rule of thumb to be asking ourselves when extracting code into packages is: would this package be theoretically reusable in another application?

This is a great idea and I like the reasoning behind it.

I don't think it's worth updating utils and ui packages since they're basically dead code; we should clean them up instead.

It looks like we are still using the color code in util to color some text.

I ended up writing a REST method for our client struct. Because it adds the token and user-agent headers like @mislav recommended. But mostly because it made it easier to test because of the RoundTrip stuff.

Because we are using the client from the update package I needed to make a public way to create the client with a context.

I pulled the goroutine/channel stuff out of the update package and put it in main, this made things a littler easier to deal with I think.

I wasn't sure what to do with this error, it felt like a bad idea to let it die silently. But it also felt pretty nasty to just dump it to stderr ¯_(ツ)_/¯

This reads the variable from the user's environment at runtime, which most likely will never be set (or, possibly, might be set for a purpose different from GitHub CLI).

The release action sets the APP_ENV variable during build, but that affects nothing, since APP_ENV is checked at runtime, not at build time.

I would suggest that instead of using an environment variable, we check for a package variable that is by default disabled:

package main

var updaterEnabled = ""

func main() {
  if updaterEnabled == "yes" {
    // kick off updater logic
  }
}

And then enable that using build flags:

ldflags:
- -X github.com/github/gh-cli.updaterEnabled=yes

GAH! I forget I'm living in a compiled app world now! Thanks