Add closed PR check to bump cmds #14396
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently we only check for closed PRs in `bump-cask-pr`. This adds that check to `bump` and `bump-formula-pr`. The idea is that this check can warn users about already updated packages or those that can't be updated easily and should be updated manually instead.
|
Review period will end on 2023-01-24 at 00:00:00 UTC. |
BrewTestBot
added
the
waiting for feedback
apainintheneck
commented
Jan 21, 2023
| @@ -26,7 +26,7 @@ def bump_args | |||
| switch "--cask", "--casks", | |||
| description: "Check only casks." | |||
| switch "--open-pr", | |||
| description: "Open a pull request for the new version if there are none already open." | |||
| description: "Open a pull request for the new version if none have been opened yet." | |||
There was a problem hiding this comment.
This is my attempt at clarifying that matching open or closed PRs will block you from opening another one. It's still probably too subtle though.
| @@ -511,7 +511,7 @@ def check_for_duplicate_pull_requests(name, tap_remote_repo, state:, file:, args | |||
| return if pull_requests.blank? | |||
|
|
|||
| duplicates_message = <<~EOS | |||
| These pull requests may be duplicates: | |||
| These #{state} pull requests may be duplicates: | |||
There was a problem hiding this comment.
This is just nice to have.
MikeMcQuaid
approved these changes
Jan 23, 2023
MikeMcQuaid
added
the
critical
BrewTestBot
removed
the
waiting for feedback
|
Review period skipped due to |
BrewTestBot
approved these changes
Jan 23, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?This fixes #14336 where we found out that the
brew bumpcommand was failing when there already existed a closed PR that tried to update the package to the same version. This recommended passing the--forceflag which thebrew bumpcommand doesn't allow. This is expected behavior inbump-formula-prthat wasn't addressed inbrew bump. The change here is to check for closed PRs in thebrew bumpcommand first and not allow users to force any PRs from that command because it's a convenience command forbrew bump-cask-prandbrew bump-formula-pr. It also adds the closed PR check tobrew bump-cask-prto match the formula version of the same command.The reason why we don't want people bumping packages that have a closed PR is because that means it was either already merged in or it requires some sort of extra attention that caused the bump to fail the first time.
One thing to keep in mind is that this will increase the number of times we query the Github API. Once more for
bump-cask-prand once more forbrew bump. We could consider grabbing everything from the Github API and then splitting it into open and closed PRs inbrew bumpwhich would save an API request. The difference is that I think we would end up grabbing all of them meaning most of the data wouldn't be relevant to us (really old closed PRs). I'm not sure it's worth the effort (and I'm lazy).bump
It now checks for the closed PRs unless the user doesn't want us to hit the Github API (
--no-pull-requests) or we've already found a matching open PR or a new version of the package isn't available.No matching PRs
Matching open PR
Matching closed PR
bump-cask-pr
It checks for closed PRs matching the version specified after checking for open ones.
Matching open PR
Matching closed PR