Automatically remove stale build label after the PR has been updated (#…

…173)

* automatically remove stale build label after the PR has been updated

* address review comments

* move from synchronize to push

constants.js

@@ -46,10 +46,11 @@ const issuesLabelCheck = 'issues-labeled-check';
 const issuesAssignedCheck = 'issues-assigned-check';
 const forcePushCheck = 'force-push-check';
 const pullRequestReviewCheck = 'pr-review-check';
-const codeOwnerCheck = 'code-owner-check'
+const codeOwnerCheck = 'code-owner-check';
 const ciFailureCheck = 'ci-failure-check';
 const updateWithDevelopCheck = 'update-with-develop-check';
 const respondToReviewCheck = 'respond-to-review-check';
+const oldBuildLabelCheck = 'old-build-label-check';
 
 const checksWhitelist = {
   'oppia-android': {
@@ -92,7 +93,7 @@ const checksWhitelist = {
     [issuesLabelEvent]: [issuesLabelCheck],
     [issuesAssignedEvent]: [issuesAssignedCheck],
     [unlabelEvent]: [datastoreLabelCheck],
-    [pushEvent]: [forcePushCheck],
+    [pushEvent]: [forcePushCheck, oldBuildLabelCheck],
     [periodicCheckEvent]: [periodicCheck],
     [pullRequestReviewEvent]: [pullRequestReviewCheck],
     [checkCompletedEvent]: [ciFailureCheck],
@@ -147,7 +148,8 @@ module.exports.pullRequestReviewCheck = pullRequestReviewCheck;
 module.exports.codeOwnerCheck = codeOwnerCheck;
 module.exports.ciFailureCheck = ciFailureCheck;
 module.exports.updateWithDevelopCheck = updateWithDevelopCheck;
-module.exports.respondToReviewCheck = respondToReviewCheck
+module.exports.respondToReviewCheck = respondToReviewCheck;
+module.exports.oldBuildLabelCheck = oldBuildLabelCheck;
 
 module.exports.getBlacklistedAuthors = function() {
   return blacklistedAuthors;

index.js

@@ -38,6 +38,7 @@ const periodicCheckModule = require('./lib/periodicChecks');
 
 const constants = require('./constants');
 const checkIssueAssigneeModule = require('./lib/checkIssueAssignee');
+const staleBuildModule = require('./lib/staleBuildChecks');
 
 const whitelistedAccounts = (process.env.WHITELISTED_ACCOUNTS || '')
   .toLowerCase()
@@ -128,12 +129,15 @@ const runChecks = async (context, checkEvent) => {
             await Promise.all([
               periodicCheckModule.ensureAllPullRequestsAreAssigned(context),
               periodicCheckModule.ensureAllIssuesHaveProjects(context),
-              periodicCheckModule.checkAndTagPRsWithOldBuilds(context)
+              staleBuildModule.checkAndTagPRsWithOldBuilds(context)
             ]);
             break;
           case constants.respondToReviewCheck:
             await checkPullRequestReviewModule.handleResponseToReview(context);
             break;
+          case constants.oldBuildLabelCheck:
+            await staleBuildModule.removeOldBuildLabel(context);
+            break;
         }
       }
     }

lib/periodicChecks.js

@@ -24,9 +24,8 @@ const {
   pingAndAssignUsers,
   getAllOpenIssues,
   getAllProjectCards,
-  hasPendingReviews
+  hasPendingReviews,
 } = require('./utils');
-const utilityModule = require('./utils');
 const mergeConflictModule = require('./checkMergeConflicts');
 const pullRequestReviewModule = require('./checkPullRequestReview');
 const { teamLeads, oppiaMaintainers } = require('../userWhitelist.json');
@@ -175,57 +174,7 @@ const ensureAllIssuesHaveProjects = async (context) => {
   });
 };
 
-/**
- * This function comments on and tags pull requests in which the last
- * commit was created 2 days ago
- * @param {import('probot').Context} context
- */
-const checkAndTagPRsWithOldBuilds = async (context) => {
-  const OLD_BUILD_LABEL = "PR: don't merge - STALE BUILD";
-
-  const allOpenPullRequests = await getAllOpenPullRequests(context);
-
-  allOpenPullRequests.forEach(async (pullRequest) => {
-    const containsOldBuildLabel = pullRequest.labels.some(
-      label => label.name === OLD_BUILD_LABEL
-    );
-    // If label has already been added, do nothing.
-    if (containsOldBuildLabel) {
-      return;
-    }
-    // Get last commit for pull request.
-    const {data: lastCommit} = await context.github.repos.getCommit(
-      context.repo({
-        ref: pullRequest.head.sha
-      })
-    );
-
-    const lastCommitDate = new Date(lastCommit.commit.author.date);
-    // Commit is older than 2 days
-    if (utilityModule.MIN_BUILD_DATE > lastCommitDate) {
-      const labelParams = context.repo({
-        issue_number: pullRequest.number,
-        labels: [OLD_BUILD_LABEL],
-      });
-      context.github.issues.addLabels(labelParams);
-
-      // Ping PR author.
-      const commentParams = context.repo({
-        issue_number: pullRequest.number,
-        body:
-            'Hi @' + pullRequest.user.login + ', the build of this PR is ' +
-            'stale and this could result in tests failing in develop. Please ' +
-            'update this pull request with the latest changes from develop. ' +
-            'Thanks!',
-      });
-      context.github.issues.createComment(commentParams);
-    }
-  }
-  );
-};
-
 module.exports = {
   ensureAllPullRequestsAreAssigned,
   ensureAllIssuesHaveProjects,
-  checkAndTagPRsWithOldBuilds
 };

lib/staleBuildChecks.js

