-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Define guidelines for improved four eyes principle #61358
Comments
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)? |
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 |
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. |
|
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. |
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 |
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 |
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. |
This seems a very well balanced proposal. |
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? |
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 |
Process WG notes:
|
@nashif - to create a pull request with the first pass of these new requirements and present to the TSC. |
@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: We have always had the problem in the project of having greatly dissimilar areas wrt engagement, churn, stability, and contribution level. We need to be careful with policies based on the assumption that there is "enough" contributors or reviewers for all areas. 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. 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. |
@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:
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. |
With how the PR is now, I'm not concerned. |
Such a policy should have constraints proportional to the number of people In other words, if only one organization cares about some area then such a One way to solve this is to relax those constraints with time e.g. if |
It looks like this rule won't apply for Intel, see e.g. #61726 |
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". |
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:
Additionally, the following should be considered:
Consider and list other possible guidelines below...
The text was updated successfully, but these errors were encountered: