Hi Julius,
On 7/6/20 10:45 PM, Julius Werner wrote:
So I suggest the following. We could have 2 ways to get a patch approved.
- For patches that have multiple code owners:
- COR+1 (not from patch owner)
- MR+1 (not from patch owner, not from the person setting COR+1)
- For patches that a have single code owner:
- CR+1 (not from patch owner)
- MR+1 (not from patch owner, not from the person setting CR+1)
Well, wouldn't this mean that patches which have a single code owner never need owner approval in any case? I think the intention was to only make a special case if the code owner themselves write the code, right?
Oh yes sorry, that's exactly what I meant (only for single code owners who are also the patch author). I can see where the confusion comes from!
In that case, you would have to write this slightly differently:
- Normal case:
- COR+1 (not from author)
- MR+1 (not from author, not from the person setting COR+1)
- Special case:
- author must have COR rights (or maybe: author must be the only
person with COR rights?)
- CR+1 (not from author)
- MR+1 (not from author, not from the person setting CR+1)
Yes, this aligns with what I had in mind (although I failed to express it correctly!)
This is what I was aiming at with the suggestion to allow code owners to COR+1 themselves, except that I wasn't sure if a rule like "author must have COR rights" can be implemented in Gerrit. If we can write such a rule, then yes, having the author set the bit explicitly is unnecessary, it's enough to let the framework confirm that they would have the rights to do so.
Ah right, I think I understand your concerns now.
At this point, I am not sure how easy (if possible at all) it will be to enforce all of this in Gerrit. But right now, it's only based on the honor system as you pointed out earlier so I think we should focus on establishing the rules, start using them in practice in day-to-day work and if we're satisfied with them after trying them out, start ACL'ing them in Gerrit.
Does that sound reasonable?
Note that in some cases, we would need to mix both policies. If a patch modifies several modules, some of them having multiple code owners and others having a single code owner, we would apply one or the other policy for each module.
Right, would have to apply this check for each module individually.
Also, 1) assumes we always want different individuals doing the code-owner review and the maintainer review. I personally don't feel strongly about this split and I would be fine with the same person doing both types of review, as they are typically looking at different criteria.
Either option seems reasonable to me. I expect in most cases the code owners will not also be maintainers anyway, so for the few cases where they are, it's probably only fair to hold them to the same standard (i.e. ensure that at least two people reviewed the patch).
IOW, you're fine with both options but you'd prefer to have different people setting MR+1 and COR+1 (or CR+1 in the special cases mentioned above), right? Just want to double-check I understood your position.
Regards, Sandrine