search

model-checking / verify-rust-std

Verifying the Rust standard library
https://model-checking.github.io/verify-rust-std/
Other
9 stars 7 forks source link

Add PR approval check for specific directories #31

Closed jaisnan closed 2 weeks ago

jaisnan commented 3 weeks ago

Adds process/CI workflow to check that the approvers for every PR are in a committee of recognized approvers. This is a much simplified version of the r?bot that rust-lang uses.

Testing Scenarios

Happy path

What happens when 2 of the approvers are in the committee

Run: https://github.com/jaisnan/rust-dev/pull/16/checks?check_run_id=26914353663

What if someone not in the list approves, and 1 from the committee approves?

In this scenario, we have a committee that consists of someone called "jaisu-1". But if the approvals came from one ID that's recognized, and another called "Jaisu-1" (Note to the reader: "jaisu-1" != "Jaisu-1"), then the workflow fails and the PR merge is (rightfully) blocked.

Run: https://github.com/jaisnan/rust-dev/pull/15/checks?check_run_id=26914179444

Call-Outs

  1. We need to add a requirement through settings that at least 2 approvers are required before merging, and allow anyone to approve (if it's not already the setting).
  2. Once the first iteration of a committee is finalized, we can merge this workflow to begin the approval checking.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

tautschnig commented 2 weeks ago

Why can we not use a combination of CODEOWNERS (to choose specific reviewers for directories) and branch protection rules (to set the number of approvals) to get this job done?

jaisnan commented 2 weeks ago

Why can we not use a combination of CODEOWNERS (to choose specific reviewers for directories) and branch protection rules (to set the number of approvals) to get this job done?

From my research into CODEOWNERS, it seems like we can enforce these rules using CODEOWNERS + Branch Protection

  1. Any changes to the specified directories require approval from the special-approvers team.
  2. At least 2 approvals are needed before merging.

But not the union of these 2 rules; so essentially even 1 approval from someone from the CODEOWNERS and someone not from the CODEOWNERS would become allowed; which is not something we want.

I could look further into rulesets which might enable us to implement the union of those rules, but at that point imo, using a github workflow would be more transparent and readable to the public.

Lmk if I'm missing a way to implement what you're suggesting.

tautschnig commented 2 weeks ago

But not the union of these 2 rules; so essentially even 1 approval from someone from the CODEOWNERS and someone not from the CODEOWNERS would become allowed; which is not something we want.

Hmm, yes, I think you are right. I don't like to be in a place where we have to maintain such infrastructure, but it seems it's actually unavoidable.

jaisnan commented 2 weeks ago

Ok for this seems to be the best feasible solution - but don't we also need to include an initial committee as part of this PR?

That's already been merged :D. PR where we merged it.