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

[Oppia Android] Require new files to be re-approved #278

Open
BenHenning opened this issue Jul 12, 2021 · 3 comments
Open

[Oppia Android] Require new files to be re-approved #278

BenHenning opened this issue Jul 12, 2021 · 3 comments

Comments

@BenHenning
Copy link
Member

BenHenning commented Jul 12, 2021

Is your feature request related to a problem? Please describe.
It's possible with current GitHub codeowners to run into a situation where reviewers approve a PR, then the author adds a new file to the PR that's owned by an existing PR approver & change it, then merge the PR without a follow-up review. While good reviewer practices make this harder (e.g. not approving until all CI checks are passed and only nits remain), it's still possible and can sometimes go undetected.

Describe the solution you'd like
It would be ideal if Oppiabot could detect cases when new files are deleted, added, or modified which fall under existing owners that have already approved, and either re-request their approval or request changes on the PR itself to block it from being merged & then reassign the related codeowners until they can take a look.

One way to look at this is: when a codeowner approves a PR, they are only approving the set of files they own at the time of approval. Any changes to that set should require another approval pass from the codeowner.

Describe alternatives you've considered
Three alternatives are:

  1. Continue with manual effort to try to prevent this (either through post-merge auditing or only approving right before merging), neither of which are ideal since they slow down the review-merging process.
  2. Prevent PR authors from merging (not ideal for core team members that might accidentally run into this situation).
  3. Enable GitHub's setting to re-require approval on any commit (not ideal since it would require substantially more reviews from existing reviewers)

Additional context
This is a scenario that was noticed specifically on Oppia Android.

@BenHenning
Copy link
Member Author

/cc @jameesjohn & @seanlip

@jameesjohn
Copy link
Contributor

Thanks, @BenHenning.

Currently, the GitHub API does not list the reviewers who have approved the PR. However, it removes the reviewer from the list of reviewers after a review (approved or changes requested). So what we could do is to check if the code owner of that file/set of files is not among the list of reviewers (they have either approved or requested changes before now), Oppiabot requests a review from them and assigns them to the PR.

What do you think?

@BenHenning
Copy link
Member Author

@jameesjohn that sounds sensible since it also covers the case of explicitly requesting a review from someone who owns newly affected files (GitHub does this automatically, but without the message that Oppiabot would leave so the extra redundancy is nice).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

3 participants