Merge pull request finos#1393 from finos/1386-remote-folder-cleanup

fix: improve handling of repository clones in the .remote folder

Author: jescalada

src/proxy/actions/Action.ts

@@ -94,7 +94,8 @@ class Action {
   }
 
   /**
-   * Set the commit range for the action.
+   * Set the commit range for the action. Changes the action.id to be based on
+   * the commit details.
    * @param {string} commitFrom the starting commit
    * @param {string} commitTo the ending commit
    */

src/proxy/chain.ts

@@ -10,15 +10,13 @@ const pushActionChain: ((req: any, action: Action) => Promise<Action>)[] = [
   proc.push.checkCommitMessages,
   proc.push.checkAuthorEmails,
   proc.push.checkUserPushPermission,
-  proc.push.pullRemote,
+  proc.push.pullRemote, // cleanup is handled after chain execution if successful
   proc.push.writePack,
   proc.push.checkHiddenCommits,
   proc.push.checkIfWaitingAuth,
   proc.push.preReceive,
   proc.push.getDiff,
-  // run before clear remote
   proc.push.gitleaks,
-  proc.push.clearBareClone,
   proc.push.scanDiff,
   proc.push.blockForAuth,
 ];
@@ -35,6 +33,7 @@ let pluginsInserted = false;
 
 export const executeChain = async (req: any, res: any): Promise<Action> => {
   let action: Action = {} as Action;
+  let checkoutCleanUpRequired = false;
 
   try {
     action = await proc.pre.parseAction(req);
@@ -44,18 +43,28 @@ export const executeChain = async (req: any, res: any): Promise<Action> => {
       action = await fn(req, action);
       if (!action.continue() || action.allowPush) {
         break;
+      } else if (fn === proc.push.pullRemote) {
+        //if the pull was successful then record the fact we need to clean it up again
+        // pullRemote should cleanup unsuccessful clones itself
+        checkoutCleanUpRequired = true;
       }
     }
   } catch (e) {
     action.error = true;
     action.errorMessage = `An error occurred when executing the chain: ${e}`;
     console.error(action.errorMessage);
   } finally {
-    await proc.push.audit(req, action);
+    //clean up the clone created
+    if (checkoutCleanUpRequired) {
+      action = await proc.post.clearBareClone(req, action);
+    }
+
+    action = await proc.post.audit(req, action);
+
     if (action.autoApproved) {
-      attemptAutoApproval(action);
+      await attemptAutoApproval(action);
     } else if (action.autoRejected) {
-      attemptAutoRejection(action);
+      await attemptAutoRejection(action);
     }
   }
 

src/proxy/index.ts

@@ -43,6 +43,16 @@ export class Proxy {
   constructor() {}
 
   private async proxyPreparations() {
+    // Clean-up the .remote folder in case anything was in-progress when we shut down
+    const remoteDir = './.remote';
+    if (fs.existsSync(remoteDir)) {
+      console.log('Cleaning up the existing .remote dir...');
+      // Recursively remove the contents of ./.remote and ignore exceptions
+      fs.rmSync(remoteDir, { recursive: true, force: true, retryDelay: 100 });
+    }
+    console.log('Creating the .remote dir...');
+    fs.mkdirSync(remoteDir);
+
     const plugins = getPlugins();
     const pluginLoader = new PluginLoader(plugins);
     await pluginLoader.load();

src/proxy/processors/index.ts

@@ -1,4 +1,5 @@
 import * as pre from './pre-processor';
 import * as push from './push-action';
+import * as post from './post-processor';
 
-export { pre, push };
+export { pre, push, post };

src/proxy/processors/push-action/audit.ts …proxy/processors/post-processor/audit.tssrc/proxy/processors/push-action/audit.ts renamed to src/proxy/processors/post-processor/audit.ts src/proxy/processors/push-action/audit.ts renamed to src/proxy/processors/post-processor/audit.ts

@@ -0,0 +1,23 @@
+import { Action, Step } from '../../actions';
+import fs from 'fs';
+
+const exec = async (req: any, action: Action): Promise<Action> => {
+  const step = new Step('clearBareClone');
+
+  // Recursively remove the contents of ./.remote and ignore exceptions
+  if (action.proxyGitPath) {
+    fs.rmSync(action.proxyGitPath, { recursive: true, force: true });
+    step.log(`.remote is deleted!`);
+  } else {
+    // This action should not be called unless a clone was made successfully as pullRemote cleans up after itself on failures
+    // Log an error as we couldn't delete the clone
+    step.setError(`action.proxyGitPath was not set and cannot be removed`);
+  }
+  action.addStep(step);
+
+  return action;
+};
+
+exec.displayName = 'clearBareClone.exec';
+
+export { exec };

src/proxy/processors/post-processor/clearBareClone.ts

@@ -0,0 +1,4 @@
+import { exec as audit } from '../post-processor/audit';
+import { exec as clearBareClone } from '../post-processor/clearBareClone';
+
+export { audit, clearBareClone };

src/proxy/processors/post-processor/index.ts

@@ -1,7 +1,7 @@
 import { exec as parsePush } from './parsePush';
 import { exec as preReceive } from './preReceive';
 import { exec as checkRepoInAuthorisedList } from './checkRepoInAuthorisedList';
-import { exec as audit } from './audit';
+
 import { exec as pullRemote } from './pullRemote';
 import { exec as writePack } from './writePack';
 import { exec as getDiff } from './getDiff';
@@ -13,14 +13,13 @@ import { exec as checkIfWaitingAuth } from './checkIfWaitingAuth';
 import { exec as checkCommitMessages } from './checkCommitMessages';
 import { exec as checkAuthorEmails } from './checkAuthorEmails';
 import { exec as checkUserPushPermission } from './checkUserPushPermission';
-import { exec as clearBareClone } from './clearBareClone';
+
 import { exec as checkEmptyBranch } from './checkEmptyBranch';
 
 export {
   parsePush,
   preReceive,
   checkRepoInAuthorisedList,
-  audit,
   pullRemote,
   writePack,
   getDiff,
@@ -32,6 +31,5 @@ export {
   checkCommitMessages,
   checkAuthorEmails,
   checkUserPushPermission,
-  clearBareClone,
   checkEmptyBranch,
 };

src/proxy/processors/push-action/clearBareClone.ts

@@ -63,6 +63,8 @@ async function exec(req: any, action: Action): Promise<Action> {
     // Strip everything after NUL, which is cap-list from
     // https://git-scm.com/docs/http-protocol#_smart_server_response
     action.branch = ref.replace(/\0.*/, '').trim();
+
+    // Note this will change the action.id to be based on the commits
     action.setCommit(oldCommit, newCommit);
 
     // Check if the offset is valid and if there's data after it