new IsAllowedToLand function that includes an existing request check (#89)

* new IsAllowedToLand function that includes an existing request check

* Move self-dep failsafe

Author: itsjackgardner

src/bitbucket/BitbucketClient.ts

@@ -17,7 +17,7 @@ export class BitbucketClient {
 
   constructor(private config: Config) {}
 
-  async isAllowedToLand(pullRequestId: number, permissionLevel: IPermissionMode) {
+  async isAllowedToMerge(pullRequestId: number, permissionLevel: IPermissionMode) {
     const pullRequest: BB.PullRequest = await this.bitbucket.getPullRequest(pullRequestId);
     const buildStatuses = await this.bitbucket.getPullRequestBuildStatuses(pullRequestId);
     const author = pullRequest.author;

src/lib/Runner.ts

@@ -49,14 +49,17 @@ export class Runner {
     return this.queue.getRunning();
   };
 
+  getWaitingAndQueued = async () => {
+    const queued = await this.getQueue();
+    const waiting = await this.queue.getStatusesForWaitingRequests();
+    return [...queued, ...waiting];
+  };
+
   moveFromQueueToRunning = async (landRequestStatus: LandRequestStatus, lockId: Date) => {
     const landRequest = landRequestStatus.request;
     const running = await this.getRunning();
     const runningTargetingSameBranch = running.filter(
-      build =>
-        build.request.pullRequest.targetBranch === landRequest.pullRequest.targetBranch &&
-        // Failsafe to prevent self-dependencies
-        build.request.pullRequestId !== landRequest.pullRequestId,
+      build => build.request.pullRequest.targetBranch === landRequest.pullRequest.targetBranch,
     );
     const maxConcurrentBuilds = this.getMaxConcurrentBuilds();
     if (runningTargetingSameBranch.length >= maxConcurrentBuilds) {
@@ -76,9 +79,10 @@ export class Runner {
       landRequest.triggererAaid,
     );
     const commit = landRequest.forCommit;
-    const isAllowedToLand = await this.client.isAllowedToLand(
+    const isAllowedToLand = await this.isAllowedToLand(
       landRequest.pullRequestId,
       triggererUserMode,
+      this.getRunning,
     );
 
     // ToDo: Extra checks, should probably not be here, but this will do for now
@@ -108,10 +112,10 @@ export class Runner {
       return landRequest.setStatus('aborted', 'PR commit changed between landing and running');
     }
 
-    if (isAllowedToLand.errors.length > 0) {
+    if (isAllowedToLand.errors.length > 0 || isAllowedToLand.existingRequest) {
       Logger.error('LandRequest no longer passes land checks', {
         namespace: 'lib:runner:moveFromQueueToRunning',
-        errors: isAllowedToLand.errors,
+        isAllowedToLand,
         landRequestStatus,
         pullRequestId: landRequest.pullRequestId,
         landRequestId: landRequest.id,
@@ -120,8 +124,12 @@ export class Runner {
       return landRequest.setStatus('fail', 'Unable to land due to failed land checks');
     }
 
+    const runningExceptSelf = runningTargetingSameBranch.filter(
+      // Failsafe to prevent self-dependencies
+      build => build.request.pullRequestId !== landRequest.pullRequestId,
+    );
     const dependencies = [];
-    for (const queueItem of runningTargetingSameBranch) {
+    for (const queueItem of runningExceptSelf) {
       if ((await queueItem.request.getFailedDependencies()).length === 0) {
         dependencies.push(queueItem);
       }
@@ -492,19 +500,18 @@ export class Runner {
         const triggererUserMode = await permissionService.getPermissionForUser(
           landRequest.triggererAaid,
         );
-        const isAllowedToLand = await this.client.isAllowedToLand(pullRequestId, triggererUserMode);
+        const isAllowedToLand = await this.isAllowedToLand(
+          pullRequestId,
+          triggererUserMode,
+          this.getQueue,
+        );
 
         if (isAllowedToLand.errors.length === 0) {
-          const queue = await this.getQueue();
-          const existingBuild = queue.find(
-            q => q.request.pullRequestId === landRequest.pullRequestId,
-          );
-          if (existingBuild) {
+          if (isAllowedToLand.existingRequest) {
             Logger.warn('Already has existing Land build', {
               pullRequestId,
               landRequestId: landRequest.id,
               landRequestStatus,
-              existingBuild,
               namespace: 'lib:runner:checkWaitingLandRequests',
             });
             await landRequest.setStatus('aborted', 'Already has existing Land build');
@@ -633,6 +640,26 @@ export class Runner {
     }
   };
 
+  async isAllowedToLand(
+    pullRequestId: number,
+    permissionLevel: IPermissionMode,
+    queueFetcher: () => Promise<LandRequestStatus[]>,
+  ) {
+    const isAllowedToMerge = await this.client.isAllowedToMerge(pullRequestId, permissionLevel);
+    let existingRequest = false;
+    const queue = await queueFetcher();
+    for (const queueItem of queue) {
+      if (queueItem.request.pullRequestId === pullRequestId) {
+        existingRequest = true;
+        break;
+      }
+    }
+    return {
+      existingRequest,
+      ...isAllowedToMerge,
+    };
+  }
+
   getState = async (requestingUser: ISessionUser): Promise<RunnerState> => {
     const requestingUserMode = await permissionService.getPermissionForUser(requestingUser.aaid);
     const [

src/routes/bitbucket/proxy/index.ts

@@ -30,7 +30,7 @@ export function proxyRoutes(runner: Runner, client: BitbucketClient) {
 
       const errors: string[] = [];
       const warnings: string[] = [];
-      const landCheckErrors: string[] = [];
+      let existingRequest = false;
       const bannerMessage = await runner.getBannerMessageState();
 
       const permissionLevel = await permissionService.getPermissionForUser(aaid);
@@ -39,19 +39,16 @@ export function proxyRoutes(runner: Runner, client: BitbucketClient) {
         if (pauseState) {
           errors.push(`Builds have been manually paused: "${pauseState.reason}"`);
         } else {
-          const landChecks = await client.isAllowedToLand(prId, permissionLevel);
+          const landChecks = await runner.isAllowedToLand(
+            prId,
+            permissionLevel,
+            runner.getWaitingAndQueued,
+          );
           warnings.push(...landChecks.warnings);
           errors.push(...landChecks.errors);
-          landCheckErrors.push(...landChecks.errors);
-
-          const queued = await runner.queue.getQueue();
-          const waiting = await runner.queue.getStatusesForWaitingRequests();
-
-          for (const queueItem of [...queued, ...waiting]) {
-            if (queueItem.request.pullRequest.prId === prId) {
-              errors.push('This PR has already been queued, patience young padawan');
-              break;
-            }
+          if (landChecks.existingRequest) {
+            existingRequest = true;
+            errors.push('This PR has already been queued, patience young padawan');
           }
         }
       } else {
@@ -66,7 +63,7 @@ export function proxyRoutes(runner: Runner, client: BitbucketClient) {
 
       res.json({
         canLand: errors.length === 0,
-        canLandWhenAble: errors.length === landCheckErrors.length && prSettings.allowLandWhenAble,
+        canLandWhenAble: !existingRequest && prSettings.allowLandWhenAble,
         errors,
         warnings,
         bannerMessage,