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.

Sign up for GitHub

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

Jump to bottom

Use gh to automerge and cleanup branches on bump* PRs. #15784

Closed
wants to merge 1 commit into from
Closed

Use gh to automerge and cleanup branches on bump* PRs. #15784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Library/Homebrew/dev-cmd/bump-cask-pr.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ def bump_cask_pr_args
description: "Print the pull request URL instead of opening in a browser."
switch "--no-fork",
description: "Don't try to fork the repository."
switch "--automerge",
env: :bump_automerge,
description: "Open pull requests with automerge and branch cleanup enabled."
flag "--version=",
description: "Specify the new <version> for the cask."
flag "--version-arm=",
Expand All @@ -58,6 +61,7 @@ def bump_cask_pr_args
conflicts "--no-audit", "--online"
conflicts "--version=", "--version-arm="
conflicts "--version=", "--version-intel="
conflicts "--"
Copy link
Member

Choose a reason for hiding this comment

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

❓

Suggested change
conflicts "--"

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, needs updated.


named_args :cask, number: 1, without_api: true
end
Expand Down
3 changes: 3 additions & 0 deletions Library/Homebrew/dev-cmd/bump-formula-pr.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ def bump_formula_pr_args
description: "Print the pull request URL instead of opening in a browser."
switch "--no-fork",
description: "Don't try to fork the repository."
switch "--automerge",
env: :bump_automerge,
description: "Open pull requests with automerge and branch cleanup enabled."
Comment on lines +51 to +53
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this works as-is yet, since bump-formula-pr PRs will still need a bottle commit pushed, so I'm not sure we want to enable automerge on them. In theory doing so should do nothing given the bottle checks in the merge queue though.

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlocab Oops, I've misunderstood the workflow. Does this make this PR basically pointless for homebrew/core (and/or homebrew/core)?

Copy link
Member

@carlocab carlocab Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

At the moment, it is pretty pointless for Homebrew/core. I can see this being useful if/when PRs are blocked from being queued to merge if they are still missing a bottle commit (and require one).

Copy link
Member Author

Choose a reason for hiding this comment

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

@Homebrew/cask would this be useful for homebrew/cask?

Copy link
Member

Choose a reason for hiding this comment

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

Cask has automation for this I believe

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, homebrew/cask* already auto-enables automerge for all maintainer-authored PRs labelled bump-cask-pr: https://github.com/Homebrew/homebrew-cask/blob/master/.github/workflows/automerge.yml.

Example: Homebrew/homebrew-cask#152038

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, problem solved then, thanks all.

comma_array "--mirror",
description: "Use the specified <URL> as a mirror URL. If <URL> is a comma-separated list " \
"of URLs, multiple mirrors will be added."
Expand Down
9 changes: 7 additions & 2 deletions Library/Homebrew/dev-cmd/bump.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
description: "Check only casks."
switch "--open-pr",
description: "Open a pull request for the new version if none have been opened yet."
switch "--automerge",
env: :bump_automerge,
description: "Open pull requests with automerge and branch cleanup enabled."
flag "--limit=",
description: "Limit number of package results returned."
flag "--start-with=",
Expand Down Expand Up @@ -288,7 +291,9 @@
return if open_pull_requests
return if closed_pull_requests

system HOMEBREW_BREW_FILE, "bump-#{type}-pr", "--no-browse",
"--message=Created by `brew bump`", "--version=#{new_version}", name
bump_args = ["--no-browse", "--message=Created by `brew bump`", "--version=#{new_version}"]

Check warning on line 294 in Library/Homebrew/dev-cmd/bump.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/bump.rb#L294 

Added line #L294 was not covered by tests
bump_args << "--automerge" if args.automerge?

system HOMEBREW_BREW_FILE, "bump-#{type}-pr", *bump_args, name

Check warning on line 297 in Library/Homebrew/dev-cmd/bump.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/bump.rb#L297 

