Require multiple reviewers for PRs

[haydentherapper]

Multiple reviewers can help for a number of reasons:

• Provide a different perspective
• Reduce the impact of single account compromise

This can be enforced by Github

This has also been brought up on Fulcio - sigstore/fulcio#374 @nsmith5

Context: New contributor friction log

[cpanato]

+1

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

[lukehinds]

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.

[lukehinds]

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.

[cpanato]

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

[dlorenc]

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.

[dlorenc]

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.

[lukehinds]

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

[dlorenc]

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.

[haydentherapper]

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?

[dlorenc]

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.

[lukehinds]

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.

[haydentherapper]

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.

[dlorenc]

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.

[lukehinds]

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.

[haydentherapper]

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

[lukehinds]

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.

[dlorenc]

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.

[haydentherapper]

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.

[dlorenc]

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.

[bobcallaway]

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.

[haydentherapper]

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.

[dlorenc]

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.

[dlorenc]

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.

[lukehinds]

+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?

[cpanato]

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

[haydentherapper]

Closing as we haven't needed this