lib: simply PR checking logic

Author: joyeecheung

lib/collaborators.js

@@ -31,7 +31,7 @@ class Collaborator {
   }
 
   getName() {
-    return `${this.name}(${this.login})`;
+    return `${this.name} (@${this.login})`;
   }
 
   getContact() {

lib/pr_checker.js

@@ -4,8 +4,8 @@ const SECOND = 1000;
 const MINUTE = SECOND * 60;
 const HOUR = MINUTE * 60;
 
-const WAIT_TIME = 48;
-const WAIT_TIME_SINGLE_APPROVAL = 168;
+const WAIT_TIME_MULTI_APPROVAL = 24 * 2;
+const WAIT_TIME_SINGLE_APPROVAL = 24 * 7;
 
 const {
   REVIEW_SOURCES: { FROM_COMMENT, FROM_REVIEW_COMMENT }
@@ -49,12 +49,11 @@ class PRChecker {
     );
   }
 
-  checkAll(comments = false) {
+  checkAll(checkComments = false) {
     const status = [
-      this.checkReviews(comments),
       this.checkCommitsAfterReview(),
       this.checkCI(),
-      this.checkPRWait(new Date()),
+      this.checkReviewsAndWait(new Date(), checkComments),
       this.checkMergeableState(),
       this.checkPRState(),
       this.checkGitConfig()
@@ -70,132 +69,104 @@ class PRChecker {
     return status.every((i) => i);
   }
 
-  getTSCHint(people) {
-    const tsc = people
+  getTSC(people) {
+    return people
       .filter((p) => p.reviewer.isTSC())
       .map((p) => p.reviewer.login);
+  }
+
+  formatReview(reviewer, review) {
     let hint = '';
-    if (tsc.length > 0) {
-      const list = `(${tsc.join(', ')})`;
-      hint = `, ${tsc.length} from TSC ${list}`;
+    if (reviewer.isTSC()) {
+      hint = ' (TSC)';
     }
-    return hint;
+    return `- ${reviewer.getName()}${hint}: ${review.ref}`;
   }
 
-  checkReviews(comments = false) {
-    const {
-      pr, cli, reviewers: { requestedChanges, approved }
-    } = this;
-    let status = true;
-
-    if (requestedChanges.length === 0) {
-      cli.ok(`Requested Changes: 0`);
-    } else {
-      status = false;
-      let hint = this.getTSCHint(requestedChanges);
-      cli.error(`Requested Changes: ${requestedChanges.length}${hint}`);
+  displayReviews(checkComments) {
+    const { cli, reviewers: { requestedChanges, approved } } = this;
+    if (requestedChanges.length > 0) {
+      cli.error(`Requested Changes: ${requestedChanges.length}`);
       for (const { reviewer, review } of requestedChanges) {
-        cli.error(`- ${reviewer.getName()}: ${review.ref}`);
+        cli.error(this.formatReview(reviewer, review));
       }
     }
+
     if (approved.length === 0) {
-      status = false;
       cli.error(`Approvals: 0`);
-    } else {
-      let hint = this.getTSCHint(approved);
-      cli.ok(`Approvals: ${approved.length}${hint}`);
-
-      if (comments) {
-        for (const { reviewer, review } of approved) {
-          if (review.source === FROM_COMMENT ||
-            review.source === FROM_REVIEW_COMMENT) {
-            cli.info(
-              `- ${reviewer.getName()} approved in via LGTM in comments`);
-          }
-        }
-      }
+      return;
+    }
 
-      const labels = pr.labels.nodes.map((l) => l.name);
-      if (labels.includes('semver-major')) {
-        const tscApproval = approved.filter((p) => p.reviewer.isTSC()).length;
-        if (tscApproval < 2) {
-          status = false;
-          cli.error('semver-major requires at least two TSC approvals');
-        }
+    cli.ok(`Approvals: ${approved.length}`);
+    for (const { reviewer, review } of approved) {
+      cli.ok(this.formatReview(reviewer, review));
+      if (checkComments &&
+          [FROM_COMMENT, FROM_REVIEW_COMMENT].includes(review.source)) {
+        cli.info(`- ${reviewer.getName()} approved in via LGTM in comments`);
       }
     }
-
-    return status;
   }
 
-  /**
-   * @param {Date} now
-   */
-  getWait(now) {
+  checkReviewsAndWait(now, checkComments) {
+    const {
+      pr, cli, reviewers
+    } = this;
+    const { requestedChanges, approved } = reviewers;
+    const labels = pr.labels.nodes.map((l) => l.name);
+
+    const isFastTracked = labels.includes('fast-track');
+    const isSemverMajor = labels.includes('semver-major');
+
+    const dateStr = new Date(pr.createdAt).toUTCString();
+    cli.info(`This PR was created on ${dateStr}`);
+    this.displayReviews(checkComments);
+    // NOTE: a semver-major PR with fast-track should have either one of
+    // these labels removed because that doesn't make sense
+    if (isFastTracked) {
+      cli.info('This PR is being fast-tracked');
+    }
+
+    if (approved.length === 0 || requestedChanges.length > 0) {
+      return false;
+    }
+
+    if (isSemverMajor) {
+      const tscApproved = approved
+        .filter((p) => p.reviewer.isTSC())
+        .map((p) => p.reviewer.login);
+      if (tscApproved.length < 2) {
+        cli.error('semver-major requires at least 2 TSC approvals');
+        return false; // 7 day rule doesn't matter here
+      }
+    }
+
     const createTime = new Date(this.pr.createdAt);
     const hoursFromCreateTime = Math.ceil(
       (now.getTime() - createTime.getTime()) / HOUR
     );
-    const timeLeft = WAIT_TIME - hoursFromCreateTime;
-    const timeLeftSingleApproval =
-      WAIT_TIME_SINGLE_APPROVAL - hoursFromCreateTime;
-
-    return {
-      timeLeft,
-      timeLeftSingleApproval
-    };
-  }
+    let timeLeftMulti = WAIT_TIME_MULTI_APPROVAL - hoursFromCreateTime;
+    const timeLeftSingle = WAIT_TIME_SINGLE_APPROVAL - hoursFromCreateTime;
 
-  // TODO: skip some PRs...we might need a label for that
-  /**
-   * @param {Date} now
-   */
-  checkPRWait(now) {
-    const {
-      pr, cli, reviewers, CIStatus
-    } = this;
-    const { approved } = reviewers;
-    const labels = pr.labels.nodes;
-
-    const fast =
-      labels.some((l) => ['fast-track'].includes(l.name));
-    if (fast) {
-      if (approved.length > 1 && CIStatus) {
-        cli.info('This PR is being fast-tracked');
+    if (approved.length >= 2) {
+      if (isFastTracked) {
         return true;
-      } else {
-        const msg = ['This PR is being fast-tracked, but awaiting '];
-        if (approved.length < 2) msg.push('approvals of 2 contributors');
-        if (!CIStatus) msg.push('a CI run');
-
-        let warnMsg = msg.length === 2
-          ? msg.join('') : `${msg[0] + msg[1]} and ${msg[2]}`;
-        cli.warn(warnMsg);
       }
-
+      if (timeLeftMulti < 0) {
+        return true;
+      }
+      cli.error(`This PR needs to wait ${timeLeftMulti} more hours to land`);
       return false;
     }
 
-    const dateStr = new Date(pr.createdAt).toDateString();
-    cli.info(`This PR was created on ${dateStr}`);
-
-    const wait = this.getWait(now);
-    if (approved.length > 1 && wait.timeLeft > 0) {
-      cli.warn(`Wait at least ${wait.timeLeft} more hours before landing`);
-      return false;
-    } else if (approved.length === 1 && wait.timeLeftSingleApproval > 0) {
-      const waitMsg = wait.timeLeft > 0
-        ? ` and ${wait.timeLeft} more hours`
-        : '';
-      cli.warn('Wait at one of the following:');
-      cli.warn(`* another approval${waitMsg}`);
-      cli.warn(`* ${wait.timeLeftSingleApproval} more hours` +
-        ' with existing single approval');
+    if (approved.length === 1) {
+      if (timeLeftSingle < 0) {
+        return true;
+      }
+      timeLeftMulti = timeLeftMulti < 0 ? 0 : timeLeftMulti;
+      cli.error(`This PR needs to wait ${timeLeftSingle} more hours to land ` +
+                `(or ${timeLeftMulti} hours if there is one more approval)`);
       return false;
     }
-
-    cli.info('No more waiting required');
-    return true;
   }
 
   hasFullOrLiteCI(ciMap) {

test/unit/collaborators.test.js

@@ -62,7 +62,7 @@ describe('collaborators', function() {
         collaborators.forEach(collaborator => {
           assert.strictEqual(
             collaborator.getName(),
-            `${collaborator.name}(${collaborator.login})`);
+            `${collaborator.name} (@${collaborator.login})`);
         });
       });
     });