@@ -0,0 +1,104 @@
+const utils = require('./utils');
+
+/**
+ * This function comments on and tags pull requests in which the last
+ * commit was created 2 days ago
+ * @param {import('probot').Context} context
+ */
+const checkAndTagPRsWithOldBuilds = async (context) => {
+  const allOpenPullRequests = await utils.getAllOpenPullRequests(
+    context
+  );
+
+  allOpenPullRequests.forEach(async (pullRequest) => {
+    const containsOldBuildLabel = pullRequest.labels.some(
+      label => label.name === utils.OLD_BUILD_LABEL
+    );
+    // If label has already been added, do nothing.
+    if (containsOldBuildLabel) {
+      return;
+    }
+    // Get last commit for pull request.
+    const {data: lastCommit} = await context.github.repos.getCommit(
+      context.repo({
+        ref: pullRequest.head.sha
+      })
+    );
+
+    const lastCommitDate = new Date(lastCommit.commit.author.date);
+    // Commit is older than 2 days
+    if (utils.MIN_BUILD_DATE > lastCommitDate) {
+      const labelParams = context.repo({
+        issue_number: pullRequest.number,
+        labels: [utils.OLD_BUILD_LABEL],
+      });
+      context.github.issues.addLabels(labelParams);
+
+      // Ping PR author.
+      const commentParams = context.repo({
+        issue_number: pullRequest.number,
+        body:
+            'Hi @' + pullRequest.user.login + ', the build of this PR is ' +
+            'stale and this could result in tests failing in develop. Please ' +
+            'update this pull request with the latest changes from develop. ' +
+            'Thanks!',
+      });
+      context.github.issues.createComment(commentParams);
+    }
+  }
+  );
+};
+
+/**
+ * This function removes the old build label when a PR has a new commit.
+ * @param {import('probot').Context} context
+ */
+const removeOldBuildLabel = async (context) => {
+  // Get the commit SHA after the push.
+  const sha = context.payload.after;
+  const maxWaitTime = 10000;
+  const waitTime = 2000;
+  /**
+   * @type {import('probot').Octokit.PullsGetResponse} pullRequest
+   */
+  let pullRequest;
+  let totalWaitTime = 0;
+
+  do {
+    // Wait for 2 seconds before searching so that the pull request
+    // would have been synchronized with the new commit.
+    await utils.sleep(waitTime);
+    totalWaitTime += waitTime;
+    if (totalWaitTime >= maxWaitTime) {
+      return;
+    }
+
+    const pullRequestData = await context.github.search.issuesAndPullRequests(
+      {
+        q: `${sha} repo:${context.payload.repository.full_name}`,
+      }
+    );
+    // Since we are searching via the sha, only one PR will be found,
+    // which is the PR that we are looking for.
+    pullRequest = pullRequestData.data.items[0];
+  } while (pullRequest === undefined);
+
+  const containsOldBuildLabel = pullRequest.labels.some(
+    label => label.name === utils.OLD_BUILD_LABEL
+  );
+
+  if (!containsOldBuildLabel) {
+    return;
+  }
+
+  context.github.issues.removeLabel(
+    context.repo({
+      issue_number: pullRequest.number,
+      name: utils.OLD_BUILD_LABEL,
+    })
+  );
+};
+module.exports = {
+  checkAndTagPRsWithOldBuilds,
+  removeOldBuildLabel
+};

lib/utils.js

@@ -31,6 +31,7 @@ const SUCCESS_STATUS = 204;
 const THREE_MINUTES = 60 * 1000 * 3;
 let MIN_BUILD_DATE = new Date();
 MIN_BUILD_DATE.setDate(MIN_BUILD_DATE.getDate() - 2); // 2 days prior.
+const OLD_BUILD_LABEL = "PR: don't merge - STALE BUILD";
 
 /**
  * Gets all the changed files in the repo
@@ -419,6 +420,7 @@ module.exports = {
   THREE_MINUTES,
   LABELS_EXCLUDED_FROM_CODEOWNER_ASSIGNMENT,
   DATASTORE_LABEL,
+  OLD_BUILD_LABEL,
   getAllChangedFiles,
   getNameString,
   getNewItemsFromFileByRegex,

spec/checkBranchPushSpec.js

@@ -20,6 +20,7 @@ const scheduler = require('../lib/scheduler');
 const pushPayload = require('../fixtures/push.json');
 const pullRequestPayload = require('../fixtures/pullRequestPayload.json');
 const checkBranchPushModule = require('../lib/checkBranchPush');
+const staleBuildModule = require('../lib/staleBuildChecks');
 
 describe('Force Push Check', () => {
   /**
@@ -39,6 +40,7 @@ describe('Force Push Check', () => {
 
   beforeEach(() => {
     spyOn(scheduler, 'createScheduler').and.callFake(() => {});
+    spyOn(staleBuildModule, 'removeOldBuildLabel').and.callFake(() => {});
     github = {
       issues: {
         createComment: jasmine.createSpy('createComment').and.returnValue({}),

spec/checkForNewCodeownerSpec.js

@@ -52,7 +52,7 @@ describe('check for new code owner', () => {
     /core/templates/google-analytics.initializer.ts @jameesjohn @vojtechjelinek
     /core/templates/base-components/ @jameesjohn @vojtechjelinek
     /core/templates/pages/Base.ts @jameesjohn @vojtechjelinek
-    /core/templates/pages/oppia_footer_directive.html 
+    /core/templates/pages/oppia_footer_directive.html
     @jameesjohn @vojtechjelinek
     /core/templates/pages/OppiaFooterDirective.ts @jameesjohn @vojtechjelinek
     /core/templates/pages/footer_js_libs.html @jameesjohn @vojtechjelinek