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.

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

workflows: publish approved PRs upon CI completion #128544

Closed
wants to merge 3 commits into from

Conversation

carlocab
Copy link
Member

This is based on one of @MikeMcQuaid's suggestions on Slack.

We add an automerge.yml workflow that is triggered by the
workflow_run event. We need this in order to escalate access of
GITHUB_TOKEN from the pull_request and pull_request_review events,
which can only have read access to the GitHub API.

The automerge workflow is triggered by two events:

  • completion of a CI workflow on a pull request
  • approval of a pull request

Because both of these events are required in order to merge, we allow
the workflow to wait for a while to check if the other event occurs.

The basic idea of this workflow was borrowed from the documentation on
workflow_run events.

@github-actions github-actions bot added automerge-skip `brew pr-automerge` will skip this pull request workflows PR modifies GitHub Actions workflow files labels Apr 17, 2023
@carlocab carlocab force-pushed the merge-on-approval-or-ci branch 5 times, most recently from 7b1ba90 to 8552b35 Compare April 17, 2023 08:27
Copy link
Member Author

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

I've tested modified versions of these workflows in a separate repository, and it seems like it should work, barring issues with concurrency and GITHUB_TOKEN permissions.

There's also the question of all the boilerplate.

@carlocab carlocab marked this pull request as ready for review April 17, 2023 08:49
@carlocab carlocab requested review from a team and MikeMcQuaid as code owners April 17, 2023 08:49
MikeMcQuaid
MikeMcQuaid previously approved these changes Apr 17, 2023
Copy link
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.

Think I'm fine with this but not sure I totally understand

  • what it's doing
  • why it's needed
  • how it differs from the existing auto-merge logic in PRs
  • how it differs from/complements/replaces the existing "brewtestbot pushes to PR" flow(s)

Happy to answer these questions post-merge but would love to understand better. Thanks again @carlocab!

@carlocab
Copy link
Member Author

This might help refresh your memory: https://machomebrew.slack.com/archives/C06G173B7/p1680113506757109

The idea is that we want a workflow triggered by pull_request_review to run brew pr-publish (or otherwise somehow merge the PR when CI is done). The problem with doing that is that pull_request_review workflows have no access to secrets, and have a read-only GITHUB_TOKEN.

The solution to that is described in the docs for workflow_run events:

The workflow started by the workflow_run event is able to access secrets and write tokens, even if the previous workflow was not. This is useful in cases where the previous workflow is intentionally not privileged, but you need to take a privileged action in a later workflow.

So at minimum, what we need is for a pull_request_review workflow to trigger a workflow_run workflow that can do brew pr-publish. Here, the workflow_run workflow is automerge.yml. Since it's triggered by successful completion of a pull_request_review workflow (reviews.yml), we may as well also trigger it on successful completion of tests.yml on a pull_request event. The rest is mopping up details.

The difference with the existing autopublish.yml workflow is that autopublish.yml runs on a timer, while in theory this should allow for merging PRs sooner than that.

This is mostly independent of the "brewtestbot pushes to PR" flow, I think. We could do this even if we didn't push to PRs, and it doesn't really improve upon or detract from that flow in any way (AFAICT).

@MikeMcQuaid
Copy link
Member

The difference with the existing autopublish.yml workflow is that autopublish.yml runs on a timer, while in theory this should allow for merging PRs sooner than that.
This is mostly independent of the "brewtestbot pushes to PR" flow, I think. We could do this even if we didn't push to PRs, and it doesn't really improve upon or detract from that flow in any way (AFAICT).

Perfect, all good to me to merge then, thanks @carlocab!

@ZhongRuoyu
Copy link
Member

ZhongRuoyu commented Apr 17, 2023

I'm thinking if this will result in duplicate bottle publish workflow runs if an approval happens to be on the hour (i.e., coincides with the hourly workflow).

Maybe we could add a concurrency to the publish-commit-bottles workflow?

@MikeMcQuaid
Copy link
Member

@ZhongRuoyu Good point.

Maybe we could add a concurrency to the publish-commit-bottles workflow?

They should probably share the same concurrency key.

@carlocab
Copy link
Member Author

Yes, this is one of the concurrency issues I said I was still not sure about. Will think about how to handle this better.

This is based on one of @MikeMcQuaid's suggestions on Slack.

We add an `automerge.yml` workflow that is triggered by the
`workflow_run` event. We need this in order to escalate access of
`GITHUB_TOKEN` from the `pull_request` and `pull_request_review` events,
which can only have read access to the GitHub API.

The `automerge` workflow is triggered by two events:
- completion of a `CI` workflow on a pull request
- approval of a pull request

Because both of these events are required in order to merge, we allow
the workflow to wait for a while to check if the other event occurs.

The basic idea of this workflow was borrowed from the documentation on
`workflow_run` events [1].

[1] https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run
@carlocab carlocab force-pushed the merge-on-approval-or-ci branch 2 times, most recently from 7b49267 to f1949c5 Compare April 18, 2023 08:23
This will make sure we only run one of these at a time for any given PR.

While we're here:
- add stricter checks to make sure we don't attempt to `pr-pull` PRs
  that we don't want or need to
- avoid looping over arrays twice when we don't need to
- improve readability by relying less on workflow expression interpolation
- use `GITHUB_TOKEN` more often
These are no longer needed.
@carlocab
Copy link
Member Author

Pushed my branch to this repo for better testing. #128697

@carlocab carlocab closed this Apr 18, 2023
@carlocab carlocab deleted the merge-on-approval-or-ci branch April 18, 2023 17:22
@github-actions github-actions bot added the outdated PR was locked due to age label May 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request outdated PR was locked due to age workflows PR modifies GitHub Actions workflow files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants