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