fix(aio): clean up non-public previews

The previous clean-up code for PR directories on the preview server assumed that
all directories were named after the PR number. With the changes introduced
in angular#17640 it is possible to have PR directories that do not follow that naming
convention (e.g. "non-public" directories).

This PR ensures that both public and non-public directories are removed when
cleaning up.

aio/aio-builds-setup/dockerbuild/scripts-js/lib/clean-up/build-cleaner.ts

@@ -2,6 +2,7 @@
 import * as fs from 'fs';
 import * as path from 'path';
 import * as shell from 'shelljs';
+import {HIDDEN_DIR_PREFIX} from '../common/constants';
 import {GithubPullRequests} from '../common/github-pull-requests';
 import {assertNotMissingOrEmpty} from '../common/utils';
 
@@ -31,8 +32,9 @@ export class BuildCleaner {
         }
 
         const buildNumbers = files.
-          map(Number).       // Convert string to number
-          filter(Boolean);   // Ignore NaN (or 0), because they are not builds
+          map(name => name.replace(HIDDEN_DIR_PREFIX, '')).   // Remove the "hidden dir" prefix
+          map(Number).                                        // Convert string to number
+          filter(Boolean);                                    // Ignore NaN (or 0), because they are not builds
 
         resolve(buildNumbers);
       });
