Block pull requests as long as changes are requested (#63)

* Block pull requests as long as changes are requested

We add a new `lgtm/blocked` state that makes sure no pull request gets merged while it still has changes requested.

Signed-off-by: Yarden Shoham <[email protected]>

Author: yardenshoham

README.md

@@ -42,9 +42,11 @@ included in any milestone.
 ### LGTM
 
 The script will maintain each pull request's LGTM count. It will add the
-appropriate label (one of `lgtm/need 2`, `lgtm/need 1`, or `lgtm/done`) based on
-the number of approvals the pull request has. It will also set the commit status
-to `success` if the pull request has 2 or more approvals (`pending` if not).
+appropriate label (one of `lgtm/need 2`, `lgtm/need 1`, `lgtm/done`, or
+`lgtm/blocked`) based on the number of approvals (or change requests) the pull
+request has. It will also set the commit status to `success` if the pull request
+has 2 or more approvals without changes requested (`pending` if not or `failure`
+if changes are requested).
 
 ## Usage
 

src/github.ts

@@ -220,7 +220,9 @@ export const getMilestones = async (): Promise<Milestone[]> => {
   return Object.values(earliestPatchVersions);
 };
 
-export const getPrApprovers = async (prNumber: number) => {
+export const getPrReviewers = async (
+  prNumber: number,
+): Promise<{ approvers: Set<string>; blockers: Set<string> }> => {
   // load all reviews
   const reviews: {
     state:
@@ -244,23 +246,29 @@ export const getPrApprovers = async (prNumber: number) => {
     page++;
   }
 
-  // count approvers by replaying all reviews (they are already sorted)
+  // count approvers and blockers by replaying all reviews (they are already sorted)
   const approvers = new Set<string>();
+  const blockers = new Set<string>();
   for (const review of reviews) {
     switch (review.state) {
       case "APPROVED":
         approvers.add(review.user.login);
+        blockers.delete(review.user.login);
         break;
       case "DISMISSED":
+        approvers.delete(review.user.login);
+        blockers.delete(review.user.login);
+        break;
       case "CHANGES_REQUESTED":
         approvers.delete(review.user.login);
+        blockers.add(review.user.login);
         break;
       default:
         break;
     }
   }
 
-  return approvers;
+  return { approvers, blockers };
 };
 
 export const createBackportPr = async (
@@ -331,7 +339,7 @@ export const createBackportPr = async (
   );
 
   // request review from original PR approvers
-  const approvers = await getPrApprovers(originalPr.number);
+  const { approvers } = await getPrReviewers(originalPr.number);
   await fetch(
     `${GITHUB_API}/repos/go-gitea/gitea/pulls/${json.number}/requested_reviewers`,
     {

src/github_test.ts

@@ -1,7 +1,7 @@
 import { assertEquals } from "https://deno.land/[email protected]/testing/asserts.ts";
-import { fetchBranch, getPrApprovers } from "./github.ts";
+import { fetchBranch, getPrReviewers } from "./github.ts";
 
-Deno.test("getPrApprovers() returns the appropriate approvers", async () => {
+Deno.test("getPrReviewers() returns the appropriate approvers", async () => {
   const prToApprovers = {
     23993: new Set(["delvh", "jolheiser"]),
     24051: new Set(["delvh", "silverwind"]),
@@ -13,7 +13,7 @@ Deno.test("getPrApprovers() returns the appropriate approvers", async () => {
   };
   await Promise.all(
     Object.entries(prToApprovers).map(async ([pr, approvers]) => {
-      assertEquals(await getPrApprovers(Number(pr)), approvers);
+      assertEquals((await getPrReviewers(Number(pr))).approvers, approvers);
     }),
   );
 });

src/lgtm.ts

@@ -1,6 +1,6 @@
 import {
   addLabels,
-  getPrApprovers,
+  getPrReviewers,
   removeLabel,
   setCommitStatus,
 } from "./github.ts";
@@ -14,15 +14,15 @@ export const setPrStatusAndLabel = async (
     number: number;
   },
 ) => {
-  let approvers;
+  let reviewers;
   try {
-    approvers = await getPrApprovers(pr.number);
+    reviewers = await getPrReviewers(pr.number);
   } catch (error) {
     console.error(error);
     return;
   }
 
-  const { state, message, desiredLabel } = getPrStatusAndLabel(approvers.size);
+  const { state, message, desiredLabel } = getPrStatusAndLabel(reviewers);
   const currentLgtmLabels = pr.labels.filter((l) => l.name.startsWith("lgtm/"));
 
   // remove any undesired lgtm labels
@@ -64,18 +64,28 @@ export const setPrStatusAndLabel = async (
 };
 
 // returns the status, message, and label for a given number of approvals
-export const getPrStatusAndLabel = (approvals: number) => {
+export const getPrStatusAndLabel = (
+  reviewers: { approvers: Set<string>; blockers: Set<string> },
+) => {
   let desiredLabel = "lgtm/need 2";
   let message = "Needs two more approvals";
-  let state: "pending" | "success" = "pending";
+  let state: "pending" | "success" | "failure" = "pending";
+
+  if (reviewers.blockers.size > 0) {
+    desiredLabel = "lgtm/blocked";
+    message = "Blocked by " + Array.from(reviewers.blockers).join(", ");
+    state = "failure";
+    return { state, message, desiredLabel };
+  }
 
-  if (approvals === 1) {
+  if (reviewers.approvers.size === 1) {
     desiredLabel = "lgtm/need 1";
     message = "Needs one more approval";
   }
-  if (approvals >= 2) {
+
+  if (reviewers.approvers.size >= 2) {
     desiredLabel = "lgtm/done";
-    message = `Approved by ${approvals} people`;
+    message = `Approved by ${reviewers.approvers.size} people`;
     state = "success";
   }