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

feat: Allow passing one or more --tap options to brew update to allow specific taps to be updated #14530

Closed
wants to merge 2 commits into from

Conversation

boblail
Copy link
Contributor

@boblail boblail commented Feb 6, 2023

Square distributes internal tools through a private tap of Homebrew formulas. It's common for Square engineers to have also tapped other repos of Homebrew formulas, both public and private. We have mechanisms to ensure that our internal tools are up-to-date — and we need to make a strong guarantee that those mechanisms work. However, they can currently fail to brew update our critical tap if any other tap is broken for any reason (e.g. its remote branch was renamed, the user's SSH key is missing, you need to be on a VPN to see some other private tap, etc). A variety of workarounds are at our disposal, but the cleanest of all would be the ability to express to brew update the list of taps we want to update the way we can express to brew install or brew upgrade the list of formulae we care about.


  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

…pecific taps to be updated

Square distributes internal tools through a private tap of Homebrew formulas. It's common for Square engineers to have also tapped other repos of Homebrew formulas, both public and private. We have mechanisms to ensure that our internal tools are up-to-date — and we need to make a strong guarantee that those mechanisms work. However, they can currently fail to `brew update` our critical tap if any other tap is broken for any reason (e.g. its remote branch was renamed, the user's SSH key is missing, you need to be on a VPN to see some other private tap, etc). A variety of workarounds are at our disposal, but the cleanest of all would be the ability to express to `brew update` the list of taps we want to update the way we can express to `brew install` or `brew upgrade` the list of formulae we care about.
if [[ " ${ALLOWLISTED_TAPS[*]} " == *" ${tap} "* ]]
then
DIRS+=("${DIR}")
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: This implementation ignores invalid taps. I think this is correct b/c it's treating "${HOMEBREW_LIBRARY}"/Taps/*/* as the source-of-truth and the users' arguments as a filter, but do you think it should print a warning in this scenario? Like:

$ brew update --tap=homebrew/core --tap=nope/nope
Tap nope/nope is not installed.
You can tap it with brew tap nope/nope.

@MikeMcQuaid
Copy link
Member

@boblail I'm not sure I understand the desire for the feature, can you elaborate on why you'd only want certain taps to be updated and not others? Note that brew update-reset does take specific taps already, that may be a better fit here.

@boblail
Copy link
Contributor Author

boblail commented Feb 6, 2023

@MikeMcQuaid, here's our use-case. We have a bootstrap tool that's foundational, minimal, and idempotent. It runs often. It guarantees that you have access to our private repos, access to our private tap, and current versions of one public and two internal formulas. (Just about everything else that runs on laptops depends on this guarantee.)

The bootstrap tool uses brew update and brew upgrade to update those internal tools. It's quite reliabile, but it's most brittle part is its call to brew update. Most of the time, brew update is failing to update some other tap that the engineer has tapped (e.g. dart-lang/dart, johanhaleby/kubetail, sourcegraph/src-cli). There's nothing wrong with the user having these other taps or using these other tools (they vary from team to team and person to person), but our foundational tool shouldn't break if they break (and shouldn't even care if they exist)!

Ideally, our tool would just brew update homebrew/core and our private tap.


I didn't know about update-reset...! 🤔 I took a look. How does it compare to update? It seems to be a vastly stripped-down version. It's listed as an internal command. On one hand, it might fit our use-case. On the other, would relying on it be like relying on Homebrew's private API? And what would our engineers miss out on if the homebrew/core tap was regularly updated with update-reset instead of update? (I'd like to keep users as close to the yellow-brick road as possible ... while being selective about updates 😄)

@MikeMcQuaid
Copy link
Member

On the other, would relying on it be like relying on Homebrew's private API?

It's documented in man brew so I'd consider it public.