@@ -49,9 +51,11 @@ export class BuildCleaner {
 
   protected removeDir(dir: string) {
     try {
-      // Undocumented signature (see https://github.com/shelljs/shelljs/pull/663).
-      (shell as any).chmod('-R', 'a+w', dir);
-      shell.rm('-rf', dir);
+      if (shell.test('-d', dir)) {
+        // Undocumented signature (see https://github.com/shelljs/shelljs/pull/663).
+        (shell as any).chmod('-R', 'a+w', dir);
+        shell.rm('-rf', dir);
+      }
     } catch (err) {
       console.error(`ERROR: Unable to remove '${dir}' due to:`, err);
     }
@@ -64,8 +68,14 @@ export class BuildCleaner {
     console.log(`Open pull requests: ${openPrNumbers.length}`);
     console.log(`Removing ${toRemove.length} build(s): ${toRemove.join(', ')}`);
 
+    // Try removing public dirs.
     toRemove.
       map(num => path.join(this.buildsDir, String(num))).
       forEach(dir => this.removeDir(dir));
+
+    // Try removing hidden dirs.
+    toRemove.
+      map(num => path.join(this.buildsDir, HIDDEN_DIR_PREFIX + String(num))).
+      forEach(dir => this.removeDir(dir));
   }
 }

aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/constants.ts

@@ -0,0 +1,2 @@
+// Constants
+export const HIDDEN_DIR_PREFIX = 'hidden--';

aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts

@@ -4,15 +4,13 @@ import {EventEmitter} from 'events';
 import * as fs from 'fs';
 import * as path from 'path';
 import * as shell from 'shelljs';
+import {HIDDEN_DIR_PREFIX} from '../common/constants';
 import {assertNotMissingOrEmpty} from '../common/utils';
 import {ChangedPrVisibilityEvent, CreatedBuildEvent} from './build-events';
 import {UploadError} from './upload-error';
 
 // Classes
 export class BuildCreator extends EventEmitter {
-  // Properties - Public, Static
-  public static HIDDEN_DIR_PREFIX = 'hidden--';
-
   // Constructor
   constructor(protected buildsDir: string) {
     super();
@@ -114,7 +112,7 @@ export class BuildCreator extends EventEmitter {
   }
 
   protected getCandidatePrDirs(pr: string, isPublic: boolean) {
-    const hiddenPrDir = path.join(this.buildsDir, BuildCreator.HIDDEN_DIR_PREFIX + pr);
+    const hiddenPrDir = path.join(this.buildsDir, HIDDEN_DIR_PREFIX + pr);
     const publicPrDir = path.join(this.buildsDir, pr);
 
     const oldPrDir = isPublic ? hiddenPrDir : publicPrDir;

aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/helper.ts

@@ -4,8 +4,8 @@ import * as fs from 'fs';
 import * as http from 'http';
 import * as path from 'path';
 import * as shell from 'shelljs';
+import {HIDDEN_DIR_PREFIX} from '../common/constants';
 import {getEnvVar} from '../common/utils';
-import {BuildCreator} from '../upload-server/build-creator';
 
 // Constans
 const TEST_AIO_BUILDS_DIR = getEnvVar('TEST_AIO_BUILDS_DIR');
@@ -104,7 +104,7 @@ class Helper {
   }
 
   public getPrDir(pr: string, isPublic: boolean): string {
-    const prDirName = isPublic ? pr : BuildCreator.HIDDEN_DIR_PREFIX + pr;
+    const prDirName = isPublic ? pr : HIDDEN_DIR_PREFIX + pr;
     return path.join(this.buildsDir, prDirName);
   }
 

aio/aio-builds-setup/dockerbuild/scripts-js/test/clean-up/build-cleaner.spec.ts

@@ -3,6 +3,7 @@ import * as fs from 'fs';
 import * as path from 'path';
 import * as shell from 'shelljs';
 import {BuildCleaner} from '../../lib/clean-up/build-cleaner';
+import {HIDDEN_DIR_PREFIX} from '../../lib/common/constants';
 import {GithubPullRequests} from '../../lib/common/github-pull-requests';
 
 // Tests
@@ -171,6 +172,16 @@ describe('BuildCleaner', () => {
     });
 
 
+    it('should remove `HIDDEN_DIR_PREFIX` from the filenames', done => {
+      promise.then(result => {
+        expect(result).toEqual([12, 34, 56]);
+        done();
+      });
+
+      readdirCb(null, [`${HIDDEN_DIR_PREFIX}12`, '34', `${HIDDEN_DIR_PREFIX}56`]);
+    });
+
+
     it('should ignore files with non-numeric (or zero) names', done => {
       promise.then(result => {
         expect(result).toEqual([12, 34, 56]);
@@ -231,10 +242,22 @@ describe('BuildCleaner', () => {
   describe('removeDir()', () => {
     let shellChmodSpy: jasmine.Spy;
     let shellRmSpy: jasmine.Spy;
+    let shellTestSpy: jasmine.Spy;
 
     beforeEach(() => {
       shellChmodSpy = spyOn(shell, 'chmod');
       shellRmSpy = spyOn(shell, 'rm');
+      shellTestSpy = spyOn(shell, 'test').and.returnValue(true);
+    });
+
+
+    it('should test if the directory exists (and return if is does not)', () => {
+      shellTestSpy.and.returnValue(false);
+      (cleaner as any).removeDir('/foo/bar');
+
+      expect(shellTestSpy).toHaveBeenCalledWith('-d', '/foo/bar');
+      expect(shellChmodSpy).not.toHaveBeenCalled();
+      expect(shellRmSpy).not.toHaveBeenCalled();
     });
 
 
@@ -294,23 +317,38 @@ describe('BuildCleaner', () => {
     });
 
 
+    it('should try removing hidden directories as well', () => {
+      (cleaner as any).removeUnnecessaryBuilds([1, 2, 3], []);
+
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize(`/foo/bar/${HIDDEN_DIR_PREFIX}1`));
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize(`/foo/bar/${HIDDEN_DIR_PREFIX}2`));
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize(`/foo/bar/${HIDDEN_DIR_PREFIX}3`));
+    });
+
+
     it('should remove the builds that do not correspond to open PRs', () => {
       (cleaner as any).removeUnnecessaryBuilds([1, 2, 3, 4], [2, 4]);
-      expect(cleanerRemoveDirSpy).toHaveBeenCalledTimes(2);
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledTimes(4);
       expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize('/foo/bar/1'));
       expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize('/foo/bar/3'));
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize(`/foo/bar/${HIDDEN_DIR_PREFIX}1`));
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize(`/foo/bar/${HIDDEN_DIR_PREFIX}3`));
       cleanerRemoveDirSpy.calls.reset();
 
       (cleaner as any).removeUnnecessaryBuilds([1, 2, 3, 4], [1, 2, 3, 4]);
       expect(cleanerRemoveDirSpy).toHaveBeenCalledTimes(0);
       cleanerRemoveDirSpy.calls.reset();
 
       (cleaner as any).removeUnnecessaryBuilds([1, 2, 3, 4], []);
-      expect(cleanerRemoveDirSpy).toHaveBeenCalledTimes(4);
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledTimes(8);
       expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize('/foo/bar/1'));
       expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize('/foo/bar/2'));
       expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize('/foo/bar/3'));
       expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize('/foo/bar/4'));
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize(`/foo/bar/${HIDDEN_DIR_PREFIX}1`));
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize(`/foo/bar/${HIDDEN_DIR_PREFIX}2`));
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize(`/foo/bar/${HIDDEN_DIR_PREFIX}3`));
+      expect(cleanerRemoveDirSpy).toHaveBeenCalledWith(path.normalize(`/foo/bar/${HIDDEN_DIR_PREFIX}4`));
       cleanerRemoveDirSpy.calls.reset();
     });