We need something to map maintainers to folders and confirm that the maintainer has actually Code Reviewed the patch.
There are at least three special cases that we need to make sure are handled. (1) A patch that touches multiple maintainers' areas. Sometimes it's useful to have both maintainer A and maintainer B fully review the patch (or at least their respective parts). Other times, it might be sufficient for A *or* B to review it. How should we handle these? (2) Overlaps and overrides. Sometimes, the maintainers for large components get overwhelmed. In these cases, I think it's necessary for the project-level architects (such as myself) to sign off on patches in their stead. Obviously some discretion is required, but as one of those project-level architects I might object if I found that the system wouldn't *let* me help in this way. (3) Mis-assigned ownership. Some files, or even parts of files, are really owned by someone other than the maintainer for that file hierarchy. For example, there are fairly large swaths of code in CLI or glusterd that really belong to tiering or geo-rep and should be reviewed by those features' maintainers instead. Snapshot functionality is spread all over other components. Again, it seems like some sort of override capability is needed, to indicate that the reviews are sufficient even if they don't include the nominal maintainer for those files.
Bang on Jeff!
Thanks Jeff for these details. This is precisely why I filed a bug: 1) For now, we'll start with A *or* B to review the code. As we figure out how many special cases we have, we'll deal with figuring out how to handle the *and* special case. I'm making an assumption that it can temporarily be dealt with as something reviewer A can trigger "Make sure you also get this reviewed by reviewer B" and then you can have the Maintainer +1. 2) Overrides, I've already thought of. My idea was to have the ultimate power in the release manager's hands. Since it's their call on what goes into each branch. But if we want to define project-level architects (superreviewers?), I'm happy to roll with that. 3) This is something we'll need to refine as we deploy. Either we make these components owned by whoever can review tiering or geo-rep or we make a few of the geo-repo and tiering people superreviewers.
(1) Starting with *or* seems good to me. As you say, we can refine later. (2) I'm actually less concerned with who the super-reviewers are (do they get cool capes?) or who can override what, so much as that the facility exist. Someone somewhere will need to issue overrides on a semi-regular basis, whether it's architects or release maintainers or someone else, so we need to make it pretty friction-free. (3) I don't suppose Gerrit has a role that allows someone to designate certain reviews as required, on a per-patch basis. That would be an ideal solution for this particular case, but I don't know if it exists.
Created attachment 1175455 [details] paths.txt These paths don't have maintainers assigned. Some of them are okay, as top-level files or files in /extras. The others probably need someone to look into.
I've found this plugin which should help us solve the primary problem https://gerrit.googlesource.com/plugins/reviewers/ I'm wondering if this second plugin can be used alongside the first as a fall back in case we have files which don't have explicit reviewers defined. https://gerrit.googlesource.com/plugins%2Freviewers-by-blame I'll do a test of them on the testing gerrit instance and report back on how that goes.
I'm wondering how we check for reviewers for the stable branches. We did not update the MAINTAINERS file with backports. There also is no notion of the release engineer for a particular branch, or even a selected version. Those release engineers should have the power to merge backports as well, but maybe not to merge anything in the master branch...
REVIEW: http://review.gluster.org/14894 (quota/marker : add maintainer for quota and marker) posted (#1) for review on master by Manikandan Selvaganesh (mselvaga)
REVIEW: http://review.gluster.org/14922 (MAINTAINERS: Adding maintainers of test component to the list) posted (#1) for review on master by M S Vishwanath Bhat (msvbhat)
REVIEW: http://review.gluster.org/14894 (quota/marker : add maintainer for quota and marker) posted (#2) for review on master by Manikandan Selvaganesh (mselvaga)
I played around with some code here: https://github.com/nigelbabu/check-maintainer I'd love it if folks could review the tests and check if we need more scenarios. It doesn't do permission inheritance yet (I'm not sure if we need it, but something to keep in mind).
We are not going through this now. When the bug was raised, we had no commit right to relevant maintainers. Now, everyone who is a maintainer gets their access to merge, so this particular feature is not required.