Added line #L297 was not covered by tests
end
end
5 changes: 5 additions & 0 deletions Library/Homebrew/env_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ module EnvConfig
description: "Use this as the browser when opening project homepages.",
default_text: "`$BROWSER` or the OS's default browser.",
},
HOMEBREW_BUMP_AUTOMERGE: {
description: "If set, `brew bump*` commands will attempt to enable automerge and branch cleanup. " \
"This will only work if you have `gh` installed and write access on the relevant repository.",
boolean: true,
},
HOMEBREW_CACHE: {
description: "Use this directory as the download cache.",
default_text: "macOS: `$HOME/Library/Caches/Homebrew`, " \
Expand Down
20 changes: 12 additions & 8 deletions Library/Homebrew/utils/github.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,17 +682,21 @@
EOS
end

begin
url = create_pull_request(tap_remote_repo, commit_message,
"#{username}:#{branch}", remote_branch, pr_message)["html_url"]
if args.no_browse?
puts url
else
exec_browser url
end
url = begin
create_pull_request(tap_remote_repo, commit_message,

Check warning on line 686 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L686 

Added line #L686 was not covered by tests
"#{username}:#{branch}", remote_branch, pr_message)["html_url"]
rescue *API::ERRORS => e
odie "Unable to open pull request: #{e.message}!"
end

# Attempt to enable automerge on the created PR if requested.
GitHub::API.run_gh_cli(args: ["pr", "merge", "--auto", "--merge", "--delete-branch", url]) if args.automerge?
Copy link
Member

@Bo98 Bo98 Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason we do this rather than call the API directly like we do everywhere else?

This sounds like this would not work with HOMEBREW_GITHUB_API_TOKEN.

Code using the GraphQL directly (untested but should work in theory if no typos):

pr = create_pull_request(...)
url = pr["html_url"]

# ...

if args.automerge?
  API.open_graphql(<<~EOS, variables: { pullRequestId: pr["node_id"] })
    mutation($pullRequestId: ID!) {
      enablePullRequestAutoMerge(input: {pullRequestId: $pullRequestId, mergeMethod: MERGE}) {
        clientMutationId
      }
    }
  EOS
end

Copy link
Member

Choose a reason for hiding this comment

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

See also similar-ish code in Homebrew/core: https://github.com/Homebrew/homebrew-core/blob/f0c6e070add5312cb25639bcade628c21a6de2af/.github/workflows/publish-commit-bottles.yml#L211-L232

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 pretty much because:

  • we already have code to shell out to gh
  • we already rely on gh for homebrew/core
  • honestly for anything that's not performance-critical: I'd rather we pushed more of our code into gh calls than doing direct API access.

If you don't consider this a hard blocker: would rather land this as-is to validate the idea/workflow and anyone who wants to can move to GraphQL later if that's desired.

Copy link
Member

@Bo98 Bo98 Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we already have code to shell out to gh

This appears to be the first beyond token fetching?

I'm not entirely convinced it'll work in Homebrew/core CI as is given how our tokens appear to be used over there (plus that additional issue Carlo pointed out), but have of course not tested it. Maybe Homebrew/core CI is a non-concern anyway judging by the original issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

This appears to be the first beyond token fetching?

Yes.

I'm not entirely convinced it'll work in Homebrew/core CI as is given how our tokens appear to be used over there

Yes, may require extra wrangling for the bump job(s).

Maybe Homebrew/core CI is a non-concern anyway judging by the original issue.

Indeed, this may be rescoped to cask only.


if args.no_browse?

Check warning on line 695 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L695 

Added line #L695 was not covered by tests
puts url
else
exec_browser url
end
end
end
end
Expand Down
21 changes: 14 additions & 7 deletions Library/Homebrew/utils/github/api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,18 +123,25 @@
JSON::ParserError,
].freeze

sig {
params(args: T::Array[String], print_stdout: T::Boolean,

Check warning on line 127 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L127 

Added line #L127 was not covered by tests
print_stderr: T::Boolean).returns(SystemCommand::Result)
}
def self.run_gh_cli(args:, print_stdout: true, print_stderr: true)
# Avoid `Formula["gh"].opt_bin` so this method works even with `HOMEBREW_DISABLE_LOAD_FORMULA`.
env = { "PATH" => PATH.new(HOMEBREW_PREFIX/"opt/gh/bin", ENV.fetch("PATH")) }
system_command "gh", args: args, env: env, print_stdout: print_stdout, print_stderr: print_stderr

Check warning on line 133 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L132-L133 

Added lines #L132 - L133 were not covered by tests
end