And what would our engineers miss out on if the homebrew/core tap was regularly updated with update-reset instead of update? (I'd like to keep users as close to the yellow-brick road as possible ... while being selective about updates 😄)

They will miss out on the updates, reporting, rename migrations, etc. that update provides.

Most of the time, brew update is failing to update some other tap that the engineer has tapped (e.g. dart-lang/dart, johanhaleby/kubetail, sourcegraph/src-cli)

Does this not break e.g. auto-updates too?


As-is I'm afraid I'm leaning towards declining this. I just don't see a good reason to add a new command when update-reset can do something along these lines and brew update specifically and intentionally wants to try to update everything and error otherwise.

@boblail
Copy link
Contributor Author

boblail commented Feb 6, 2023

@MikeMcQuaid, how about if I try out using update-reset in our tool for a week or two, gather data, and report back here on whether it had any unintended side-effects?

@carlocab
Copy link
Member

carlocab commented Feb 7, 2023

Maybe it makes more sense to prevent brew update from bailing entirely when it encounters a broken tap? It can give up on the broken tap, but it should go on to update others where it can.

@MikeMcQuaid
Copy link
Member

@boblail Works for me!

Maybe it makes more sense to prevent brew update from bailing entirely when it encounters a broken tap? It can give up on the broken tap, but it should go on to update others where it can.

@carlocab Yeh, this makes sense to me too 👍🏻

@nemith
Copy link
Contributor

nemith commented Feb 16, 2023

I have a very similar (or exact) need and also didn't know update-reset.

However I am not sure how to use update-reset. It seems to do nothing with no errors.

$ brew tap -v
capicuadev/sshpass
homebrew/cask-fonts
homebrew/services
jandedobbeleer/oh-my-posh
company/tools
withgraphite/tap
$ brew update-reset company/tools
$ brew update-reset https://internal.repo.server/company/homebrew-tools.git

@carlocab
Copy link
Member

However I am not sure how to use update-reset. It seems to do nothing with no errors.

brew update-reset company/tools should work, but it doesn't for some reason. Try

brew update-reset "$(brew --repo company/tools)"

But we really should fix that.

@carlocab
Copy link
Member

But we really should fix that.

#14667

@MikeMcQuaid
Copy link
Member

I have a very similar (or exact) need and also didn't know update-reset.

@nemith Can you elaborate on why you have this need? What are the things that are causing brew update to fail?

@nemith
Copy link
Contributor

nemith commented Feb 16, 2023

Yeah, like @boblail I am investigating using homebrew to distribute internal tools however we have a private instance of Github Enterprise which is only accessible on VPN.

Out-of-date tools is more of a concern as we want to do releases often and a lot of our users are will not be power homebrew users and automatically updating. So to combat out-of-date packages we want to include a outdated_check into the binary itself to tell the user that a new version is available.

This needs to be fast and quick so updating just a our single repo and then doing a brew outdated is one way to do this check. The other option is to do the check outside of homebrew entirely which is less ideal.

The other side of this is when off vpn the tools should still work and maybe not error out on the unaccessable repo, but that is less of a concern right now.

Most of this is in the design stage.

@MikeMcQuaid
Copy link
Member

However, they can currently fail to brew update our critical tap if any other tap is broken for any reason (e.g. its remote branch was renamed, the user's SSH key is missing, you need to be on a VPN to see some other private tap, etc)

@boblail @nemith I tested this out and cannot reproduce this behaviour. As far as I see, if brew update fails to update a specific tap: it'll still update all the other taps and just complain (rightfully) about the tap it failed to update and set a failed exit code.

I don't understand what the desired behaviour is here?

@MikeMcQuaid
Copy link
Member

@boblail @nemith gentle nudge on the above!

@nemith
Copy link
Contributor

nemith commented Feb 22, 2023

@carlocab was the one who suggested not failing on update. That isn't the behavior/issue I was describing so I have less of an issue there.

There is a bit of an issue with the error messaging in the fact it's "fatal" errors which give the impression that not all repos are fetched but I guess that may not be something you desire to fix.

brandonbennett@HQ-QX0FQ102LC ~ % brew update; echo $?  
fatal: unable to access 'https://github.company.com/brandonbennett/homebrew-tools.git/': Failed to connect to github.company.com port 443 after 5 ms: Couldn't connect to server
Error: Fetching /opt/homebrew/Library/Taps/company/homebrew-nettools failed!
1

My specific issue was being able to check for outdated recipes quickly during tool invocation which brew update is faster but not fast enough. Like i said before I am still evaulating solutions (being able to install other versions or canary versions is also important to me) so perhaps I have a round hole that i am trying to shove a square peg through.

Feel free to disregard my comments.

@MikeMcQuaid
Copy link
Member

There is a bit of an issue with the error messaging in the fact it's "fatal" errors which give the impression that not all repos are fetched but I guess that may not be something you desire to fix.

This is output by git rather than brew and we can't alter the messaging there, only silence it (which I think would cause more harm than good).

My specific issue was being able to check for outdated recipes quickly during tool invocation which brew update is faster but not fast enough. Like i said before I am still evaulating solutions (being able to install other versions or canary versions is also important to me) so perhaps I have a round hole that i am trying to shove a square peg through.

Feel free to disregard my comments.

Yeh, not sure we can help there, sorry!

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Mar 16, 2023
@MikeMcQuaid
Copy link
Member

Passing on this, sorry!

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants