Automating PR merges proposal - Dev

Sage Weil

9 Aug 9 Aug

5:15 p.m.

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/> >

Reply

Ernesto Puerta

9:10 p.m.

Thanks, Sage. I'll check that script. It's interesting to know how this has been done so far. Yuri, thanks a lot for the detailed description of steps. So let's rephrase those ones in a different way: what should block a PR from being merged? - The approval of a component lead/team members? - Then let's put a "reviewed-by=@ceph/component-lead" condition. We just started to enforce this in Dashboard by using Code-Owners. - Integration/E2E tests passing? - As part of the PR checklist, both PR author or reviewers SHOULD identify whether a PR needs to pass a QA run and add the "qa-needed" label accordingly. In that case let's make "qa-needed" label block PR merges (or rely on the exiting rule for "DNM" label). Or we may create the opposite label "qa-not-needed" and assume that by default all PRs need QA? - We may use this Github Action <https://github.com/marketplace/actions/issue-checklist-checker>to ensure the PR checklist is followed (it'll add an "Incomplete Tasks" label if some checkbox is not ticked). - Issue/Bug logging/status updates? Unfortunately this is out of the scope of Mergify, but it could be 'easily' automated via Github Hooks/Actions or Jenkins. I feel that we are missing workflows in Github issues, and maybe labels are used to fill that gap? In any case, IMHO we should avoid falling back to unspecific conditions/labels, like using a "DNM"/"Ready-to-be-merged" label, as doing that would end up being more or less the same as clicking the Merge button. Kind Regards, Ernesto Puerta He / Him / His Senior Software Engineer, Ceph Red Hat <https://www.redhat.com/> <https://www.redhat.com/> On Fri, Aug 9, 2019 at 5:19 PM Yuri Weinstein <yweinste(a)redhat.com> wrote:

...

In between all steps we do semi-manual testing of every PR before merging them, e.g. batch up PRs based on dev area, build wips, run unit tests, etc. and run test suites on them. After that, we have results reviewed by dev leads, bug logged as needed and PRs are cleared for merging. BTW, merging PRs is a tedious process and if it's automated, that'd be great! (Personally I'd prefer this to be semi-automated, e.g. run a script on ready-t-merge PRs) How do you see an entry and exit criteria to indicate this in this new process? E.g. do we need to add new labels/tags to indicate this? So for example, backported PRs for nautilus release must have `label:nautilus-batch-1` and `label:needs-qa` to be tested. (Prior to this step we also have a step to create a tracker ticket for bookkeeping and release notes generation) Then we have lots of steps to build and tests PRs - do we need a new label to indicate this say "in-testing". And after PRs cleared to be merged - do we also need a new label "done-testing" Thx YuriW On Fri, Aug 9, 2019 at 4:49 AM Ernesto Puerta <epuertat(a)redhat.com> 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? 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(a)ceph.io To unsubscribe send an email to dev-leave(a)ceph.io

Reply

Ernesto Puerta

12 Aug 12 Aug

5:01 p.m.

Sure, Matt! Please, let me know what time is that (don't see it in the community calendar) or if you need some extra info from my side beforehand. Thanks! Kind regards, Ernesto Puerta He / Him / His Senior Software Engineer, Ceph Red Hat <https://www.redhat.com/> <https://www.redhat.com/> On Fri, Aug 9, 2019 at 6:26 PM Matt Benjamin <mbenjami(a)redhat.com> wrote:

...

Maybe we could have some wider discussion in the leads meeting next week? I'm not sure I get the entire idea, and it seems to be evolving. Matt On Fri, Aug 9, 2019 at 12:41 PM Ernesto Puerta <epuertat(a)redhat.com> wrote:

Thanks, Sage. I'll check that script. It's interesting to know how this has been done so far. Yuri, thanks a lot for the detailed description of steps. So let's rephrase those ones in a different way: what should block a PR from being merged? - The approval of a component lead/team members? - Then let's put a "reviewed-by=@ceph/component-lead" condition. We just started to enforce this in Dashboard by using Code-Owners. - Integration/E2E tests passing? - As part of the PR checklist, both PR author or reviewers SHOULD identify whether a PR needs to pass a QA run and add the "qa-needed" label accordingly. In that case let's make "qa-needed" label block PR merges (or rely on the exiting rule for "DNM" label). Or we may create the opposite label "qa-not-needed" and assume that by default all PRs need QA? - We may use this Github Action <https://github.com/marketplace/actions/issue-checklist-checker>to ensure the PR checklist is followed (it'll add an "Incomplete Tasks" label if some checkbox is not ticked). - Issue/Bug logging/status updates? Unfortunately this is out of the scope of Mergify, but it could be 'easily' automated via Github Hooks/Actions or Jenkins. I feel that we are missing workflows in Github issues, and maybe labels are used to fill that gap? In any case, IMHO we should avoid falling back to unspecific conditions/labels, like using a "DNM"/"Ready-to-be-merged" label, as doing that would end up being more or less the same as clicking the Merge button. Kind Regards, Ernesto Puerta He / Him / His Senior Software Engineer, Ceph Red Hat <https://www.redhat.com/> <https://www.redhat.com/> On Fri, Aug 9, 2019 at 5:19 PM Yuri Weinstein <yweinste(a)redhat.com> wrote:

In between all steps we do semi-manual testing of every PR before merging them, e.g. batch up PRs based on dev area, build wips, run unit tests, etc. and run test suites on them. After that, we have results reviewed by dev leads, bug logged as needed and PRs are cleared for merging. BTW, merging PRs is a tedious process and if it's automated, that'd be great! (Personally I'd prefer this to be semi-automated, e.g. run a script on ready-t-merge PRs) How do you see an entry and exit criteria to indicate this in this new process? E.g. do we need to add new labels/tags to indicate this? So for example, backported PRs for nautilus release must have `label:nautilus-batch-1` and `label:needs-qa` to be tested. (Prior to this step we also have a step to create a tracker ticket for bookkeeping and release notes generation) Then we have lots of steps to build and tests PRs - do we need a new label to indicate this say "in-testing". And after PRs cleared to be merged - do we also need a new label "done-testing" Thx YuriW On Fri, Aug 9, 2019 at 4:49 AM Ernesto Puerta <epuertat(a)redhat.com> 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? 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(a)ceph.io To unsubscribe send an email to dev-leave(a)ceph.io

_______________________________________________

Dev mailing list -- dev(a)ceph.io To unsubscribe send an email to dev-leave(a)ceph.io

-- Matt Benjamin Red Hat, Inc. 315 West Huron Street, Suite 140A Ann Arbor, Michigan 48103 http://www.redhat.com/en/technologies/storage tel. 734-821-5101 fax. 734-769-8938 cel. 734-216-5309

Reply

Ernesto Puerta

11:43 p.m.

Hi Greg, Thanks a lot for helping identify potential shortcomings to this. *"It's not easy to identify when satisfactory testing has been completed for a given PR (since teuthology is not integrated, and a run can have failures that do or don't matter for a given PR)"* *"**Mergify [...] seems like it works best when [.**..] the necessary reviews and test suites are very well-integrated into the GitHub API, are the sole qualifier for merging, and can be counted on to be accurate. Unfortunately**, neither of those apply to the Ceph project as a whole."* That sounds to me like there are a couple of issues over there. So: - Can we trigger package builds and schedule teuthology runs from GitHub (e.g: when qa-needed label is added)? - Can we identify QA suites to run based on labels, paths, components, comments ("Jenkins run teuthology-suite..."), ...? Besides, if we add CODEOWNERS to Ceph components, we can reuse these mappings to pick QA suites. - Can we either stabilize flapping tests or, in the meantime, flag/ignore/remove them? - Can we publish teuthology results back to GitHub and, if all positive, remove qa-needed label? To me this sounds like we already have there 90% of what we need to automate the whole dev workflow, but that 10% missing is blocking us from moving forward on CI. Even if we can only implement one or two of the above steps I think we'll have quite improved the issues you stated before. And the automated process may always fall back to a manual one if some condition is not met. Kind Regards, Ernesto El mar., 13 ago. 2019 0:22, Gregory Farnum <gfarnum(a)redhat.com> escribió:

...

On Fri, Aug 9, 2019 at 9:41 AM Ernesto Puerta <epuertat(a)redhat.com> wrote:

Thanks, Sage. I'll check that script. It's interesting to know how this has been done so far. Yuri, thanks a lot for the detailed description of steps. So let's rephrase those ones in a different way: what should block a PR from being merged? - The approval of a component lead/team members? - Then let's put a "reviewed-by=@ceph/component-lead" condition. We just started to enforce this in Dashboard by using Code-Owners. - Integration/E2E tests passing? - As part of the PR checklist, both PR author or reviewers SHOULD identify whether a PR needs to pass a QA run and add the "qa-needed" label accordingly. In that case let's make "qa-needed" label block PR merges (or rely on the exiting rule for "DNM" label). Or we may create the opposite label "qa-not-needed" and assume that by default all PRs need QA? - We may use this Github Action <https://github.com/marketplace/actions/issue-checklist-checker>to ensure the PR checklist is followed (it'll add an "Incomplete Tasks" label if some checkbox is not ticked). - Issue/Bug logging/status updates? Unfortunately this is out of the scope of Mergify, but it could be 'easily' automated via Github Hooks/Actions or Jenkins. I feel that we are missing workflows in Github issues, and maybe labels are used to fill that gap? In any case, IMHO we should avoid falling back to unspecific conditions/labels, like using a "DNM"/"Ready-to-be-merged" label, as doing that would end up being more or less the same as clicking the Merge button.

I'm not sure how the dashboard development process works, but Yuri's questions and the issues they provoke worry me about any kind of automated merge system in the broader Ceph codebase. It's not easy to identify when satisfactory testing has been completed for a given PR (since teuthology is not integrated, and a run can have failures that do or don't matter for a given PR), and reviewed-by statements or Github approvals can be provided either before or after that teuthology testing is done, and sometimes contingent on it passing. (ie, "LGTM assuming tests pass") I really don't see a way we can programmatically interrogate the necessary "win" conditions right now without requiring some kind of explicit label that is semantically equivalent to pushing the merge button. Plus, our leads already have workflows built up to automate some portion of those steps when they're comfortable. From looking at how Mergify sells itself and is used in ceph-ansible, it seems like it works best when either 1) the merge conditions are very loose (ie, 1-2 approvals), or 2) the necessary reviews and test suites are very well-integrated into the GitHub API, are the sole qualifier for merging, and can be counted on to be accurate. Unfortunately, neither of those apply to the Ceph project as a whole. :( -Greg

Reply