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

bump-formula-pr: Replace `hub` with GH API calls #3870

Merged
merged 2 commits into from Apr 8, 2018

Conversation

Projects
None yet
4 participants
@GauthamGoli
Copy link
Member

GauthamGoli commented Mar 3, 2018

  • 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 tests with your changes locally?

Replace hub in bump-formula-pr with calls to GH API to fork and to open pull request.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:bump-formula-gh-api branch 2 times, most recently from 0efa0ec to 1b0052b Mar 3, 2018

formula.path.atomic_write(backup_file) unless ARGV.dry_run?
odie "cannot get remote from 'hub'!"
end
response = GitHub.create_fork(formula.tap.full_name)

This comment has been minimized.

@ilovezfs

ilovezfs Mar 3, 2018

Contributor

I assume this can fail? In which case, the backup file needs to be restored, I think.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 3, 2018

Member

Added fallbacks to exceptions now. Thanks 👍

@@ -29,10 +29,6 @@
#: If `--message=`<message> is passed, append <message> to the default PR
#: message.
#:
#: If `--no-browse` is passed, don't pass the `--browse` argument to `hub`

This comment has been minimized.

@ilovezfs

ilovezfs Mar 3, 2018

Contributor

Are we going to replace the browser functionality?

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 3, 2018

Member

Do we want to ? I thought that browser functionality was due to hub.
Its possible to preserve that functionality if required.

This comment has been minimized.

@ilovezfs

ilovezfs Mar 3, 2018

Contributor

The browser functionality is due to hub, but hub is doing it to attempt to be user friendly. You just opened a PR so presumably you want to inspect the final PR in your browser. The existing bump-formula-pr behavior is to print the PR url if --no-browse was passed, otherwise open the PR in the user's browser. I do think we should at least be printing the PR's url.

@MikeMcQuaid any strong feelings about the Terminal.app-to-Safari.app 🚀 ?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 3, 2018

Member

Can use open to open URLs with default handler. Use the PATH_OPEN constant.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 3, 2018

Member

Using exec_browser method defined in utils to open the browser.

@@ -391,7 +353,9 @@ def bump_formula_pr
#{user_message}
EOS
end
safe_system "hub", "pull-request", *hub_args, "-m", pr_message
pr_title = "#{formula.name} #{new_formula_version}#{devel_message}"
GitHub.create_pull_request(formula.tap.full_name, pr_title,

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 3, 2018

Member

Even this can fail. How should the exception be handled here ?

This comment has been minimized.

@ilovezfs

ilovezfs Mar 3, 2018

Contributor

The backup file needs to be restored with

formula.path.atomic_write(backup_file) unless ARGV.dry_run?

or equivalent

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:bump-formula-gh-api branch 3 times, most recently from 27ebddf to c55f64f Mar 3, 2018

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:bump-formula-gh-api branch 3 times, most recently from fdc84b5 to 08ff5f0 Mar 3, 2018

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:bump-formula-gh-api branch from 08ff5f0 to 732c425 Mar 4, 2018

"#{username}:#{branch}", "master", pr_message)["html_url"]

if ARGV.include?("--no-browse")
puts "Pull Request: #{url}"

This comment has been minimized.

@ilovezfs

ilovezfs Mar 5, 2018

Contributor

If !$stdout.tty? I think we should just output the url.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 6, 2018

Member

Agreed.

ohai "git fetch --unshallow origin" if shallow
ohai "git checkout --no-track -b #{branch} origin/master"
ohai "git commit --no-edit --verbose --message='#{formula.name} #{new_formula_version}#{devel_message}' -- #{formula.path}"
ohai "git push --set-upstream $HUB_REMOTE #{branch}:#{branch}"
ohai "hub pull-request #{hub_args.join(" ")} -m '#{formula.name} #{new_formula_version}#{devel_message}'"
ohai "create pull-request"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 6, 2018

Member

create pull-request with GitHub API or maybe just the URL for the endpoint?

else
hub_args << "--browse"
end
git_final_checkout_args << "--quiet"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 6, 2018

Member

Could remove this variable and just apply --quiet below where it's used.

"#{username}:#{branch}", "master", pr_message)["html_url"]

if ARGV.include?("--no-browse")
puts "Pull Request: #{url}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 6, 2018

Member

