On Fri, 9 Aug 2019, Ernesto Puerta wrote:
Hi all,
At Ceph-Dashboard's meetings we have discussed a couple of times how to
improve PR review & merge process. Sometimes Pull Requests get stalled with
no clear reason: they have positive reviews, perhaps some
comments/suggestions, but not clear rejections. And, after a certain
moment, the longer they remain open, the harder they seem to get merged (or
closed).
An idea taken from Ceph-Ansible team would be to 'code' the set of rules
that a PR needs to meet in order to be merged and use some tool (e.g.:
Mergify <https://mergify.io>) to implement that. This would change the
contribution pipeline from a push model (PRs not merged unless manual
action taken) to pull model (rules met, PRs get merged unless manual action
taken).
This PR <https://github.com/ceph/ceph/pull/29496> brings Mergify to
Dashboard, with the following set of rules:
- PR base branch is 'master'
- PR is labeled 'dashboard'
- PR title starts with "mgr/dashboard: "
- All Jenkins checks pass (except arm64)
- 2 review approvals required with no changes-requested or unaddressed
comments. All requested reviewers have issued a review (perhaps too strict).
- As Dashboard's CODEOWNERS PR
<https://github.com/ceph/ceph/pull/29451> is already merged, that
means that at least 1 one the approvals must come from a @ceph/dashboard
team member.
- 'DNM' label not present
One downside identified is that Mergify does not allow to specify a message
for the merge commit (e.g.: to add "Reviewed-by:" metadata). I'm preparing
a PR to Mergify repo <https://github.com/Mergifyio/mergify-engine>,
but the GitHub
Reviews <https://developer.github.com/v3/pulls/reviews/> API does not
easily provide with every reviewer's e-mail (nor the Users API
<https://developer.github.com/v3/users/> does), so it needs to be extracted
from the Events API <https://developer.github.com/v3/activity/events/>
(which is kind of tricky). And this use case seem to be specific to us,
right?
The ptl-tool.py script (src/scripts/ptl-tool.py) uses the .githubmap file
in the ceph.git repo to do the github username -> name+email mapping.
This all sounds great!
sage
> So, would it be ok to have PR merge commit messages with "Reviewed-By:
> FirstName LastName <@github_login>" or even with no message at all?
>
> BTW, apart from the "merge" action, Mergify provides other actions
> <https://doc.mergify.io/actions.html> that could be useful to automate
> other daily manual steps:
>
> - Backport the PR to another branch
> - Add a comment to the PR
> - Add or remove labels (e.g: if the PR title matches "^mgr/dashboard:
",
> let's add a "dashboard" label).
>
> Any feedback welcome!
>
> Kind Regards,
>
> Ernesto Puerta
>
> He / Him / His
>
> Senior Software Engineer, Ceph
>
> Red Hat <https://www.redhat.com/>
> <https://www.redhat.com/>
>