# Gets the token from the GitHub CLI for github.com.
sig { returns(T.nilable(String)) }
def self.github_cli_token
# Avoid `Formula["gh"].opt_bin` so this method works even with `HOMEBREW_DISABLE_LOAD_FORMULA`.
env = { "PATH" => PATH.new(HOMEBREW_PREFIX/"opt/gh/bin", ENV.fetch("PATH")) }
gh_out, _, result = system_command "gh",
args: ["auth", "token", "--hostname", "github.com"],
env: env,
print_stderr: false
result = run_gh_cli args: ["auth", "token", "--hostname", "github.com"],

Check warning on line 139 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L139 

Added line #L139 was not covered by tests
print_stdout: false,
print_stderr: false
return unless result.success?

gh_out.chomp
result.stdout.chomp

Check warning on line 144 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L144 

Added line #L144 was not covered by tests
end

# Gets the password field from `git-credential-osxkeychain` for github.com,
Expand Down
3 changes: 3 additions & 0 deletions completions/bash/brew
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ _brew_bump() {
case "${cur}" in
-*)
__brewcomp "
--automerge
--cask
--debug
--formula
Expand All @@ -461,6 +462,7 @@ _brew_bump_cask_pr() {
case "${cur}" in
-*)
__brewcomp "
--automerge
--commit
--debug
--dry-run
Expand Down Expand Up @@ -494,6 +496,7 @@ _brew_bump_formula_pr() {
case "${cur}" in
-*)
__brewcomp "
--automerge
--commit
--debug
--dry-run
Expand Down
3 changes: 3 additions & 0 deletions completions/fish/brew.fish
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@ __fish_brew_complete_arg 'bottle' -a '(__fish_brew_suggest_formulae_installed)'


__fish_brew_complete_cmd 'bump' 'Display out-of-date brew formulae and the latest version available'
__fish_brew_complete_arg 'bump' -l automerge -d 'Open pull requests with automerge and branch cleanup enabled'
__fish_brew_complete_arg 'bump' -l cask -d 'Check only casks'
__fish_brew_complete_arg 'bump' -l debug -d 'Display any debugging information'
__fish_brew_complete_arg 'bump' -l formula -d 'Check only formulae'
Expand All @@ -395,6 +396,7 @@ __fish_brew_complete_arg 'bump; and not __fish_seen_argument -l formula -l formu


__fish_brew_complete_cmd 'bump-cask-pr' 'Create a pull request to update cask with a new version'
__fish_brew_complete_arg 'bump-cask-pr' -l automerge -d 'Open pull requests with automerge and branch cleanup enabled'
__fish_brew_complete_arg 'bump-cask-pr' -l commit -d 'When passed with `--write-only`, generate a new commit after writing changes to the cask file'
__fish_brew_complete_arg 'bump-cask-pr' -l debug -d 'Display any debugging information'
__fish_brew_complete_arg 'bump-cask-pr' -l dry-run -d 'Print what would be done rather than doing it'
Expand All @@ -419,6 +421,7 @@ __fish_brew_complete_arg 'bump-cask-pr' -a '(__fish_brew_suggest_casks_all)'


