Skip to content

bump-formula-pr: enable same-repo (no-fork) Pull Requests#6723

Merged
MikeMcQuaid merged 7 commits intoHomebrew:masterfrom
maxim-belkin:gh-actions/no-fork-prs
Nov 22, 2019
Merged

bump-formula-pr: enable same-repo (no-fork) Pull Requests#6723
MikeMcQuaid merged 7 commits intoHomebrew:masterfrom
maxim-belkin:gh-actions/no-fork-prs

Conversation

@maxim-belkin
Copy link
Copy Markdown
Contributor

@maxim-belkin maxim-belkin commented Nov 11, 2019

  • 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?

To build bottles with GitHub actions, we need to create PRs from branches that live in the same tap. Here I add --no-fork option that instructs bump-formula-pr to create branch in origin rather than in a forked repo.

Tested at maxim-belkin/homebrew-xorg#581

Copy link
Copy Markdown
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Change makes sense, just thinking about ways to clean code while we do it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe this could all be extracted to another function to make use of e.g. early returns?

@issyl0
Copy link
Copy Markdown
Member

issyl0 commented Nov 15, 2019

This needs a rebase now #6718 has been merged.

@maxim-belkin maxim-belkin force-pushed the gh-actions/no-fork-prs branch from 2da02a5 to 37a1203 Compare November 19, 2019 19:44
Comment thread Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated
Comment thread Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated
Comment thread Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated
Comment thread Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated
Comment thread Library/Homebrew/utils/github.rb Outdated
Comment thread Library/Homebrew/utils/github.rb Outdated
@MikeMcQuaid
Copy link
Copy Markdown
Member

Conflicting files

Library/Homebrew/dev-cmd/bump-formula-pr.rb

Just to note this. Should be good to go after that (pending one final review).

@maxim-belkin maxim-belkin force-pushed the gh-actions/no-fork-prs branch from c8482bd to c7f065b Compare November 21, 2019 17:13
@maxim-belkin
Copy link
Copy Markdown
Contributor Author

Rebased the branch and it seems to be working as expected.
Tested here: maxim-belkin/homebrew-xorg#588

@MikeMcQuaid MikeMcQuaid merged commit 28635a7 into Homebrew:master Nov 22, 2019
@MikeMcQuaid
Copy link
Copy Markdown
Member

MikeMcQuaid commented Nov 22, 2019

Thanks again @maxim-belkin!

@dawidd6
Copy link
Copy Markdown
Contributor

dawidd6 commented Nov 22, 2019

I think you meant @maxim-belkin :)

@MikeMcQuaid
Copy link
Copy Markdown
Member

Oops 😉! Edited!

@dawidd6
Copy link
Copy Markdown
Contributor

dawidd6 commented Nov 23, 2019

This PR broke current standard workflow with forks. I'll shamelessly paste here the output that @chenrui333 got:

$ brew bump-formula-pr babel --url=https://registry.npmjs.org/@babel/core/-/core-7.7.4.tgz --sha256=3bb9a1d79c68a0c1c41885b86471d306b99efefabb2def80ea3a511d3476f048 --verbose
==> replace /https:\/\/registry\.npmjs\.org\/@babel\/core\/\-\/core\-7\.7\.2\.tgz/ with "https://registry.npmjs.org/@babel/core/-/core-7.7.4.tgz"
==> replace "e2e9745819df1a040265933785536c2b3317661bc635c63e98497e4882ae6a39" with "3bb9a1d79c68a0c1c41885b86471d306b99efefabb2def80ea3a511d3476f048"
/usr/local/bin/brew audit /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/babel.rb
Error: Not Found

It's probably because of this new function: GitHub.check_fork_exists(tap_full_name).

@dawidd6 dawidd6 mentioned this pull request Nov 23, 2019
6 tasks
@maxim-belkin
Copy link
Copy Markdown
Contributor Author

maxim-belkin commented Nov 23, 2019 via email

@maxim-belkin
Copy link
Copy Markdown
Contributor Author

The only change that should be noticeable after this PR is that now bump-formula-pr will not attempt to push to origin without --no-fork and vice a versa: with --no-fork it will not attempt to create a fork.

@dawidd6 and @chenrui333, could you please post here a repro next time you encounter an error? I see that babel has been updated...

@dawidd6
Copy link
Copy Markdown
Contributor

dawidd6 commented Nov 23, 2019

The repro is actually pretty simple, try to brew bump-formula-pr anything without --no-fork option passed and the error will appear.

Maybe it has something to do with:

def api_credentials
@api_credentials ||= begin
if ENV["HOMEBREW_GITHUB_API_TOKEN"]
ENV["HOMEBREW_GITHUB_API_TOKEN"]
elsif ENV["HOMEBREW_GITHUB_API_USERNAME"] && ENV["HOMEBREW_GITHUB_API_PASSWORD"]
[ENV["HOMEBREW_GITHUB_API_PASSWORD"], ENV["HOMEBREW_GITHUB_API_USERNAME"]]
else
github_credentials = api_credentials_from_keychain
github_username = github_credentials[/username=(.+)/, 1]
github_password = github_credentials[/password=(.+)/, 1]
if github_username && github_password
[github_password, github_username]
else
[]
end
end
end
end

This function is used in check_fork_exists function that you introduced in this PR, but I only have HOMEBREW_GITHUB_API_TOKEN set and it seems like that function depends on HOMEBREW_GITHUB_API_USERNAME being set, yet I don't have the need to set it.

Also the new forked_repo_info function is a bit broken (no begin, too few arguments, ...).

@maxim-belkin
Copy link
Copy Markdown
Contributor Author

maxim-belkin commented Nov 23, 2019 via email

@maxim-belkin maxim-belkin deleted the gh-actions/no-fork-prs branch November 27, 2019 21:00
@lock lock bot added the outdated PR was locked due to age label Jan 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants