On Fri, Aug 9, 2019 at 12:41 PM Ernesto Puerta <epuertat@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 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,On Fri, Aug 9, 2019 at 5:19 PM Yuri Weinstein <yweinste@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"
ThxYuriW
On Fri, Aug 9, 2019 at 4:49 AM Ernesto Puerta <epuertat@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) 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 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 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, but the GitHub Reviews API does not easily provide with every reviewer's e-mail (nor the Users API does), so it needs to be extracted from the Events API (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 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,
Dev mailing list -- dev@ceph.io
To unsubscribe send an email to dev-leave@ceph.io
Dev mailing list -- dev@ceph.io
To unsubscribe send an email to dev-leave@ceph.io
