> I became curious with these statements regarding self-review > (committer==reviewer) and so I ran a couple > of queries against the gerrit database to see how often this occurs: > > 1) For the puppet repo, 84.1% of the commits is self-reviewed. > 2) For the mediawiki core repo, 27.9% of the commits is self-reviewed.
I'm actually very surprised to see such a high number for MediaWiki core. Why is this the case? When you say committer==reviewer, so do you mean the reviewer did a +2 and merged? Does this take into account committers that also just added comments? > 3) For the mediawiki extensions repos, 67.8% of the commits is self-reviewed. > This doesn't surprise me at all. I have a *really* hard time getting my extensions reviewed and I work for the foundation. I have to beg for reviews, and even then it can take up to a couple weeks sometimes. It's problematic. That said, I don't necessarily think my extensions need pre-merge review. I'd really like Gerrit to support post-merge review in some fashion. > I think we need to take a step back from a tool-focused discussion and first > hash out what our commit workflows are / should be. In particular: > > 1) Should there be one commit workflow that applies to all teams? Looking at > current practise, the answer seems to be no but I am curious to hear what > other people think. If the answer is that it's okay for different teams to > have different commit workflows, then we should also look for tools that > support this. > Things that are going into production should be reviewed, preferably pre-merge. It should definitely be reviewed before deployment. Insecure code on a wikimedia.org domain has the ability to affect everything else too. > 2) If self-review is so prevalent, does that mean that the pre-commit review > workflow has failed? > Self-review is so prevalent in puppet because it's very difficult to do operations any other way. We occasionally do 50-100 deployments a day. Labs isn't a fully functional replica of everything in production yet, either, so we have no way to stage deployments, which means we can't wait on reviews. That said, pre-merge review is working exactly as intended. The fact that nearly 20% of the merges are reviewed shows exactly how well it's working. If you take a look at that more closely, I'm betting you'll see that those commits are almost completely outside of the operations staff. That's an amazingly huge win for a team that had 0 merged commits outside of the operations staff. This process simply wouldn't work without pre-merge review. - Ryan _______________________________________________ Wikitech-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikitech-l
