Skip to content

Add new pr-publish command#7202

Merged
jonchang merged 3 commits intoHomebrew:masterfrom
jonchang:pr-publish
Mar 24, 2020
Merged

Add new pr-publish command#7202
jonchang merged 3 commits intoHomebrew:masterfrom
jonchang:pr-publish

Conversation

@jonchang
Copy link
Copy Markdown
Contributor

@jonchang jonchang commented Mar 22, 2020

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

This is the maintainer-facing command we need before switching over to GitHub Actions with self-hosted runners.

This new command issues a single GitHub API call to fire the repository_dispatch event, which starts this workflow in Homebrew/core: https://github.com/Homebrew/homebrew-core/blob/master/.github/workflows/publish-commit-bottles.yml

The workflow:

  1. Gets the details of the user who fired the dispatch event (name and email address)
  2. Authenticates against GitHub using the BrewTestBot personal access token and checks out Homebrew/core
  3. Downloads artifacts (bottles) associated with the pull request specified in brew pr-publish
  4. Uploads those bottles to Bintray using brew test-bot --ci-upload --publish (c.f. test-bot: use separate API call on publish homebrew-test-bot#344)
  5. Modifies all new commits to set the committer to the user who fired the dispatch event and signs off the last commit (similar to brew pull --bottle)
  6. Pushes the new commits to master

Maintainers who want to opt-in to the new workflow should simply run brew pr-publish <pr> [<pr> ...]. This will use the GitHub Actions bottles rather than the Jenkins bottles. The Jenkins workflow can still be used with brew pull --bottle <pr>.

Example of a successful run:

@jonchang jonchang requested a review from MikeMcQuaid March 22, 2020 02:25
@dawidd6
Copy link
Copy Markdown
Contributor

dawidd6 commented Mar 22, 2020

But Jon, you didn't merge the PR commit to master. Mednafen is still on previous version, while the bottle is on the newest.

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.

A few suggested style tweaks/cleanups but looks good, great work again!

Comment on lines +17 to +24
switch :verbose
end
end

def pr_publish
pr_publish_args.parse

odie "You need to specify a pull request number!" if Homebrew.args.named.empty?
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.

Suggested change
switch :verbose
end
end
def pr_publish
pr_publish_args.parse
odie "You need to specify a pull request number!" if Homebrew.args.named.empty?
switch :verbose
named 1
end
end
def pr_publish
pr_publish_args.parse

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.

@jonchang This DSL would be preferable to use here

comments.any? { |comment| comment["body"].eql?(body) }
end

def dispatch(user, repo, event, **payload)
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.

Suggested change
def dispatch(user, repo, event, **payload)
def repo_dispatch_event(user, repo, event, **payload)

or

Suggested change
def dispatch(user, repo, event, **payload)
def dispatch_event(user, repo, event, **payload)

tap = Tap.fetch(user, repo) if repo.match?(HOMEBREW_OFFICIAL_REPO_PREFIXES_REGEX)
odie "Not a GitHub pull request: #{arg}" unless issue
ohai "Dispatching #{tap} pull request ##{issue}"
GitHub.dispatch(user, repo, "Publish ##{issue}", pull_request: issue)
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.

Could this (or a line after) output a URL that you can use to check progress? If not possible: no worries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This unfortunately returns 204 No Content. We'd have to find it using another API call.

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.

😭. Even if it outputs the full PR URL somewhere that might be nice. Where does the "action" show up; just in the actions tab on the repo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, just in the Actions tab. You can filter by actor and workflow type though which can help. In Linuxbrew/core we have @BrewTestBot ping the requestor when the upload job fails. This could be added to the workflow in Homebrew/core:

https://github.com/Homebrew/linuxbrew-core/blob/3d460c215425e7d1547865ad0c2f5b4b7bce7dd7/.github/workflows/upload-bottles.yml#L165-L173
https://github.com/Homebrew/linuxbrew-core/pull/19902#issuecomment-601881442

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.

Yeh, this would be great to have there too 👍

@jonchang
Copy link
Copy Markdown
Contributor Author

jonchang commented Mar 22, 2020

But Jon, you didn't merge the PR commit to master. Mednafen is still on previous version, while the bottle is on the newest.

Fixed!

@Moisan
Copy link
Copy Markdown
Member

Moisan commented Mar 22, 2020

Great work! I've tested it and noticed that in the bottle commit, the sign-off email is different than what we usually see when using brew pull --bottle:

Signed-off-by: Thierry Moisan <31669+Moisan@users.noreply.github.com>

Is that expected?

@jonchang
Copy link
Copy Markdown
Contributor Author

Great work! I've tested it and noticed that in the bottle commit, the sign-off email is different than what we usually see when using brew pull --bottle:


Signed-off-by: Thierry Moisan <31669+Moisan@users.noreply.github.com>

Is that expected?

Yes, it either uses your public GitHub email address or your noreply address to associate the commit with your account. See https://help.github.com/en/github/setting-up-and-managing-your-github-user-account/setting-your-commit-email-address

@Bo98
Copy link
Copy Markdown
Member

Bo98 commented Mar 23, 2020

Just to check: presumably this means that any sort of squashing, commit rewording etc will no longer be possible and will now require asking the pull request author to do so and run a new CI build?

@jonchang
Copy link
Copy Markdown
Contributor Author

Just to check: presumably this means that any sort of squashing, commit rewording etc will no longer be possible and will now require asking the pull request author to do so and run a new CI build?

With this workflow, yes. Either a maintainer or the pull request author would need to make these changes on the pull request branch. I'm not sure how we could do something similar to our current workflow with GitHub Actions.

@jonchang jonchang merged commit 5f53180 into Homebrew:master Mar 24, 2020
@jonchang jonchang deleted the pr-publish branch March 24, 2020 01:24
@Bo98
Copy link
Copy Markdown
Member

Bo98 commented Mar 24, 2020

I suppose the payload could be used, but the different scenarios probably would make it get complex very fast.

@lock lock bot added the outdated PR was locked due to age label Apr 25, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 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.

5 participants