Skip to content

ci: enable automatic merging of pull requests #3585

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

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

jamacku
Copy link
Contributor

@jamacku jamacku commented Jan 15, 2025

PR that meets the following criteria will be automatically merged:

  • Passing CI
  • Approved by at least one reviewer
  • No Draft or WIP title
  • No conflicts

The merging workflow runs on schedule every 45 minutes. It can be executed manually on demand from the Actions tab. It internally uses several GitHub Actions to perform the merge.


I have tested the workflow on jamacku#1

A working example can be found at redhat-plumbers/systemd-rhel10#45 (comment). It contains a few more pieces, but merging logic is also present.


There are a few limitations/requirements:

/cc @xsuchy

PR that meets the following criteria will be automatically merged:

- Passing CI
- Approved by at least one reviewer
- No Draft or WIP title
- No conflicts

Merging workflow is running on schedule every 45 minutes.
It can be executed manually on demand from the Actions tab.
It internally uses several GitHub Actions to perform the merge.

- https://github.com/redhat-plumbers-in-action/gather-pull-request-metadata to gather PR metadata
- https://github.com/redhat-plumbers-in-action/pull-request-validator to check reviews and CI status
- https://github.com/redhat-plumbers-in-action/auto-merge to perform the merge
- https://github.com/redhat-plumbers-in-action/issue-commentator to comment on PRs about the merge status
@mfocko
Copy link
Contributor

mfocko commented Jan 15, 2025

Just out of interest, what's the reason for using GH Action instead of the GitHub's built-in functionality?

At the same time, from the reviewer's PoV, if I review the PR and click on the big green button denoting merge when ready, it has basically the same effect, cause in the branch protection rules I can require specific CI checks to pass, and also there cannot be any merge conflicts.

@jamacku
Copy link
Contributor Author

jamacku commented Jan 16, 2025

Just out of interest, what's the reason for using GH Action instead of the GitHub's built-in functionality?

This is a valid question. We (rhel systemd) had this automation before GitHub introduced all the Auto-merge stuff. We have other automation pieces related to Jira and Upstream that work together with auto-merge functionality. It allows us to have more customization and flexibility regarding the behavior. For example, we can waive the CI, all CI checks block us, and we pick non-blocking checks; when PR is merged, we transition Jira into a new state, etc.

For the copr project, it might be sufficient to use rules and auto-merge. But I know the copr project has a policy about merging approved PRs only after 24 hours from submission. This is currently not possible by using only rules. It is currently not possible by our action either, but it can be easily extended to support this requirement.

@praiskup
Copy link
Member

I'm pretty happy with the approach we currently have, there are no delays in fedora-copr/copr project caused by manual merges; getting a proper review is much more expensive than hitting "rebase and merge" button (if so, I'm completely fine to resolve concerns).

Also, somewhat nitpicking - I like if "Committer: " value != bot account.

Then there are often situations when somebody reviews, and since we are not in hurry and we don't merge immediately - another team member has a chance to review even after more than 24 hours (once merged, nobody reviews).

Note though that this is an opinion of just one team-member, others may feel differently, so let's give them a chance to speak.
Also note that automatic merges after ~24h or maybe even ~7days would be very welcome in other, rather low-traffic projects (this one though in particular is under a pedantic set of humans' radars).

@praiskup
Copy link
Member

praiskup commented Feb 5, 2025

We discussed this yesterday on the team meeting; summary is that the team seems to be for this change. I see no benefits, just risks, and path to a code-quality decrease (maybe maintenance problems). I'm confident with my 👎 vote, sorry.

One of the ideas was to have a rule of "at least two reviewers", would that be possible?

@nikromen promised to make a proper review.

Copy link
Member

@nikromen nikromen left a comment

Choose a reason for hiding this comment

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

I say let's try it and see, mayble we'll like it, maybe not :D but yup let's discuss this on planing

@nikromen nikromen merged commit 18f102d into fedora-copr:main Feb 10, 2025
11 checks passed
@jamacku jamacku deleted the merger branch February 10, 2025 12:07
@praiskup
Copy link
Member

Two post-merge remarks:

  • the tool seems to label all the pull requests with its own labels, not sure it is anyhow useful, but we'll see long term
  • what is very bad is that it "touches" each PR periodically, bumping the timestamp; when I sort the PRs per Recently updated, it seems all the PRs are updated all at the same time (so I don't see which PR got recent updates by human)
    Screenshot_20250218_113752

@jamacku
Copy link
Contributor Author

jamacku commented Feb 18, 2025

Thank you Pavel for the feedback.

  • the tool seems to label all the pull requests with its own labels, not sure it is anyhow useful, but we'll see long term

The labels could be disabled if you don't find them useful.

  • what is very bad is that it "touches" each PR periodically, bumping the timestamp; when I sort the PRs per Recently updated, it seems all the PRs are updated all at the same time (so I don't see which PR got recent updates by human)

This is new to me. I have never used sorting by recently updated, but I will try to improve this so it won't touch PRs that frequently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants