New state for when a PR is in the process of merging (#110)

itsjackgardner · web-flow · commit be92297b2120 · 2021-04-30T10:51:16.000+10:00
* New merging state

* Fix tests to include merge callback and new merge state

* Minor updates
diff --git a/src/bitbucket/BitbucketAPI.ts b/src/bitbucket/BitbucketAPI.ts
@@ -162,7 +162,7 @@ export class BitbucketAPI {
     // 5 attempts total
     await attemptMerge(1, 4);
 
-    // We throw before here if the merge is unsuccessul
+    // We throw before here if the merge is unsuccessful
     Logger.info('Merged Pull Request', {
       namespace: 'bitbucket:api:mergePullRequest',
       landRequestId,
diff --git a/src/bitbucket/descriptor.ts b/src/bitbucket/descriptor.ts
@@ -45,7 +45,9 @@ export const makeDescriptor = () => ({
         key: 'atlaskid-addon-panel',
         name: {
           value: `Landkid Queue${
-            process.env.LANDKID_DEPLOYMENT !== 'prod' ? ` (${process.env.LANDKID_DEPLOYMENT})` : ''
+            process.env.LANDKID_DEPLOYMENT !== 'prod'
+              ? ` (${process.env.LANDKID_DEPLOYMENT || 'local'})`
+              : ''
           }`,
         },
         url:
diff --git a/src/db/index.ts b/src/db/index.ts
@@ -172,6 +172,7 @@ export class LandRequestStatus extends Model<LandRequestStatus> implements IStat
         'queued',
         'running',
         'awaiting-merge',
+        'merging',
         'success',
         'fail',
         'aborted',
diff --git a/src/lib/AccountService.ts b/src/lib/AccountService.ts
@@ -43,6 +43,7 @@ export class AccountService {
 
         return JSON.parse(cached);
       },
+      undefined,
     );
     return info || this.getAccountInfo(aaid, retry - 1);
   };
diff --git a/src/lib/Runner.ts b/src/lib/Runner.ts
@@ -215,102 +215,104 @@ export class Runner {
       return false; // did not move state, return false
     }
 
-    // Try to merge PR
-    try {
-      // skip CI if there is a dependent PR that is awaiting merge
-      const skipCI =
-        dependentsAwaitingMerge.length > 0 &&
-        this.config.mergeSettings &&
-        this.config.mergeSettings.skipBuildOnDependentsAwaitingMerge;
-      const pullRequestId = landRequest.pullRequestId;
-      Logger.verbose('Attempting merge pull request', {
-        namespace: 'lib:runner:moveFromAwaitingMerge',
-        pullRequestId,
-        landRequestId: landRequest.id,
-        lockId,
-      });
-      await this.client.mergePullRequest(landRequestStatus, { skipCI });
-      Logger.info('Successfully merged PR', {
-        namespace: 'lib:runner:moveFromAwaitingMerge',
-        landRequestId: landRequest.id,
-        pullRequestId,
-        lockId,
-      });
+    Logger.info('Triggering merge attempt', {
+      namespace: 'lib:runner:moveFromAwaitingMerge',
+      pullRequestId: landRequest.pullRequestId,
+      landRequestId: landRequest.id,
+      lockId,
+    });
 
-      const end = Date.now();
-      const queuedDate = await this.getLandRequestQueuedDate(landRequest.id);
-      const start = queuedDate!.getTime();
-      eventEmitter.emit('PULL_REQUEST.MERGE.SUCCESS', {
-        landRequestId: landRequestStatus.requestId,
-        pullRequestId: landRequest.pullRequestId,
-        commit: landRequest.forCommit,
-        sourceBranch: pullRequest.sourceBranch,
-        targetBranch: pullRequest.targetBranch,
-        duration: end - start,
-      });
-      return landRequest.setStatus('success');
-    } catch (err) {
-      eventEmitter.emit('PULL_REQUEST.MERGE.FAIL', {
-        landRequestId: landRequestStatus.requestId,
-        pullRequestId: landRequest.pullRequestId,
-        commit: landRequest.forCommit,
-        sourceBranch: pullRequest.sourceBranch,
-        targetBranch: pullRequest.targetBranch,
+    // Start merge attempt
+    await landRequest.setStatus('merging');
+
+    // skip CI if there is a dependent PR that is awaiting merge
+    const skipCI =
+      dependentsAwaitingMerge.length > 0 &&
+      this.config.mergeSettings &&
+      this.config.mergeSettings.skipBuildOnDependentsAwaitingMerge;
+
+    this.client
+      .mergePullRequest(landRequestStatus, { skipCI })
+      .then(async () => {
+        const end = Date.now();
+        const queuedDate = await this.getLandRequestQueuedDate(landRequest.id);
+        const start = queuedDate!.getTime();
+        eventEmitter.emit('PULL_REQUEST.MERGE.SUCCESS', {
+          landRequestId: landRequestStatus.requestId,
+          pullRequestId: landRequest.pullRequestId,
+          commit: landRequest.forCommit,
+          sourceBranch: pullRequest.sourceBranch,
+          targetBranch: pullRequest.targetBranch,
+          duration: end - start,
+        });
+        await landRequest.setStatus('success');
+      })
+      .catch(async () => {
+        eventEmitter.emit('PULL_REQUEST.MERGE.FAIL', {
+          landRequestId: landRequestStatus.requestId,
+          pullRequestId: landRequest.pullRequestId,
+          commit: landRequest.forCommit,
+          sourceBranch: pullRequest.sourceBranch,
+          targetBranch: pullRequest.targetBranch,
+        });
+        await landRequest.setStatus('fail', 'Unable to merge pull request');
       });
-      return landRequest.setStatus('fail', 'Unable to merge pull request');
-    }
   };
 
   // Next must always return early if ever doing a single state transition
   next = async () => {
-    const runNextAgain = await withLock('status-transition', async (lockId: Date) => {
-      const queue = await this.queue.getQueue();
-      Logger.info('Next() called', {
-        namespace: 'lib:runner:next',
-        lockId,
-        queue,
-      });
+    const runNextAgain = await withLock(
+      'status-transition',
+      async (lockId: Date) => {
+        const queue = await this.queue.getQueue();
+        Logger.info('Next() called', {
+          namespace: 'lib:runner:next',
+          lockId,
+          queue,
+        });
 
-      for (const landRequestStatus of queue) {
-        // Check for this _before_ looking at the state so that we don't have to wait until
-        const landRequest = landRequestStatus.request;
-        const failedDeps = await landRequest.getFailedDependencies();
-        if (failedDeps.length !== 0) {
-          Logger.info('LandRequest failed due to failing dependency', {
-            namespace: 'lib:runner:next',
-            lockId,
-            landRequestId: landRequest.id,
-            pullRequestId: landRequest.pullRequestId,
-            landRequestStatus,
-            failedDeps,
-          });
-          const failedPrIds = failedDeps.map(d => d.request.pullRequestId).join(', ');
-          const failReason = `Failed due to failed dependency builds: ${failedPrIds}`;
-          await landRequest.setStatus('fail', failReason);
-          await landRequest.update({ dependsOn: null });
-          await this.client.stopLandBuild(landRequest.buildId, lockId);
-          const user = await this.client.getUser(landRequest.triggererAaid);
-          return landRequest.setStatus('queued', `Queued by ${user.displayName || user.aaid}`);
-        }
-        if (landRequestStatus.state === 'awaiting-merge') {
-          const awaitingMergeQueue = Runner.getDependentsAwaitingMerge(queue, landRequestStatus);
-          const didChangeState = await this.moveFromAwaitingMerge(
-            landRequestStatus,
-            lockId,
-            awaitingMergeQueue,
-          );
-          // if we moved, we need to exit early, otherwise, just keep checking the queue
-          if (didChangeState) return true;
-        } else if (landRequestStatus.state === 'queued') {
-          const didChangeState = await this.moveFromQueueToRunning(landRequestStatus, lockId);
-          // if the landrequest was able to move from queued to running, exit early, otherwise, keep
-          // checking the rest of the queue
-          if (didChangeState) return true;
+        for (const landRequestStatus of queue) {
+          // Check for this _before_ looking at the state so that we don't have to wait until
+          const landRequest = landRequestStatus.request;
+          const failedDeps = await landRequest.getFailedDependencies();
+          if (failedDeps.length !== 0) {
+            Logger.info('LandRequest failed due to failing dependency', {
+              namespace: 'lib:runner:next',
+              lockId,
+              landRequestId: landRequest.id,
+              pullRequestId: landRequest.pullRequestId,
+              landRequestStatus,
+              failedDeps,
+            });
+            const failedPrIds = failedDeps.map(d => d.request.pullRequestId).join(', ');
+            const failReason = `Failed due to failed dependency builds: ${failedPrIds}`;
+            await landRequest.setStatus('fail', failReason);
+            await landRequest.update({ dependsOn: null });
+            await this.client.stopLandBuild(landRequest.buildId, lockId);
+            const user = await this.client.getUser(landRequest.triggererAaid);
+            return landRequest.setStatus('queued', `Queued by ${user.displayName || user.aaid}`);
+          }
+          if (landRequestStatus.state === 'awaiting-merge') {
+            const awaitingMergeQueue = Runner.getDependentsAwaitingMerge(queue, landRequestStatus);
+            const didChangeState = await this.moveFromAwaitingMerge(
+              landRequestStatus,
+              lockId,
+              awaitingMergeQueue,
+            );
+            // if we moved, we need to exit early, otherwise, just keep checking the queue
+            if (didChangeState) return true;
+          } else if (landRequestStatus.state === 'queued') {
+            const didChangeState = await this.moveFromQueueToRunning(landRequestStatus, lockId);
+            // if the landrequest was able to move from queued to running, exit early, otherwise, keep
+            // checking the rest of the queue
+            if (didChangeState) return true;
+          }
+          // otherwise, we must just be running, nothing to do here
         }
-        // otherwise, we must just be running, nothing to do here
-      }
-      return false;
-    });
+        return false;
+      },
+      false,
+    );
     if (runNextAgain) {
       await this.next();
     }
@@ -522,42 +524,46 @@ export class Runner {
   };
 
   checkWaitingLandRequests = async () => {
-    await withLock('status-transition', async () => {
-      const waitingRequestStatuses = await this.queue.getStatusesForWaitingRequests();
+    await withLock(
+      'status-transition',
+      async () => {
+        const waitingRequestStatuses = await this.queue.getStatusesForWaitingRequests();
+
+        Logger.info('Checking for waiting landrequests ready to queue', {
+          namespace: 'lib:runner:checkWaitingLandRequests',
+          waitingRequestStatuses,
+        });
 
-      Logger.info('Checking for waiting landrequests ready to queue', {
-        namespace: 'lib:runner:checkWaitingLandRequests',
-        waitingRequestStatuses,
-      });
+        for (const landRequestStatus of waitingRequestStatuses) {
+          const landRequest = landRequestStatus.request;
+          const pullRequestId = landRequest.pullRequestId;
+          const triggererUserMode = await permissionService.getPermissionForUser(
+            landRequest.triggererAaid,
+          );
+          const isAllowedToLand = await this.isAllowedToLand(
+            pullRequestId,
+            triggererUserMode,
+            this.getQueue,
+          );
 
-      for (const landRequestStatus of waitingRequestStatuses) {
-        const landRequest = landRequestStatus.request;
-        const pullRequestId = landRequest.pullRequestId;
-        const triggererUserMode = await permissionService.getPermissionForUser(
-          landRequest.triggererAaid,
-        );
-        const isAllowedToLand = await this.isAllowedToLand(
-          pullRequestId,
-          triggererUserMode,
-          this.getQueue,
-        );
-
-        if (isAllowedToLand.errors.length === 0) {
-          if (isAllowedToLand.existingRequest) {
-            Logger.warn('Already has existing Land build', {
-              pullRequestId,
-              landRequestId: landRequest.id,
-              landRequestStatus,
-              namespace: 'lib:runner:checkWaitingLandRequests',
-            });
-            await landRequest.setStatus('aborted', 'Already has existing Land build');
-            continue;
+          if (isAllowedToLand.errors.length === 0) {
+            if (isAllowedToLand.existingRequest) {
+              Logger.warn('Already has existing Land build', {
+                pullRequestId,
+                landRequestId: landRequest.id,
+                landRequestStatus,
+                namespace: 'lib:runner:checkWaitingLandRequests',
+              });
+              await landRequest.setStatus('aborted', 'Already has existing Land build');
+              continue;
+            }
+            const movedState = await this.moveFromWaitingToQueued(landRequestStatus);
+            if (movedState) return this.next();
           }
-          const movedState = await this.moveFromWaitingToQueued(landRequestStatus);
-          if (movedState) return this.next();
         }
-      }
-    });
+      },
+      undefined,
+    );
   };
 
   getStatusesForLandRequests = async (
diff --git a/src/lib/utils/locker.ts b/src/lib/utils/locker.ts
@@ -4,7 +4,11 @@ import { Logger } from '../Logger';
 
 const redlock = new RedLock([client]);
 
-export const withLock = async <T>(resource: string, fn: (lockId: Date) => Promise<T>) => {
+export const withLock = async <T>(
+  resource: string,
+  fn: (lockId: Date) => Promise<T>,
+  fallback: T,
+) => {
   let lock: RedLock.Lock;
   let lockId: Date;
   try {
@@ -15,7 +19,7 @@ export const withLock = async <T>(resource: string, fn: (lockId: Date) => Promis
       lockId,
     });
   } catch {
-    return;
+    return fallback;
   }
   let result: T;
   try {
diff --git a/src/static/current-state/components/Header.tsx b/src/static/current-state/components/Header.tsx
@@ -18,16 +18,9 @@ const userInfoStyles = css({
 });
 
 const userNameStyles = css({
-  marginRight: 8,
   fontWeight: 'bold',
 });
 
-const userImgStyles = css({
-  height: 32,
-  width: 32,
-  borderRadius: '100%',
-});
-
 interface HeaderProps {
   user?: ISessionUser;
 }
@@ -38,10 +31,6 @@ export const Header: React.FunctionComponent<HeaderProps> = ({ user }) => (
     {user ? (
       <div className={userInfoStyles}>
         <span className={userNameStyles}>{user.displayName}</span>
-        <img
-          src={`https://bitbucket.org/account/${user.username}/avatar/`}
-          className={userImgStyles}
-        />
       </div>
     ) : null}
   </div>
diff --git a/src/static/current-state/components/QueueItem.tsx b/src/static/current-state/components/QueueItem.tsx
@@ -154,29 +154,32 @@ export const StatusItem: React.FunctionComponent<StatusItemProps> = props => (
 
 const landStatusToAppearance: Record<IStatusUpdate['state'], LozengeAppearance> = {
   'will-queue-when-ready': 'new',
-  'awaiting-merge': 'new',
   queued: 'new',
   running: 'inprogress',
+  'awaiting-merge': 'new',
+  merging: 'new',
   success: 'success',
   fail: 'removed',
   aborted: 'moved',
 };
 
 const landStatusToNiceString: Record<IStatusUpdate['state'], string> = {
   'will-queue-when-ready': 'Waiting to Land',
-  'awaiting-merge': 'Awaiting Merge',
   queued: 'In Queue',
   running: 'Running',
+  'awaiting-merge': 'Awaiting Merge',
+  merging: 'Merging',
   success: 'Succeeded',
   aborted: 'Aborted',
   fail: 'Failed',
 };
 
 const landStatusToPastTense: Record<IStatusUpdate['state'], string> = {
   'will-queue-when-ready': 'Told To Land When Ready',
-  'awaiting-merge': 'Told to Merge',
   queued: 'Told To Land',
   running: 'Started',
+  'awaiting-merge': 'Ready to Merge',
+  merging: 'Merge Started',
   success: 'Succeeded',
   fail: 'Failed',
   aborted: 'Aborted',
diff --git a/tests/integration/utils/index.ts b/tests/integration/utils/index.ts
diff --git a/tests/unit/BitbucketAPI.test.ts b/tests/unit/BitbucketAPI.test.ts
diff --git a/typings/ambient.d.ts b/typings/ambient.d.ts
