+1 for re-introduction of 'Code-Review' label in addition to 'Code-Owner-Review' label.
I would like to see the enforcement of authors of patches not being allowed to self review eventually for all labels. Hopefully we can enable maintainers who are not the author of deadlocked patches to allow the setting of all required +1's to labels is situations where this is necessary for admin purposes after due consideration. I agree though while this process is settling down an honour system is appropriate.
Cheers
Joanna
On 01/07/2020, 08:56, "TF-A on behalf of Sandrine Bailleux via TF-A" <tf-a-bounces@lists.trustedfirmware.org on behalf of tf-a@lists.trustedfirmware.org> wrote:
Hi Julius,
On 7/1/20 2:13 AM, Julius Werner wrote: > Hi Sandrine, > > Sounds like good changes in general! I'm curious what the ACLs are for > Code-Owner-Review? Is it tied to docs/about/maintainers.rst or just > based on the honor system? (I notice that I seem to be able to give a > +1 for code I'm not owning, but maybe that's because I am a > maintainer?)
Right now, the ACLs are not tied to docs/about/maintainers.rst (any registered user could vote on this label) and it is just based on the honor system. However, I'd like this to be enforced in the future, we just haven't had the time to put the right tooling in place for that.
Also we did not want to spend time on developing such scripts before we tried out these process changes in practice. If they proved too heavy or inconvenient, part of this work would have gone to waste.
BTW, any help for the tooling is welcome! If you've got plugin configuration files or scripts we could reuse (with an appropriate license) or even tips on how to best set this up, please feel free to share these.
> Also, are code owners allowed to +1 themselves (I think > we said we didn't want maintainers to do that, but for code owners I > could see how we might want to allow it since there are usually not > that many)? What do we do when someone uploads the first patch for a > new platform, do they just COR+1 themselves (since there is no code > owner yet)?
That is indeed a grey area. As you say, many modules have a single code owner and we need to agree on how we would like such code reviews to be conducted. Thanks for starting this discussion!
One option as you say would be to allow code owners to self-review their patches but I am not convinced we would gain anything out of this. It sounds like a tick-box exercise to me, an admin overhead just to get the patch through the system and I would like to avoid that as much as possible. It is likely that a patch submitter has already self-reviewed his code before posting a patch anyway.
The alternative we've been discussing in the team is to call out another reviewer in these situations. I think that there is still value in having a second fresh pair of eyes on a patch. Even if the reviewer has no particular expertise on this specific module, they can still catch potential logic problems or structural issues in the code.
The latter would be my preference. What do others think?
> I think it might still be useful to retain the existing Code-Review as > a +1/-1 label next to the two new ones, just to allow other interested > parties to record their opinion (without it having any enforcing > effect on the merge). In other Gerrit instances I have used people > often like to give CR+1 as a "I'm not the one who needs to approve > this but I have looked at it and think it's fine" or a CR-1 as a "I > can't stop you from doing this but I still want to be clear that I > don't think it's a good idea". It allows people outside the official > approval process a clearer way to participate and can aid the official > approvers in their decisions (e.g. when I am reviewing a patch as a > maintainer that already has a CR-1 from someone else I know to pay > extra attention to their concerns, and it's more visible than just > some random comment further up in the list). What do you think?
That's a very good idea, thanks! It also aligns with Javier's concerns, which sound perfectly valid to me.
I think your proposal of having 3 distinct labels is better than having code owners and non-code owners voting on the same generic 'Code-Review' label, which is what Javier was suggesting. It clarifies further the intent of each label and as you say allows us to configure different rules for each (i.e. make the generic 'Code-Review' label informative/optional, while making the 'Code-Owner-Review' label mandatory for patch submission).
I am gonna wait for others' opinions on this before changing the configuration in Gerrit again. I see Raghu agrees with this approach. If nobody disagrees by the end of the week, I'll do these changes on Monday.
In the meantime, as discussed above, any registered user can vote on the new 'Code-Owner-Review' label so let's continue to use that for the rest of this week.
Regards, Sandrine -- TF-A mailing list TF-A@lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a
BTW, any help for the tooling is welcome! If you've got plugin configuration files or scripts we could reuse (with an appropriate license) or even tips on how to best set this up, please feel free to share these.
The Chromium Gerrit has an owners-enforcement system but I'm not familiar with how exactly they set it up. I think they're using this plugin https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/master and this is where it's integrated into Gerrit submission rules: https://chromium.googlesource.com/chromiumos/+/refs/meta/config/rules.pl . It works by having a file called OWNERS in certain directories which sets the owners for the subtree below it, so we would have to rewrite the current code owner list in that format if we wanted to use it.
There also seems to be this completely separate plugin that claims to do roughly the same thing, I have no idea where the difference is between the two: https://gerrit.googlesource.com/plugins/owners/+/refs/heads/master
One option as you say would be to allow code owners to self-review their patches but I am not convinced we would gain anything out of this. It sounds like a tick-box exercise to me, an admin overhead just to get the patch through the system and I would like to avoid that as much as possible. It is likely that a patch submitter has already self-reviewed his code before posting a patch anyway. The alternative we've been discussing in the team is to call out another reviewer in these situations. I think that there is still value in having a second fresh pair of eyes on a patch. Even if the reviewer has no particular expertise on this specific module, they can still catch potential logic problems or structural issues in the code.
I definitely did not mean to suggest that these patches should not be reviewed at all -- I was just talking about the Code-Owner-Review label. There would still be a maintainer review, of course, and it seems like a good idea to get additional reviews from other people too. They just couldn't set the COR+1 bit once we start ACLing it.
Basically, each of these bits have their own purpose, and I would see the purpose of the COR+1 bit to be that the person most familiar with that particular piece of code has made sure that it fits in and doesn't cause unexpected problems with certain quirky configurations or something like that. That's important when, say, I make a generic refactoring that touches a platform I'm unfamiliar with, but if the code owner adds to their own platform that's probably already a given and they are likely still the best person to judge that, even for their own code.
That all code gets reviewed by people other than the author I would see as a somewhat orthogonal concern that should be checked independently. So maybe the rule could be that if the code owner sets COR+1 for themselves, there needs to be at least one Code-Review+1 (if we reintroduce that label) from another person (who is also not the one setting Maintainer-Review+1) to make sure the minimum amount of reviews stays the same. Or maybe these should all be completely independent checks so every patch needs a Code-Owner-Review+1 (can be from the author), a Maintainer-Review+1 (not from the author) and at least two Code-Review+1 from people other than the author -- with the normal expectation that when a maintainer or code owner reviews a patch, they would also set Code-Review+1 (as a general sign that they reviewed the patch) in addition to their special meaning flag.
Hi Julius and all,
As agreed and announced last week, I've now reintroduced the Code-Review label in addition to the Code-Owner-Review and Maintainer-Review ones.
The Code-Review label is purely informational and won't influence whether a patch is submittable in Gerrit. Anyone should be able to vote on this label, if you face any issue please let me know.
On 7/1/20 11:12 PM, Julius Werner wrote:
BTW, any help for the tooling is welcome! If you've got plugin configuration files or scripts we could reuse (with an appropriate license) or even tips on how to best set this up, please feel free to share these.
The Chromium Gerrit has an owners-enforcement system but I'm not familiar with how exactly they set it up. I think they're using this plugin https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/master and this is where it's integrated into Gerrit submission rules: https://chromium.googlesource.com/chromiumos/+/refs/meta/config/rules.pl . It works by having a file called OWNERS in certain directories which sets the owners for the subtree below it, so we would have to rewrite the current code owner list in that format if we wanted to use it.
This "find-owners" plugin sounds promising! Thanks for the pointers.
I notice the "reviewers" plugin [1] is installed on review.tf.org but it does not sound as configurable as "find-owners".
Rewriting the current code owners list to use the OWNERS format does not sound like a big task to me (just a dull one... could be scripted, though) so this approach definitely sounds worth investigating to me.
[1] https://review.trustedfirmware.org/plugins/reviewers/Documentation/index.htm...
There also seems to be this completely separate plugin that claims to do roughly the same thing, I have no idea where the difference is between the two: https://gerrit.googlesource.com/plugins/owners/+/refs/heads/master
I notice the "find-owners" plugin documentation mentions that "this plugin works with Gerrit projects that use Android or Chromium compatible OWNERS files" so maybe they started off from the generic "owners" plugin and customized it for Android/Chromium's needs? Just a guess.
Anyway, worth a look as well, thanks!
[2] https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/master/src/...
One option as you say would be to allow code owners to self-review their patches but I am not convinced we would gain anything out of this. It sounds like a tick-box exercise to me, an admin overhead just to get the patch through the system and I would like to avoid that as much as possible. It is likely that a patch submitter has already self-reviewed his code before posting a patch anyway. The alternative we've been discussing in the team is to call out another reviewer in these situations. I think that there is still value in having a second fresh pair of eyes on a patch. Even if the reviewer has no particular expertise on this specific module, they can still catch potential logic problems or structural issues in the code.I definitely did not mean to suggest that these patches should not be reviewed at all -- I was just talking about the Code-Owner-Review label. There would still be a maintainer review, of course, and it seems like a good idea to get additional reviews from other people too. They just couldn't set the COR+1 bit once we start ACLing it.
Right, I see (and agree with you).
Basically, each of these bits have their own purpose, and I would see the purpose of the COR+1 bit to be that the person most familiar with that particular piece of code has made sure that it fits in and doesn't cause unexpected problems with certain quirky configurations or something like that. That's important when, say, I make a generic refactoring that touches a platform I'm unfamiliar with, but if the code owner adds to their own platform that's probably already a given and they are likely still the best person to judge that, even for their own code.
Yes, that makes sense to me.
That all code gets reviewed by people other than the author I would see as a somewhat orthogonal concern that should be checked independently. So maybe the rule could be that if the code owner sets COR+1 for themselves, there needs to be at least one Code-Review+1 (if we reintroduce that label) from another person (who is also not the one setting Maintainer-Review+1) to make sure the minimum amount of reviews stays the same. Or maybe these should all be completely independent checks so every patch needs a Code-Owner-Review+1 (can be from the author), a Maintainer-Review+1 (not from the author) and at least two Code-Review+1 from people other than the author -- with the normal expectation that when a maintainer or code owner reviews a patch, they would also set Code-Review+1 (as a general sign that they reviewed the patch) in addition to their special meaning flag.
I still don't see the benefit of code owners setting COR+1 for themselves. I would think that a patch owner already reviews his own patch before posting it for review so a self COR+1 is just reinstating the obvious in my eyes.
Your first suggestion (i.e. having at least one CR+1 from another person when a patch author cannot find another code owner to review their patches) sounds aligned with what I proposed earlier.
Your second suggestion sounds a bit too heavy to me. The fact that people would have to replay their vote on several labels sounds like an admin overhead to me. I guess this would simplify the ACL configuration but I would prefer to avoid this if we can.
So I suggest the following. We could have 2 ways to get a patch approved.
1) 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)
2) 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)
In 2), I like your suggestion of mandating 2 CR+1 but I fear this might be difficult to achieve in practice (simply because we might not have enough reviewers for this).
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.
How does that sound?
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.
What do others think?
Regards, Sandrine
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? In that case, you would have to write this slightly differently:
1) Normal case: * COR+1 (not from author) * MR+1 (not from author, not from the person setting COR+1)
2) 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)
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.
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).
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
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?
Yeah, sure, I think that's reasonable. I don't think keeping the owner review flag on the honor system is a big problem actually (it's not like we expect our contributors to intentionally break the rules), I just asked about it originally because I was curious what your plans were.
If we start doing this now and want to switch it to a plugin-supported enforced system later I think we may find that there might be no plugin allowing us to exactly implement what we want though, and then we'll have to change how things work again.
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.
Right.
tf-a@lists.trustedfirmware.org
-
Joanna Farley -
Julius Werner -
Sandrine Bailleux