search

Homebrew / brew

🍺 The missing package manager for macOS (or Linux)
https://brew.sh
BSD 2-Clause "Simplified" License
41.43k stars 9.75k forks source link

Add more flags to `brew bump --open-pr` to match `bump-*-pr` #13171

Closed nihaals closed 1 year ago

nihaals commented 2 years ago

Provide a detailed description of the proposed feature

See: #13166 and https://github.com/Homebrew/brew/issues/13166#issuecomment-1104954203

On Homebrew 3.4.7:

brew bump --help ``` Usage: brew bump [options] [formula|cask ...] Display out-of-date brew formulae and the latest version available. If the returned current and livecheck versions differ or when querying specific formulae, also displays whether a pull request has been opened with the URL. --full-name Print formulae/casks with fully-qualified names. --no-pull-requests Do not retrieve pull requests from GitHub. --formula, --formulae Check only formulae. --cask, --casks Check only casks. --open-pr Open a pull request for the new version if there are none already open. --limit Limit number of package results returned. --start-with Letter or word that the list of package results should alphabetically follow. -d, --debug Display any debugging information. -q, --quiet Make some output more quiet. -v, --verbose Make some output more verbose. -h, --help Show this message. ```
brew bump-cask-pr --help ``` Usage: brew bump-cask-pr [options] cask Create a pull request to update cask with a new version. A best effort to determine the SHA-256 will be made if the value is not supplied by the user. -n, --dry-run Print what would be done rather than doing it. --write-only Make the expected file modifications without taking any Git actions. --commit When passed with --write-only, generate a new commit after writing changes to the cask file. --no-audit Don't run brew audit before opening the PR. --online Run brew audit --online before opening the PR. --no-style Don't run brew style --fix before opening the PR. --no-browse Print the pull request URL instead of opening in a browser. --no-fork Don't try to fork the repository. --version Specify the new version for the cask. --message Append message to the default pull request message. --url Specify the URL for the new download. --sha256 Specify the SHA-256 checksum of the new download. --fork-org Use the specified GitHub organization for forking. -f, --force Ignore duplicate open PRs. -d, --debug Display any debugging information. -q, --quiet Make some output more quiet. -v, --verbose Make some output more verbose. -h, --help Show this message. ```

Part of this issue is also documenting what brew bump actually does/how it differs from brew bump-cask-pr. From flags like --no-audit, --no-style, --no-browse and --fork-org, it's fairly clear what bump-cask-pr actually does, whereas bump only gives a high level overview. It isn't clear if it runs style, audit or opens the PR in your browser (or even creates the PR for you vs. taking you to open the PR in your browser). Adding a --dry-run could help with this, and some of these additional flags are exclusive to --open-pr, which might make reading the manual/--help more difficult.

What is the motivation for the feature?

From #13166 (referring to adding a brew bumo-cask-pr --livecheck flag):

Currently if you want to bump a cask's version you need to get the latest version either from the site/app or from brew livecheck/brew bump. Since Homebrew has access to the current latest version already with the livecheck, passing the actual version shouldn't need to be required when bumping. You can currently use brew livecheck --json to make your own function that passes it for you, but this adds complexity for a new user to use it.

How will the feature be relevant to at least 90% of Homebrew users?

From #13166:

This is for users who update casks, but for those users it will make it a faster process for anyone who sees an app update themselves and wants to PR a version bump.

What alternatives to the feature have been considered?

From https://github.com/Homebrew/brew/issues/13166#issuecomment-1104501806:

  • Add livecheck to bump-cask-pr
  • Add more flags to brew bump --open-pr
  • Try to merge the commands in some way
apainintheneck commented 2 years ago

Internally brew bump --open-pr delegates to brew bump-cask-pr or brew bump-formula-pr if a newer version of the cask or formula exists without an open pull request. You are right in suggesting that it doesn't currently allow you to pass flags to those commands though.

This is the full command that gets called internally when using brew bump --open-pr.

brew bump-[formula|cask]-pr --no-browse --message-"Created by `brew bump`"
--version=[new version] [formula|cask name]
MikeMcQuaid commented 2 years ago

Makes sense to me!

