Define guidelines for improved four eyes principle

[nashif]

Background for this was feedback we got from the community in the 'Meet the Maintainers' session during ZDS 2023.

Right now we require at least 2 approvals (4 eyes) for a pull request to be merged. In the Zephyr case, all eyes (submitter, approvers and merger) can be of the same organisation or team. A change that might seem harmless and if merged quickly to address an issue or add a feature without having being reviewed by a larger group of users might have negative effects and should be avoided.
Ideally we want at least one set of eyes looking at the changes from a different organisation, This for example could be the person merging the change, however, having reviews and approvals from other organisation will simplify things further and the merger + the approval of the assignee removes any ambiguity about the review.

We can further optimize this as we go, but at minimum we shall avoid the following:

• (a) Submitter, Approvers and Merger are from the same organisation

Additionally, the following should be considered:

• (b) Changes to common and shared code shall always have reviews from different organisations (at least one review and approval from a different organisation as the submitter)
• (c) with changes limited only to platform code (driver, soc, boards), at least the merger shall be from a different organisation.
• ....

Consider and list other possible guidelines below...

[henrikbrixandersen]

Thank you for starting the dialogue on this. I think (a) and (b) are a very good start, not sure how much benefit we will get from enforcing (c)?

[nordicjm]

(a) Submitter, Approvers and Merger are from the same organisation

I have a problem with this idea, it is basically impossible to get people to review a lot of PRs hence the backlog in the dev review meeting, and this makes things worse. An example of a system that only gets reviews from the same organisation is MCUmgr: code is submitted by me (or de-nordic), the other approves it, then generally is merged by Carles - how does it in any way help if instead we now need to find someone else to review it (apparently no-one else does) or have to get someone else to merge it that isn't going to know anything about that system. For some systems where there's a lot of contributors it makes sense, but for others this just seems like a really pointless exercise. Areas where this will not work due to lack of people reviewing things that I commonly see: MCUmgr, MCUboot, retention, retained memory, documentation, sysbuild, tests, scripts, satellite, (some) drivers

[nashif]

how does it in any way help if instead we now need to find someone else to review it (apparently no-one else does) or have to get someone else to merge it that isn't going to know anything about that system.

In this case someone from some other org should merge, and that will make things completly transparent. We have 15 (Fifteen) people with merge rights, so it does not need to be someone from the same org and we have dedicated release engineers actively monitoring PRs.
The problem here will be dealing with exceptions and there will be exceptions and we have to work around special cases, as long as we follow the guidelines for most cases in place and reviewers, submitters and release engineers (mergers) are aware of this.

[nordicjm]

how does it in any way help if instead we now need to find someone else to review it (apparently no-one else does) or have to get someone else to merge it that isn't going to know anything about that system.

In this case someone from some other org should merge, and that will make things completly transparent. We have 15 (Fifteen) people with merge rights, so it does not need to be someone from the same org and we have dedicated release engineers actively monitoring PRs.

Then things like this end up happening:
#60199
#60207

[nashif]

Then things like this end up happening:
#60199
#60207

I can list 100s of PRs where this happens and this is not limited to mcumgr or myself. In this case we are dealing with backports which are usually dealt with in the release meeting, so there seems to be a gap, but this really has nothing to do with this proposal.

[nordicjm]

Fair comment for backport PRs. But there needs to be a way to deal with areas that only have 1 organisation or person contributing to them e.g. ec_host mgmt #57287 whereby it's just google. If we could get more people reviewing all types of PRs that would be great but it's not something that can be forced

[nashif]

Then things like this end up happening:
#60199
#60207

for starters, those are manual Backports, so they did not get the 'Backport' label and were never added to the Backport project that is usually reviewed on a weekly basis in the release meeting.

added now, see https://github.com/orgs/zephyrproject-rtos/projects/32

[nashif]

e.g. ec_host mgmt #57287 whereby it's just google.

interestingly, the PR is being also reviewed by @erwango, so that should have gotten a review from him :)

Anyways, as I said, there will always be exceptions and some areas where we have one org contributing and reviewing, I tried to cover this in point (c) at least for platforms, but we can extend that to other grey areas.

This is just a beginning of a proposal and we need to to get more input and be able to deal with exceptions etc. So please follow this as it evolves and see if it still a problem when we have reached a conclusion.

[Laczen]

This seems a very well balanced proposal.

[fabiobaltieri]

Doesn't sound too far from what I think we are already doing so it may make sense to put it down as a guideline, but there's many judgment calls that would be hard to formalize, let alone monitor. (emails in github are a bit of a disaster, how do you find the org of a user?)

[henrikbrixandersen]

Doesn't sound too far from what I think we are already doing so it may make sense to put it down as a guideline, but there's many judgment calls that would be hard to formalize, let alone monitor. (emails in github are a bit of a disaster, how do you find the org of a user?)

Well, do we really need an automated check for this? Isn't a policy enough?

[fabiobaltieri]

Well, do we really need an automated check for this? Isn't a policy enough?

Yeah but imagine wanting to write a check or search pattern or something to flag, say, PR with two reviewers from different companies, or different companies from my own user or some other filter like that to make the operational life a it easier. Well you probably can't, so you end up relying on release eng keeping track of who works where.

[nashif]

Yeah but imagine wanting to write a check or search pattern or something to flag, say, PR with two reviewers from different companies, or different companies from my own user or some other filter like that to make the operational life a it easier. Well you probably can't, so you end up relying on release eng keeping track of who works where.

