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