apainintheneck commented 2 years ago

Since brew bump is delegating to those other commands internally, wouldn't the simplest solution be to just pass those arguments directly to the other command instead of adding more flags to brew bump. Something like the following...

brew bump --open-pr="--no-browse --no-style --fork-org --no-audit"

Then, validation of those arguments would just get handled by the next command. Just a thought.

nihaals commented 2 years ago 
brew bump --open-pr="--no-browse --no-style --fork-org --no-audit"

That does sound like an interesting and simple solution, although it doesn't feel the "smoothest" e.g. breaks shell completions.

MikeMcQuaid commented 2 years ago

Then, validation of those arguments would just get handled by the next command.

I think it'd be nicer to have the validation and documentation handled by the first command and also to add only the specific arguments here that @nihaals or whoever needs rather than all of them.

apainintheneck commented 2 years ago

@nihaals Could you list the specific arguments you'd want added to brew bump?

nihaals commented 2 years ago

Could you list the specific arguments you'd want added to brew bump?

@apainintheneck I think these make the most sense to me:

apainintheneck commented 2 years ago

I'm not sure about the --no-style option because that option is only used with bump-cask-pr and not bump-formula-pr but the other ones sound reasonable.

MikeMcQuaid commented 2 years ago
  • --no-audit
  • --no-style

I'd rather we didn't proliferate these too much further.

  • --no-fork
  • --fork-org

Why do you need both of these?

  • --dry-run?

This will be a non-trivial lift to add.

  • --online?

This tool doesn't make sense offline.

nihaals commented 2 years ago

I agree with the points about the other flags.

  • --no-fork
  • --fork-org

Why do you need both of these?

I thought I ran into an issue where because I used a GitHub token with just public_repo (as I had already forked the repo, added the remote and was using SSH), I was getting authorisation errors and needed to use --no-fork, although I didn't have this issue when using bump --open-pr earlier today.

As for --fork-org, I assume Homebrew uses the owner of the token as the owner of the fork, but there may be a case where someone does want to use an org fork instead of a personal fork. I haven't had a chance to look at the source yet to see how Homebrew looks at existing remotes/what it expects the remote to be called and the logic it uses for checking for existing forks, but if it always assumes a personal fork, then someone might need this flag.

Since I haven't looked at the source yet, I also haven't seen how much --no-fork saves in cases where the fork already exists and the remote is already added, but I assume the flag saves a single API request, which probably doesn't matter for a PAT created just for Homebrew, so it could probably be left.

  • --online?

This tool doesn't make sense offline.

Should --online always be passed to brew bump-[formula|cask]-pr then?

MikeMcQuaid commented 2 years ago

Should --online always be passed to brew bump-[formula|cask]-pr then?

Probably, yes.

In general, I'd like to see more of these flags replaced with better handling/defaults rather than just passing them through.

nihaals commented 2 years ago

Hopefully this makes it easier to keep track of each flag:

bump-cask-pr flag Add to bump? Notes
--dry-run Out of scope
--write-only
--commit
--no-audit :x: https://github.com/Homebrew/brew/issues/13171#issuecomment-1152448644
--online :x: https://github.com/Homebrew/brew/issues/13171#issuecomment-1154841214
--no-style :x: https://github.com/Homebrew/brew/issues/13171#issuecomment-1151422766, https://github.com/Homebrew/brew/issues/13171#issuecomment-1152448644
--browse
--no-fork
--fork-org
--force Covered by --no-pull-requests?
Porkepix commented 2 years ago

--force isn't covered by --no-pull-requests if you want to open the PR: Error: Invalid usage: Options --no-pull-requests and --open-pr are mutually exclusive.

Which means when it find irrelevant opened PR, you currently can't force it to open one from brew bump.

Also, I don't don't see --version listed, which make brew bump unusable on many throttled formulas, excepted if it can calculate the latest acceptable version in the interval rather than the latest, or if throttled formulas would accept any version not multiple of X where X is the throttle, but any version with an interval of more than X instead.

MikeMcQuaid commented 1 year ago

Passing on this. We'll review a PR but not going to leave it open indefinitely, sorry!