__fish_brew_complete_cmd 'bump-formula-pr' 'Create a pull request to update formula with a new URL or a new tag'
__fish_brew_complete_arg 'bump-formula-pr' -l automerge -d 'Open pull requests with automerge and branch cleanup enabled'
__fish_brew_complete_arg 'bump-formula-pr' -l commit -d 'When passed with `--write-only`, generate a new commit after writing changes to the formula file'
__fish_brew_complete_arg 'bump-formula-pr' -l debug -d 'Display any debugging information'
__fish_brew_complete_arg 'bump-formula-pr' -l dry-run -d 'Print what would be done rather than doing it'
Expand Down
3 changes: 3 additions & 0 deletions completions/zsh/_brew
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ _brew_bottle() {
# brew bump
_brew_bump() {
_arguments \
'--automerge[Open pull requests with automerge and branch cleanup enabled]' \
'--debug[Display any debugging information]' \
'--full-name[Print formulae/casks with fully-qualified names]' \
'--help[Show this message]' \
Expand All @@ -512,6 +513,7 @@ _brew_bump() {
# brew bump-cask-pr
_brew_bump_cask_pr() {
_arguments \
'--automerge[Open pull requests with automerge and branch cleanup enabled]' \
'--commit[When passed with `--write-only`, generate a new commit after writing changes to the cask file]' \
'--debug[Display any debugging information]' \
'(--write)--dry-run[Print what would be done rather than doing it]' \
Expand Down Expand Up @@ -539,6 +541,7 @@ _brew_bump_cask_pr() {
# brew bump-formula-pr
_brew_bump_formula_pr() {
_arguments \
'--automerge[Open pull requests with automerge and branch cleanup enabled]' \
'--commit[When passed with `--write-only`, generate a new commit after writing changes to the formula file]' \
'--debug[Display any debugging information]' \
'(--write-only)--dry-run[Print what would be done rather than doing it]' \
Expand Down
9 changes: 9 additions & 0 deletions docs/Manpage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,8 @@ formulae, also displays whether a pull request has been opened with the URL.
Check only casks.
* `--open-pr`:
Open a pull request for the new version if none have been opened yet.
* `--automerge`:
Open pull requests with automerge and branch cleanup enabled.
* `--limit`:
Limit number of package results returned.
* `--start-with`:
Expand Down Expand Up @@ -1046,6 +1048,8 @@ supplied by the user.
Print the pull request URL instead of opening in a browser.
* `--no-fork`:
Don't try to fork the repository.
* `--automerge`:
Open pull requests with automerge and branch cleanup enabled.
* `--version`:
Specify the new *`version`* for the cask.
* `--version-arm`:
Expand Down Expand Up @@ -1098,6 +1102,8 @@ nor vice versa. It must use whichever style specification the formula already us
Print the pull request URL instead of opening in a browser.
* `--no-fork`:
Don't try to fork the repository.
* `--automerge`:
Open pull requests with automerge and branch cleanup enabled.
* `--mirror`:
Use the specified *`URL`* as a mirror URL. If *`URL`* is a comma-separated list of URLs, multiple mirrors will be added.
* `--fork-org`:
Expand Down Expand Up @@ -2116,6 +2122,9 @@ example, run `export HOMEBREW_NO_INSECURE_REDIRECT=1` rather than just

*Default:* `$BROWSER` or the OS's default browser.

- `HOMEBREW_BUMP_AUTOMERGE`
<br>If set, `brew bump*` commands will attempt to enable automerge and branch cleanup. This will only work if you have `gh` installed and write access on the relevant repository.

- `HOMEBREW_CACHE`
<br>Use this directory as the download cache.

Expand Down
18 changes: 18 additions & 0 deletions manpages/brew.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1450,6 +1450,10 @@ Check only casks\.
Open a pull request for the new version if none have been opened yet\.
.
.TP
\fB\-\-automerge\fR
Open pull requests with automerge and branch cleanup enabled\.
.
.TP
\fB\-\-limit\fR
Limit number of package results returned\.
.
Expand Down Expand Up @@ -1496,6 +1500,10 @@ Print the pull request URL instead of opening in a browser\.
Don\'t try to fork the repository\.
.
.TP
\fB\-\-automerge\fR
Open pull requests with automerge and branch cleanup enabled\.
.
.TP
\fB\-\-version\fR
Specify the new \fIversion\fR for the cask\.
.
Expand Down Expand Up @@ -1575,6 +1583,10 @@ Print the pull request URL instead of opening in a browser\.
Don\'t try to fork the repository\.
.
.TP
\fB\-\-automerge\fR
Open pull requests with automerge and branch cleanup enabled\.
.
.TP
\fB\-\-mirror\fR
Use the specified \fIURL\fR as a mirror URL\. If \fIURL\fR is a comma\-separated list of URLs, multiple mirrors will be added\.
.
Expand Down Expand Up @@ -3041,6 +3053,12 @@ Use this as the browser when opening project homepages\.
\fIDefault:\fR \fB$BROWSER\fR or the OS\'s default browser\.
.
.TP
\fBHOMEBREW_BUMP_AUTOMERGE\fR
.
.br
If set, \fBbrew bump*\fR commands will attempt to enable automerge and branch cleanup\. This will only work if you have \fBgh\fR installed and write access on the relevant repository\.
.
.TP
\fBHOMEBREW_CACHE\fR
.
.br
Expand Down
Loading