Skip to content
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

Add PR approval check for specific directories #31

Merged
merged 12 commits into from
Jul 10, 2024

Conversation

jaisnan
Copy link

@jaisnan jaisnan commented Jul 2, 2024

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.

Add check only for library and challenges folder

python error when checking for approvals

replace with JS

Add script to check for no of approvers

Re-trigger same run, instead of creating new run

Remove pull_request trigger

Add workflow step

Test re-trigger when second approval happens

Optimize

Add committe checking flow

add second account to committee for testing

Add comma

Add try/catch block around the parsing

change toml file name

Change parsing

fix name of toml file
@jaisnan jaisnan changed the title Pr approval workflow Add Pr approval workflow for specific directories Jul 2, 2024
@jaisnan jaisnan changed the title Add Pr approval workflow for specific directories Add Pr approval check for specific directories Jul 2, 2024
@jaisnan jaisnan changed the title Add Pr approval check for specific directories Add PR approval check for specific directories Jul 2, 2024
@jaisnan jaisnan marked this pull request as ready for review July 3, 2024 18:21
@jaisnan jaisnan requested a review from a team as a code owner July 3, 2024 18:21
@celinval celinval requested a review from tautschnig July 9, 2024 18:54
@tautschnig
Copy link
Member

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
Copy link
Author

jaisnan commented Jul 9, 2024

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
Copy link
Member

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.

Copy link
Member

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@jaisnan
Copy link
Author

jaisnan commented Jul 9, 2024

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.

@jaisnan jaisnan enabled auto-merge (squash) July 10, 2024 16:34
@jaisnan jaisnan merged commit bbfbb19 into model-checking:main Jul 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants