search

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.82k stars 6.59k forks source link

Introduce a new label to mark PRs as requiring extended review time. #71107

Open nashif opened 7 months ago

nashif commented 7 months ago

Introduction

We currently have labels to mark review as trivial (4h review time), Hotfixes which can be merged immediately to fix major issues in CI and the tree and the default review time of 2 business days.

While this works well for most PRs, we have a class of PRs that introduce fundemental changes that impact more than one area, have the potential of breaking out of tree users or simply require more eyes because of the naturef of the feature being introduced.

Problem description

Some major changes get merged very soon without thorough review.

Proposed change

This RFC proposes for a new label which can be applied by the a contributor, a maintainer or a collaborator in one of the affected areas. The need for the label can be disputed and the label can be removed if it was deemded unnecessary by the maintainer or a member of the releas engineering team.

Detailed RFC

Dependencies

Adapt https://merge-list.zephyrproject.io/ t

Concerns and Unresolved Questions

Need to agree on extended review time and criteria for setting such label, we do not want to get into arguments if that label is needed or now for every second PR...

Alternatives

Extend the default review time of 2 days, which is not a great alternative.

fabiobaltieri commented 7 months ago

I feel like ~this is solving the wrong problem~ the problem this is trying to solve is not just a matter of more or less time. The issue with those PR is not that they should sit for a specific amount of time but rather that we don't have a way to assign an owner that can guide the proposal to completion. For a complex issue is more than just bake time, I'd rather see someone stepping up as assignee and judge on all fronts and not just time, like making sure all the relevant stakeholders have been involved (is 2 reviewer enough? which reviewer etc...).

Maybe we could set up a group of contributors that takes the bullet and babysit these proposals and have a set of responsibilities? Then the person can just use DNM to hold the PR back.

nashif commented 7 months ago

I feel like ~this is solving the wrong problem~ the problem this is trying to solve is not just a matter of more or less time. The issue with those PR is not that they should sit for a specific amount of time but rather that we don't have a way to assign an owner that can guide the proposal to completion. For a complex issue is more than just bake time, I'd rather see someone stepping up as assignee and judge on all fronts and not just time, like making sure all the relevant stakeholders have been involved (is 2 reviewer enough? which reviewer etc...).

Maybe we could set up a group of contributors that takes the bullet and babysit these proposals and have a set of responsibilities? Then the person can just use DNM to hold the PR back.

I feel you are commenting on something other what this proposal is about. You can have a PR that has an assignee who is driving the discussion, multiple reviews, not only 2 etc etc, however the PR might be touching too many things or introducing something new, maybe it was not discussed in any other forum before and it can still get merged fast enought for the wide community and various stakeholders to look at or react to it, what we have seen in the past was:

nashif commented 7 months ago

Maybe we could set up a group of contributors that takes the bullet and babysit these proposals and have a set of responsibilities? Then the person can just use DNM to hold the PR back.

that is a solution to different problem and yes, something we need to address as well. I was talking exactly about this issue earlier today with Benjamin, mostly about what we call "unmaintained areas" or areas with inactive maintainers where we have to reassign. I am not a big fan of calling things unmaintained, anything that is unmaintained should not be in the tree tbh, so having a a devnull target which consists of a group of people who can drive anything not covered is a good idea, not related to this PR though, with or without maintainers/champions driving a PR, some changes would need more time and should not be merged with 2 reviews, i.e. "treewide" and major changes etc.

fabiobaltieri commented 7 months ago

Could use some more data, maybe some examples of PRs that allegedly went bad and what should have been done to make them go better and if having tag that extends the review time to X days would have helped. Also who is supposed to set it? Not much incentives for the author to do it, if they would they'd just DNM.

nashif commented 7 months ago

Could use some more data, maybe some examples of PRs that allegedly went bad and what should have been done to make them go better and if having tag that extends the review time to X days would have helped.

where did I say anything about PRs going bad?

Also who is supposed to set it? Not much incentives for the author to do it, if they would they'd just DNM.

see "Concerns and Unresolved Questions"

aescolar commented 7 months ago

