I think I brought this topic up somewhere before, but I don't remember
where so let's discuss it again :)
I think our current Prow configuration, which got that way by default
rather than any conscious decision, makes use of the /lgtm command too
high-stakes. A single /lgtm suffices to merge a PR when *either* the
author or the reviewer is an approver. The reasons for this are that an
approver submitting a PR automatically marks it as approved, and an
approver adding /lgtm to a PR also marks it approved.
Because we have an awesome and very respectful community, the main
result of that is that nobody wants to /lgtm a patch until absolutely
everyone has looked at it because they don't want to step on any toes. I
think it also contributes to a reluctance to add people as reviewers,
even though our stated policy is that basically anyone who is involved
in the project on an ongoing basis should be a reviewer.
I also believe it is wrong in principle. In an open source community,
maintainers should submit themselves to the same code review standards
as everyone else. It's part of the social contract (see the introduction
to [1] - full disclosure: which I wrote). Projects that fail to apply
this rule have a tendency to run into culture problems at scale (perhaps
you can think of a certain kernel that this might apply to).
Doug has done the hard work of figuring out the magic syntax to fix the
config (thanks Doug!), so if you agree then go here and, uh, I guess
/approve:
https://github.com/metal3-io/project-infra/pull/190
If folks agree then let's a adopt a heuristic that in general every PR
should have a /approve from an approver and a /lgtm from a separate
reviewer neither of whom are the author, but also that in general it
need not have *more* than that. (Obviously continue to use your
judgement in individual cases.) I think that by making it harder to
merge a PR, we should actually be able to speed up merging by quite a bit.
A possible exception is design proposals in metal3-docs. Currently we
don't have a clear policy for those, and the result is similarly that it
takes forever to merge stuff (often they merge after the code
implementing it). Informally we've been doing it by deadline, calling
for a lazy consensus in the community meeting. Perhaps we should
formalise that policy?
cheers,
Zane.
[1]
https://docs.openstack.org/project-team-guide/review-the-openstack-way.html