we will figure out some way to enforce this or automate aspects of that down the road, we need the policy set first. I also believe that many of those who merge the most (a few of us) by now are familiar with many of the contributors and maintainers. I see such cases very often and tbh, I do merge PRs submitted and reviewed only by folks in my organisation and I will and other will pay more attention once we have a policy in place.

If we agree that Submitter, Approvers and Merger SHALL NOT be from the same organisation, then as a merger, you will be familiar with the submitters or the approvers if they are from the same org :)

[fabiobaltieri]

Process WG notes:

• @dleach02 many of the current reviewers already use this policy (no submitter+approver+merger from the same company)
• @nashif some PRs are merged quickly when it's not really necessary
• @kartben it's a good idea to use tooling to flag when PRs are ready
• @galak need to be cogniscent on how that's implemented, many reviewer may just wait for the tag to go before reviewing
  • we are also trying to move to a mode where merges may be more auotmated, may be hard to figure out who needs to review from the path
• @nashif there' existing tooling in github to do this, but it may not work in our case
• @MaureenHelm maybe we could validate if the assignee approve with a bot
• @nashif we need to figure how to implement this with the github APIs

[keith-zephyr]

@nashif - to create a pull request with the first pass of these new requirements and present to the TSC.

[aescolar]

@nashif The problem description in #61358 (comment) seems too vague to me: Have there been actual problems in some areas? were those areas related to common code (driver APIs, kernel,..)? were the normal review times respected in those cases? did affected maintainers review and did they have reasonable time to review?

In any case this seems to be leading to a too "simple" solution that may easily cause trouble:
In some areas we just do not have enough contributors/organizations, and even the current 2 review policy is a pain.

We have always had the problem in the project of having greatly dissimilar areas wrt engagement, churn, stability, and contribution level.
That is, in some areas we have lots of potential knowledgeable reviewers, in some areas we have a single company doing the work, in some a single person. But how many organizations/contributors work in an area does not correlate with how many bugs are there or how many regressions introduced. It does on the other hand correlate very strongly with how fast changes can get in.

We need to be careful with policies based on the assumption that there is "enough" contributors or reviewers for all areas.
Also we need to be careful with policies that require reviewers without knowledge of an area to review it.

Outside reviewers (people not engaged in a subsystem) do not have the same motivation to get fixes done, and improvements in. But negative reviews have a tendency to bog down or permanently block PRs even if the rejection is on some triviality.
I have seen too many reviews in which "outsider" reviewers do not understand the original code causing unnecessary delays, or argue about tangential topics or start bikeshedding, and in too many cases reject/block and never come back. (This is just human nature, we are all busy).
To that, add that most people walk on eggshells wrt to dismissing reviews, or merging if there is any indication of possible disagreement, specially in areas with little attention.

If there is some areas where we have changes going in that shouldn't have gone in, maybe we should talk about those, and in what particular circumstances that happened.

[nashif]

@aescolar not following what is vague in the above and if you follow the initial description and comments, this is not immediately asking for a change in how we review and aknowledges the fact we have areas where this will be a problem, the major ask for now is:

We can further optimize this as we go, but at minimum we shall avoid the following:

(a) Submitter, Approvers and Merger are from the same organisation

and then set the guideline that we should at least attempt to have more reviewers from different organisations where possible.

and this is reflected in the related PR.

[aescolar]

@aescolar not following what is vague in the above...

With how the PR is now, I'm not concerned.
My concern was with the original PR wording, and some of the comments above, as it appeared to me as setting the bar too high given what is typically possible in quite a few areas.

[npitre]

Such a policy should have constraints proportional to the number of people
being affected, which is normally a good proxy for the number of people
with the proper knowledge and interest to provide a review.

In other words, if only one organization cares about some area then such a
policy will simply create frustration towards the whole project and aleniate
that organization.

One way to solve this is to relax those constraints with time e.g. if
no one from a different organization provided a review after a week or so
then such requirement is no longer required. This should give plenty of time
for changes to areas with high impact to trigger reviews and change requests
from different organizations without making the narrow-impact narrow-interest
areas stuck forever. And if a change with high impact is consensual then
gathering approvals from different organizations quickly won't be a problem.

[gmarull]

It looks like this rule won't apply for Intel, see e.g. #61726

[marc-hb]

I tried to cover this in point (c) at least for platforms, but we can extend that to other grey areas.

One way to solve this is to relax those constraints with time e.g. if
no one from a different organization provided a review after a week or so
then such requirement is no longer required.

I think that's a good point. It's desirable to reduce the number of variables in policies to make them simpler but it can go too far. The current description already mentions that the guidelines must (obviously) be relaxed for vendor-specific code. So I think that time-in-review should also be a valid "relaxation" per the usual "you had your chance and you said nothing" (assuming good Github notifications, another big problem).

Another variable is the size and invasiveness of the change. For instance I just submitted a one-line, cmake error message fix and I don't think very scarce reviewers' time should be wasted on that due to too much "red tape". This is hard to measure but there are some objective factors: for instance standalone changes not part of a long chain of dependencies are easier to revert if needed.

There is unfortunately some "combinatorial" explosion of these parameters: for instance waiting 1 week in review may be enough for minor change but not for a more significant one; some people take vacations sometimes.

"As simple as possible, but no simpler".