@nashif I think this is a good idea. When we introduced the review-time labels long ago, we introduced the TSC label being "overloaded" to mean both 1 week review and TSC attention being required (https://docs.zephyrproject.org/latest/project/dev_env_and_tools.html#give-reviewers-time-to-review-before-code-merge). We kind of use the arch. wg label in this way also, but not fully. In some cases we do not need to bring something to the TSC, but benefit from getting more time and attention on a PR. So I would be happy with this new label. Wrt. to @fabiobaltieri comment, assignees, and other reviewers should already try to do what you are describing, but it is too easy to forget or overlook doing it, as we are all busy.

fabiobaltieri commented 7 months ago

Wrt. to @fabiobaltieri comment, assignees, and other reviewers should already try to do what you are describing, but it is too easy to forget or overlook doing it, as we are all busy.

Are they really trying to handle PRs as "this is not ok to be merged with 2 reviewers and 2 days, but it's fine to merge with the very same 2 reviewers and 5 days"? They probably want some more specific reviewers to review, but not as a "hey X an Y, could you review this before it gets in? Or completely ignore for 5 days, either is fine."

If a PR needs more attention from a general broader audience there's already the Arch WG tag. For complex PR wouldn't it make more sense to have a mechanism where we could flag a requirement of specific approval for some specific people?

aescolar commented 7 months ago

Well.. in general it does not harm to have a label to indicate that we need to give people a bit more time if it is not abused. The point is not that a PR just needs to shimmer, as much as that some PRs change things which require people to spend a couple of hours thinking about it.

But I'd be also very happy with a better mechanism to signal who is expected to review, and whose review is considered gating. Let me propose again that it would be nice if the bot generated a comment with a list of areas with a weight (say lines of diff) and the respective maintainers and collaborators in that area. I think that would help everybody involved know who is expected to chip in more, and could help realize about patterns we'd want to automate.

If a PR needs more attention from a general broader audience there's already the Arch WG tag.

Yes and no. It gives some visibility, but the label may be added the same day as the meeting and removed a few hours later, and the PR merged all in 2 days.

pdgendt commented 6 months ago

Well.. in general it does not harm to have a label to indicate that we need to give people a bit more time if it is not abused. The point is not that a PR just needs to shimmer, as much as that some PRs change things which require people to spend a couple of hours thinking about it.

I thought the RFC label had this purpose, maybe it should imply DNM.

fabiobaltieri commented 6 months ago

Counter proposal: internally we have a system where we can tag a PR so it must be approved by a certain set of people before it can be merged, how about implementing something like that? On our side it works by adding a "WANT_LGTM=username,username,username..." line to the commit message, we could do the same, the DNM workflow could get the list from the PR message, check the approval and block if needed.

That'd cover the use case where you see a PR want to make sure that the right people approve it, regardless of the time. I don't really see myself ever thinking "I really want these people to approve this, or just ignore it for long enough, either is fine".

keith-zephyr commented 6 months ago

Process WG notes 2024-04-24:

AIs: @nashif to add support for new Extended Review label @nashif to also provide an example PR that would be solved by the new extended review label

@nashif There is outdated language that required more review, including TSC, Security, and maintainer labels @nashif - the dashboard now makes merging almost automatic once requirements are met (2 approvals, assignee approved, 2 day soak). This proposal is to tag a small set of PRs that warrant more reviews. Treewide changes, multiple vendors, etc. @nashif how to block large scale PRs to ensure there is consensus. @fabiobaltieri agrees with the concept, but just extended the PR soak time to 5 days doesn't really solve the problem. Fabio's proposal is to add a tag for people that are required to review and approve. E.g. multiple subsystems, each maintainer from each subsystem would need to approve. @dleach02 - doesn't the bot already pull in the correct people? @fabiobaltieri concern is that there is only one assignee.
@fabiobaltieri - dashboard could be modified to require all assignees to approve before merging. @dleach02 - what about the code owners tag? @fabiobaltieri code owners mostly provides push notifications for code areas. @nashif - doesn't want to use assignee, this interferes with normal process. Generally the appropriate people are already tagged as reviewers. @nashif - is there a danger to having too many assignees with @fabiobaltieri proposal @dleach02 - PR authors could use the DNM label to ensure the PR is held until specific reviewers. @nashif - DNM is more for PRs with other problems. @dleach02 - how to ensure the proper reviewers look at the PR? @fabiobaltieri - this label only serves to allow PRs to merge after 5 days, and the author can say "you had your 5 days". @nashif - new label and WANT_LGTM can compliment each other @dleach02 - why is 5 days enough? We really want reviews from key individuals. @nashif - extended review can be removed if a maintainer sees the appropriate reviews have been granted. @dleach02 - it would be assignee responsibility to remove label once needed approvals occur @fabiobaltieri - proposal to manually add a list of a few reviewers that need to look at it. @nashif - thinks extended review label and tagging specific reviewers are orthogonal @fabiobaltieri - for PRs that need multiple maintainers to review, PRs tend to be longer than 5 days anyway. @fabiobaltieri is okay adding tag as is. It's an easy to add to merge dashboard. @fabiobaltieri - can we delete the time guards for the other labels? @nashif suggest submit a PR with a new proposal. @fabiobaltieri feels this might be a solution looking for a problem, but adding support for the new label is trivial @dleach02 - assignee is meant to be the one person to help unblock a PR. So not a good place to add multiple maintainer areas @fabiobaltieri - another open issue is that no assignee for PRs for new subsystems

keith-zephyr commented 5 months ago

@nashif - to move this to the Release WG