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