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/> > _______________________________________________ Dev mailing list -- dev@xxxxxxx To unsubscribe send an email to dev-leave@xxxxxxx
