Require multiple reviewers for PRs about community HOT 26 CLOSED

I'm not sure we've got CODEOWNERS configured correctly across the various sigstore repos, per the documentation:

e.g. sigstore/cosign/CODEOWNERS should probably read:

*            @sigstore/cosign-codeowners

instead of:

@sigstore/cosign-codeowners

because otherwise the entire list of members of that team would be getting added as reviewers to all PRs (which I don't think is happening right now)?

The three changes I would like to propose are:

1. Get the right people reviewing PRs through setting up CODEOWNERS correctly at a more granular level - across at least cosign, fulcio, rekor
2. For "non-trivial" changes (e.g. typos, update go.mod, etc), a 24 hour embargo period so that community members can +-1, make comments, etc. I hope we could avoid formally defining "non-trivial" and rely on good judgement rather than default to looking at LoC or PR size & automation as an enforcement mechanism
3. A /hold feature for non-trivial changes, where a maintainer or community member can ask for:
   • more time to review it personally before it merges
   • more reviewers to weigh in
     Until the /hold is removed, we shouldn't merge the PR but instead use GitHub, Slack and/or mailing lists to drive towards a decision on it. I'd be curious to hear other's opinion as to whether this /hold should expire after some number of days for a comment period where the consensus of maintainers should prevail.

I agree with @lukehinds and @haydentherapper - I think we could do better at getting more reviews (both quantity and quality), especially as we push towards a GA state. Personally I have had a few experiences where a non-trivial PR is merged within a number of hours before I've had a chance to review it - which was frustrating.

I also agree with the sentiment expressed @dlorenc that we don't want to decrease our velocity and add unnecessary friction and process if it isn't needed. Part of what makes this community a fun place to engage and collaborate is the quick turnaround time on issues and features.

I'd propose we make these three changes, keep a pulse on how things are going, and revisit things after 6-8 weeks to see what further adjustments (if any) might be needed.

from community.

+1

@lukehinds @dlorenc since you are the admins for the org, can we enforce this for the main repos?

from community.

One thing we need to consider, this makes sense for the large projects (fulcio, rekor, cosign), but some of the smaller projects only have a small amount of reviewers (sometimes only two). If someone make a PR that is also a reviewer (and can't self review), they won't be able to hit the needed quorum.

My thought are this could easily be resolved, but requiring all projects (and any new projects) have a minimum of three reviewers.

from community.

Tekton! The core project is struggling and maintainers constantly need to beg for a second set of eyes on minor PRs, which turn into rubber stamps and don't end up adding value anyway.

This leads to slower changes, which means people to try submit larger ones to get more work done in one chunk, which means the reviews take even longer. It's a compounding effect that creeps up over time.

Please - I'm really begging everyone to not make this change. This is a blunt hammer that fixes a problem we don't have. Kubernetes and many other large, successful projects with healthy communities operate fine without this mandatory two person review requirement.

from community.

The scenario I would like to improve is where a PR gets made and within a short space of time it gets merged before it's had a good chance for other maintainers to view the change.

from community.

I support this, especially with GA on the near horizon. It also gets us on a nice trajectory for SLSA level 4.

Adding @bobcallaway for his views.

from community.

agree, I think we should enforce for the large repos like, cosign, rekor, fulcio

for the medium/smaller we can keep as is as we are building the community around, as soon we have enough contributors in the other repos we enforce this there as well

from community.

I'm very very very strongly against this as a requirement. It will kill velocity.

I think we can document guidelines on who should review which changes and ask to get multiple eyes on larger ones, but ultimately we need to trust maintainers to use their judgment.

An across the board requirement removes that discretion from maintainers and eliminates the ability for maintainers to quickly merge dependabot PRs, small bug fixes, and minor doc changes.

from community.

I'll also add that the SLSA requirement does not require two people to review/approve every change. It requires that projects prevent unilateral changes. In some cases this may require a second review, but not always.

If maintainer A sends a PR and maintainer B approves it, that's two people.

If random unknown GitHub account sends a PR and maintainer A approves it, we can't prove that's two people because the random unknown account may be a sock puppet account from the maintainer.

That's the intended goal of the SLSA requirement, which we can hit without enforcing this requirement.

from community.

@dlorenc do you have anything we can use a case study on where its had a negative impact on velocity? I can't see it being an issue myself, as priority issues can be expedited through direct contact between maintainers. regarding small issues, it should not be difficult to co-ordinate people merging dependency updates.

from community.

I'm not sure if there's existing tooling to support this, but could we start by requiring multiple reviewers for larger PRs, as defined by number of lines and/or files changed?

from community.

I'm not sure if there's existing tooling to support this, but could we start by requiring multiple reviewers for larger PRs, as defined by number of lines and/or files changed?

What threshold would you suggest? Maintainers, reviewers, and PR authors are always free to request multiple reviews where they feel it's necessary or useful. I'm not convinced we have a problem here that requires automation.

from community.

PRs that are small in scope and do not add new functionality, like small bug fixes or documentation changes. I expect these PRs would only change a few files.

Without at least the triage permission, contributors are unable to explicitly request multiple reviewers. It'd be up to maintainers to decide when to request additional reviews.

from community.

The scenario I would like to improve is where a PR gets made and within a short space of time it gets merged before it's had a good chance for other maintainers to view the change.

Maybe we could point to some examples there? That's a different problem from the one originally outlined here. I think the right answer there is to just address it right away rather than introduce arbitrary, uniform delays or time requirements.

I'd be fine with something like this text in a maintainers handbook:

"Large, potentially contentious PRs should be left open for long enough to allow multiple maintainers to review. Use your best judgment here as maintainers, and don't be afraid to revert changes or ask for further discussion."

The /hold tool in prow is one way I've seen this applied in practice in the Kubernetes ecosystem to prevent mistakes.

from community.

I can't see a handbook entry honestly making much difference. We already have sections in contributors saying to raise an issue first and provide a helpful descriptive commit message.

from community.

Do you think auto-assigning codeowners/maintainers to do reviews would help reduce friction with having multiple reviewers?

from community.

Do you think auto-assigning codeowners/maintainers to do reviews would help reduce friction with having multiple reviewers?

I think github already does that, it looks at the codeowners file and who has worked on the file before.

from community.

Without at least the triage permission, contributors are unable to explicitly request multiple reviewers. It'd be up to maintainers to decide when to request additional reviews.

This is an easy fix! I don't really understand why GitHub works this way, but we can just add contributors to the org to get people the permissions they need.

from community.

I think github already does that, it looks at the codeowners file and who has worked on the file before.

This may have to be configured, I haven't seen this happen. If we do want to enable this, it may be worthwhile making sure all codeowners know they'll be auto-assigned. From looking over the past few weeks of PRs, it seems like only a handful of codeowners regularly do code reviews.

This is an easy fix! I don't really understand why GitHub works this way, but we can just add contributors to the org to get people the permissions they need.

Yea, I was also surprised, I never understood why a PR author couldn't request their own reviewers, at least after they've made a couple of contributions.

from community.

Yea, I was also surprised, I never understood why a PR author couldn't request their own reviewers, at least after they've made a couple of contributions.

There's a self-service tool for this called peribolos used in Kubernetes where people can request they be added to an organization automatically: https://github.com/kubernetes/test-infra/tree/master/prow/cmd/peribolos
It typically runs inside prow, but I think it can work in a github action too.

I'm happy to just add people manually for now if they file issues in sigstore/community.

from community.

That sounds good with me Bob.

As for /hold expiring, I'd say let's start with not having it expire and see if there's any issues. I'd prefer that whoever initiated the hold come back and remove the hold once they're satisfied with the resolution.

from community.

I'd propose we make these three changes, keep a pulse on how things are going, and revisit things after 6-8 weeks to see what further adjustments (if any) might be needed.

I'm fine with this as an experiment, but I'd like to try to formalize it a bit more first. I'd like to see how many additional reviews we actually get with the 24 hour hold, and what the average merge latency is, before and after.

It's not that I don't like quality! I've just found that any additional hurdles and process and latency have the opposite effect. People become less willing to make small changes when the minimum latency goes from hours to a day.

As an example, the large rekor refactor a few weeks ago would not have been possible without your fast turnaround time on each individual PR that allowed me to break it up into bite size chunks.

Also, cosign has been GA for over 6 months now and I still feel things are working fine so I'm really hesitant to add this there. What we have is working there, and I'm really against adding this.

from community.

A /hold feature for non-trivial changes, where a maintainer or community member can ask for:

• more time to review it personally before it merges

I don't think we need it to expire, but only maintainers should be able to actually place the hold or remove it.

from community.

+1 @bobcallaway 's proposal.

One question, to implement /hold we need to integrate prow. I can't profess to know prow to well, but is this something we need to host somewhere or is it a bot?

from community.

we can deploy prow and use those things or we can write some actions to do that similar work for us as well.

happy to help in any option

from community.

Closing as we haven't needed this

from community.