MAINTAINERS: disallow self-LGTMs

[cyphar]

Apart from being a sign of respect to other maintainers, it also ensures
that all pull requests get equal amounts of review -- no matter who
wrote them. Maintainers should know better than to make broken
patchsets, but it's also a sign of respect to the community that all
pull requests have equal treatment.

Signed-off-by: Aleksa Sarai [email protected]

[wking]

On Thu, May 26, 2016 at 08:28:21PM -0700, Aleksa Sarai wrote:

Apart from being a sign of respect to other maintainers, it also ensures
that all pull requests get equal amounts of review -- no matter who
wrote them. Maintainers should know better than to make broken
patchsets, but it's also a sign of respect to the community that all
pull requests have equal treatment.

I'm not convinced ;). For example in runtime-spec:

$ git log --first-parent --format='%h (%aN %ad) %s' --date=short origin/master | grep 'Vincent.*vbatts'
cbea66a (Vincent Batts 2016-05-03) Merge pull request #422 from vbatts/travis_make_target
e6427cb (Vincent Batts 2016-04-27) Merge pull request #349 from vbatts/target-install-tools
3f1c150 (Vincent Batts 2016-04-12) Merge pull request #379 from vbatts/bump_version
7aa7dcd (Vincent Batts 2016-03-23) Merge pull request #347 from vbatts/user-name
eea2a6c (Vincent Batts 2016-03-09) Merge pull request #310 from vbatts/multi-platform
c8de60b (Vincent Batts 2016-01-20) Merge pull request #295 from vbatts/vbatts-test
d7df1b4 (Vincent Batts 2015-12-21) Merge pull request #263 from vbatts/printable
e71b6dd (Vincent Batts 2015-10-21) Merge pull request #227 from opencontainers/vbatts-no-hyphens
2d9842b (Vincent Batts 2015-09-09) Merge pull request #167 from vbatts/validate-dco

But most of those are “@vbatts is the maintainer that cares the most
about CI tooling and printable docs”.

If you're concerned about abusive maintainers, I'd just suggest the
other maintainers warn and then boot anyone they feel isn't playing
nice (without defining “playing nice” for them).

And if you want a rule to minimize abuse, I think a better approach is
to put a minimum cook time on PRs. Then maintainers (and other
community members) know how often they need to poll the pending queue
if they want a chance to comment on anything before it lands. That
also incentivizes maintainers to at least chime in with “please hold
this for another day, I haven't had time to review it yet” instead of
having a PR languish with a single LGTM for months ;).

[wking]

There are also a few earlier comments on this issue starting at 1.
I still haven't been able to dig up a reference to the earlier
discussion…

[cyphar]

@wking

I don't really agree with the idea that we should value "increased development speed" over code quality and review. More importantly, it should be clear to the community that while maintainers have merge rights they go through precisely the same amount of review as everyone else. Maintainers make mistakes too, and we benefit from code review like everyone else.

In addition, having more time for maintainers to review code makes sure that all maintainers are on the same page in regard to what is being merged -- for example I didn't see the PullApprove PR until after it was merged (I was asleep) and I have some strong opinions on it that I didn't get to voice before it was merged.

To be clear: I never said that I have a problem with maintainers merging their own PRs (which is what your example appears to be pointing out). We just need to go through the two LGTM process like everyone else.

I do like the idea of a cook time for PRs, but I honestly think that we should have both. Mainly because I'm in a different timezone from everyone else, so I usually miss some PRs.

[wking]

On Fri, May 27, 2016 at 04:23:58AM -0700, Aleksa Sarai wrote:

More importantly, it should be clear to the community that while
maintainers have merge rights they go through precisely the same
amount of review as everyone else.

And they do (still have to have two maintainer LGTMs). The submitter
would just be wearing two hats (submitter and maintainer) for a single
PR.

Maintainers make mistakes too, and we benefit from code review like
everyone else.

Absolutely, but trying to catch all the mistakes on the way in is
impossible, and as I said before, you can always boot abusive
maintainers (and file follow-up PRs).

In addition, having more time for maintainers to review code makes
sure that all maintainers are on the same page in regard to what is
being merged -- for example I didn't see the PullApprove PR until
after it was merged (I was asleep) and I have some strong opinions
on it that I didn't get to voice before it was merged.

Right, but I think the problem there is cook-time.
opencontainers/runc#847 was as @caniszczyk commit LGTMed by two runC
maintainers (so it checks all the boxes you're currently asking for).
The reason you didn't have time to chime in was that it was only open
for 2.5 hours (from 3:33pm to 6pm PDT). And I think the solution to
that is a minimum cook time (2 weekdays? A week?) 1 or having
maintainers boot/warn merge-happy fellow maintainers 1.

[cyphar]

Admittedly, I did change my point mid-comment. The general point is that I don't like the whole "two hats" approach to maintainence -- when you write code and submit it you are a contributor like anyone else (who happens to have more experience than most, but you don't need to be a maintainer to have experience). Follow-up PRs become very messy after a while (especially when you end up with disagreements about the actual architecture of a change) -- it's much nicer to deal with all of the upfront problems upfront.

I didn't say LGTM-ing your own PR is abusive, it just seems disrespectful to me (it might just be a cultural thing though, you don't get special treatment when writing a scientific paper just because you happen to be the head of school) -- I know that you are confident in your code (everyone is), but peer review helps make sure that everyone gets equal scrutiny.

As a separate point, I'd be okay with a 1-2 weekday cook time for every PR (purely for timezone reasons), but there's a valid question about whether or not the cook time should be reset on each push (as the code could've drastically changed).

[wking]

On Fri, May 27, 2016 at 09:44:02AM -0700, Aleksa Sarai wrote:

Follow-up PRs become very messy after a while (especially when you
end up with disagreements about the actual architecture of a
change)…

If you don't trust a maintainer to wait for sufficient consensus on
architectural PRs, you have bigger problems than self-LGTMs ;).

As a separate point, I'd be okay with a 1-2 weekday cook time for
every PR (purely for timezone reasons), but there's a valid question
about whether or not the cook time should be reset on each push (as
the code could've drastically changed).

Yeah, I think it should be reset (just like the LGTM count is
currently being reset).

[crosbymichael]

LGTM

[vbatts]

LGTM

[vbatts]

and this is also, not to say that the creator of the PR can't be the one to merge it, if the two LGTM is satisfied, which is what I gather from the some of these comments.

[crosbymichael]

Ya, that is what I assumed, as long as it has the proper lgtm's then it does not matter who presses the merge button.

[cyphar]

@wking I'll open a PR (or you can if you like) about the cook time issue.

[wking]

On Tue, May 31, 2016 at 06:13:57PM -0700, Aleksa Sarai wrote:

@wking I'll open a PR (or you can if you like) about the cook time
issue.

Go for it ;). My personal preference is still to leave this level of
detail to private communication between the maintainers themselves
(see ‘without defining “playing nice”’ in 1).

[cyphar]

Yeah, I agree that we should allow maintainers to decide how long a cook time is reasonable for large PRs. But for small ones we should at least have a rule of thumb, which should be followed at the maintainers' discretion.