Agreed.

@@ -8,6 +8,8 @@ module GitHub

CREATE_GIST_SCOPES = ["gist"].freeze
CREATE_ISSUE_SCOPES = ["public_repo"].freeze

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 6, 2018

Member

Rename to CREATE_ISSUE_FORK_OR_PR_SCOPES

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:bump-formula-gh-api branch 2 times, most recently from 314c733 to b13aa73 Mar 9, 2018

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Mar 9, 2018

@MikeMcQuaid Addressed comments and updated the PR.

else
exec_browser url
end
rescue => e

This comment has been minimized.

@alyssais

alyssais Mar 12, 2018

Contributor

What exceptions are we intending to rescue here? Can we name them? (And above)

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 12, 2018

Member

Updated this. Can you please see if the new code is right?

formula.path.atomic_write(backup_file) unless ARGV.dry_run?
odie "cannot get remote from 'hub'!"
end
remote_url = response["clone_url"]

This comment has been minimized.

@alyssais

alyssais Mar 12, 2018

Contributor

Can we use #fetch instead of #[] throughout? It means that if a key isn’t present, we’ll get an exception right away, rather than a hard to debug nil error.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 12, 2018

Member

Addressed.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:bump-formula-gh-api branch from b13aa73 to 6fd5f5c Mar 12, 2018

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

A few more comments but: looking good!

if ARGV.dry_run?
ohai "hub fork # read $HUB_REMOTE"
ohai "fork"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

fork repository with GitHub API

ohai "git fetch --unshallow origin" if shallow
ohai "git checkout --no-track -b #{branch} origin/master"
ohai "git commit --no-edit --verbose --message='#{formula.name} #{new_formula_version}#{devel_message}' -- #{formula.path}"
ohai "git push --set-upstream $HUB_REMOTE #{branch}:#{branch}"
ohai "hub pull-request #{hub_args.join(" ")} -m '#{formula.name} #{new_formula_version}#{devel_message}'"
ohai "create pull-request with GitHub API"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

pull request

hub -C "#{git_path}" fork
Or setting HOMEBREW_GITHUB_TOKEN with at least 'public_repo' scope.
EOS
onoe "Unable to fork"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

Would be good to output the actual exception message too.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 20, 2018

Member

There is raise statement just below this line. It will lead to the exception message being shown.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

Ah, I see, missed that, sorry. What do you think about instead doing odie "Unable to fork: #{e.message}! or similar?

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 20, 2018

Member

Seems fine, but these exceptions have their own formatting, I'm not sure if
odie "Unable to fork: #{e.message}!" would mess up the formatting of e.message.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

It'll avoid duplication of Error: and adding an extra line unnecessarily.


begin
url = GitHub.create_pull_request(formula.tap.full_name, pr_title,
"#{username}:#{branch}", "master", pr_message)["html_url"]

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

Indentation seems a bit weird on this line.

end
rescue GitHub::AuthenticationFailedError, GitHub::HTTPNotFoundError,
GitHub::RateLimitExceededError, GitHub::Error, JSON::ParserError
onoe "Unable to open pull request"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

Would be good to print out the exception here too. Also, given these are the same exceptions as before, may want to have a wrapper for handling these rescues or just a shared array of them all.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 31, 2018

Member

Using a shared array now, updated the code & PR.

This comment has been minimized.

@GauthamGoli

GauthamGoli Mar 31, 2018

Member

My bad. Lost track of this PR. Took long to address this.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:bump-formula-gh-api branch from 6fd5f5c to b56b8b6 Mar 20, 2018

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:bump-formula-gh-api branch from b56b8b6 to 5a2db1d Mar 31, 2018

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:bump-formula-gh-api branch from 5a2db1d to 1e0f9fb Mar 31, 2018

@MikeMcQuaid MikeMcQuaid referenced this pull request Apr 8, 2018

Closed

wget 1.19.5 #26324

All comments addressed

@MikeMcQuaid MikeMcQuaid merged commit 54a594e into Homebrew:master Apr 8, 2018

1 of 3 checks passed

codecov/patch 10.52% of diff hit (target 70.14%)
Details
codecov/project 70.03% (-0.11%) compared to 60d6b4d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 8, 2018

Thanks again @GauthamGoli!

@lock lock bot added the outdated label May 8, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.