Skip to content

gh: enable --HEAD install and upgrade notifier#70784

Closed
mislav wants to merge 1 commit intoHomebrew:masterfrom
mislav:gh-updater-enabled
Closed

gh: enable --HEAD install and upgrade notifier#70784
mislav wants to merge 1 commit intoHomebrew:masterfrom
mislav:gh-updater-enabled

Conversation

@mislav
Copy link
Contributor

@mislav mislav commented Feb 9, 2021 •
edited
Loading

This enables the following upgrade notifier when gh notices that it is out of date:

Screen Shot 2021-02-08 at 13 56 41

Ref. cli/cli#2929

As a bonus, this also enables brew install gh --HEAD.

@BrewTestBot BrewTestBot added the go Go use is a significant feature of the PR or issue label Feb 9, 2021
@mislav mislav changed the title gh: enable --head install and upgrade notifier gh: enable --HEAD install and upgrade notifier Feb 9, 2021
carlocab
carlocab reviewed Feb 9, 2021
View reviewed changes
@mislav mislav force-pushed the gh-updater-enabled branch from 88e2bf9 to e3f357a Compare February 9, 2021 17:46
carlocab
carlocab reviewed Feb 9, 2021
View reviewed changes
Formula/gh.rb Outdated
Copy link
Member

@carlocab carlocab Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
revision 1

We need the revision so that existing users get a version with this update.

Copy link
Contributor Author

@mislav mislav Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, but I purposefully left the revision out from my PR, since it's not needed. The feature this enables is only present in the HEAD of the cli/cli repo and is slated for the next release. So I do not want the bottles for gh v1.5.0 to be rebuilt.

Copy link
Member

@carlocab carlocab Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the new feature only works on HEAD builds? Seems like you should wrap the changes in an if build.head? block first, though I guess it's fine if they're a no-op for source builds from the current stable release.

If we're leaving out the revision, then let's be consistent with other formulae:

Suggested change

Copy link
Contributor Author

@mislav mislav Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carlocab I have pushed the removal of the blank line.

The new feature will work for all future stable releases. That's why I don't want to add the if build.head? condition. It's just that the current bottles shouldn't be rebuilt.

@bayandin
Copy link
Contributor

bayandin commented Feb 9, 2021

Probably my comment is about the feature itself than this PR:

@carlocab
Copy link
Member

carlocab commented Feb 9, 2021

This does seem more reliable. The current mechanism seems to assume the new version will be available in Homebrew no later than 24 hours after a new release is tagged. While that is almost always true, it'd be nice to never have to be concerned about the situation where gh is telling users to brew upgrade gh, but brew upgrade gh won't actually do anything (yet).

@mislav
Copy link
Contributor Author

mislav commented Feb 9, 2021 •
edited
Loading

  • brew update part is not required, you can just run brew upgrade gh 😄

I understand, but in our experience, a lot of Homebrew users don't get the auto-updating behavior on install. Then I suggest that they run brew update, and suddenly the latest version is available for them. I suspect that some of these users might have had set HOMEBREW_NO_AUTO_UPDATE=true in their environment and later forgot about it.

We wanted to give users copy-pasteable instructions that are guaranteed to work.

This does seem more reliable. The current mechanism seems to assume the new version will be available in Homebrew no later than 24 hours after a new release is tagged.

For sure, this is a good suggestion, but I feel better if gh is pinging the GitHub API rather than brew.sh to avoid encoding any specific integration with Homebrew's servers. The homebrew-core team has always been fast with merging version bumps to the gh formula, so we're quite fine with the 24h delay.

@mislav mislav force-pushed the gh-updater-enabled branch from e3f357a to 5fe131e Compare February 10, 2021 17:55
carlocab
carlocab approved these changes Feb 10, 2021
View reviewed changes
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @mislav.

@BrewTestBot
Copy link
Contributor

BrewTestBot commented Feb 10, 2021

🤖 A scheduled task has triggered a merge.

@mislav mislav deleted the gh-updater-enabled branch February 10, 2021 19:33
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 13, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@carlocab carlocab carlocab approved these changes

Assignees

No one assigned

Labels

go Go use is a significant feature of the PR or issue outdated PR was locked due to age

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mislav @bayandin @carlocab @BrewTestBot