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 Backports

[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.