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