diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/clean-up/build-cleaner.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/clean-up/build-cleaner.ts index 64e2a33f7e..3a23b19096 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/clean-up/build-cleaner.ts +++ b/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)); } } diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/constants.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/constants.ts new file mode 100644 index 0000000000..b586f634ac --- /dev/null +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/constants.ts @@ -0,0 +1,2 @@ +// Constants +export const HIDDEN_DIR_PREFIX = 'hidden--'; diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts index 1593597891..c0f1ce29cb 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts +++ b/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; diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/helper.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/helper.ts index 040d7faba7..62af299416 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/helper.ts +++ b/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); } diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/test/clean-up/build-cleaner.spec.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/test/clean-up/build-cleaner.spec.ts index f7b336f8cd..d6e7f262d5 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/test/clean-up/build-cleaner.spec.ts +++ b/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,11 +317,22 @@ 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]); @@ -306,11 +340,15 @@ describe('BuildCleaner', () => { 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(); });