From 021f4344b117f20530fa1de2a80a6384ce3e15d8 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Sat, 15 Sep 2018 15:31:39 +0300 Subject: [PATCH] fix(docs-infra): fix preview server periodic clean-up (#25671) Includes the following fixes: - Fix cron entry format for clean-up script. Crontabs in `/etc` should not have a user field. No idea why it used to work before, but it started giving errors recently: `/bin/sh: root: not found`. - Set required env variable in clean-up script. (Broken in cc6f36a9d.) This was producing the following error: `ERROR: Missing required environment variable 'AIO_CIRCLE_CI_TOKEN'!` - Use the correct path for downloads to be removed. (Broken in cc6f36a9d.) PR Close #25671 --- .../dockerbuild/cronjobs/aio-builds-cleanup | 2 +- .../scripts-js/lib/clean-up/build-cleaner.ts | 2 +- .../test/clean-up/build-cleaner.spec.ts | 57 ++++++++++++------- .../dockerbuild/scripts-sh/clean-up.sh | 1 + 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/aio/aio-builds-setup/dockerbuild/cronjobs/aio-builds-cleanup b/aio/aio-builds-setup/dockerbuild/cronjobs/aio-builds-cleanup index 3e6e5117ea..f820811ab2 100644 --- a/aio/aio-builds-setup/dockerbuild/cronjobs/aio-builds-cleanup +++ b/aio/aio-builds-setup/dockerbuild/cronjobs/aio-builds-cleanup @@ -1,2 +1,2 @@ # Periodically clean up builds that do not correspond to currently open PRs -0 12 * * * root /usr/local/bin/aio-clean-up >> /var/log/cron.log 2>&1 +0 12 * * * /usr/local/bin/aio-clean-up >> /var/log/cron.log 2>&1 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 fb770c9725..d52cc6ebbc 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 @@ -122,6 +122,6 @@ export class BuildCleaner { this.logger.log(`Existing downloads: ${existingDownloads.length}`); this.logger.log(`Removing ${toRemove.length} download(s): ${toRemove.join(', ')}`); - toRemove.forEach(filePath => shell.rm(filePath)); + toRemove.forEach(filePath => shell.rm(path.join(this.downloadsDir, filePath))); } } 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 d2bb81946c..398d52f5ff 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 @@ -9,10 +9,10 @@ import {Logger} from '../../lib/common/utils'; const EXISTING_BUILDS = [10, 20, 30, 40]; const EXISTING_DOWNLOADS = [ - 'downloads/10-ABCDEF0-build.zip', - 'downloads/10-1234567-build.zip', - 'downloads/20-ABCDEF0-build.zip', - 'downloads/20-1234567-build.zip', + '10-ABCDEF0-build.zip', + '10-1234567-build.zip', + '20-ABCDEF0-build.zip', + '20-1234567-build.zip', ]; const OPEN_PRS = [10, 40]; const ANY_DATE = jasmine.any(String); @@ -26,7 +26,7 @@ describe('BuildCleaner', () => { beforeEach(() => { loggerErrorSpy = spyOn(Logger.prototype, 'error'); loggerLogSpy = spyOn(Logger.prototype, 'log'); - cleaner = new BuildCleaner('/foo/bar', 'baz', 'qux', '12345', 'downloads', 'build.zip'); + cleaner = new BuildCleaner('/foo/bar', 'baz', 'qux', '12345', '/downloads', 'build.zip'); }); describe('constructor()', () => { @@ -54,11 +54,13 @@ describe('BuildCleaner', () => { toThrowError('Missing or empty required parameter \'githubToken\'!'); }); + it('should throw if \'downloadsDir\' is empty', () => { expect(() => new BuildCleaner('/foo/bar', 'baz', 'qux', '12345', '', 'build.zip')). toThrowError('Missing or empty required parameter \'downloadsDir\'!'); }); + it('should throw if \'artifactPath\' is empty', () => { expect(() => new BuildCleaner('/foo/bar', 'baz', 'qux', '12345', 'downloads', '')). toThrowError('Missing or empty required parameter \'artifactPath\'!'); @@ -166,6 +168,7 @@ describe('BuildCleaner', () => { } }); + it('should reject if \'removeUnnecessaryDownloads()\' rejects', async () => { try { cleanerRemoveUnnecessaryDownloadsSpy.and.callFake(() => Promise.reject('Test')); @@ -174,6 +177,7 @@ describe('BuildCleaner', () => { expect(err).toBe('Test'); } }); + }); @@ -283,12 +287,14 @@ describe('BuildCleaner', () => { prDeferred.resolve([{id: 0, number: 1}, {id: 1, number: 2}, {id: 2, number: 3}]); }); + it('should log the number of open PRs', () => { promise.then(prNumbers => { expect(loggerLogSpy).toHaveBeenCalledWith( ANY_DATE, 'BuildCleaner: ', `Open pull requests: ${prNumbers}`); }); }); + }); @@ -308,9 +314,9 @@ describe('BuildCleaner', () => { }); - it('should get the contents of the builds directory', () => { + it('should get the contents of the downloads directory', () => { expect(fsReaddirSpy).toHaveBeenCalled(); - expect(fsReaddirSpy.calls.argsFor(0)[0]).toBe('downloads'); + expect(fsReaddirSpy.calls.argsFor(0)[0]).toBe('/downloads'); }); @@ -324,7 +330,7 @@ describe('BuildCleaner', () => { }); - it('should resolve with the returned files (as numbers)', done => { + it('should resolve with the returned file names', done => { promise.then(result => { expect(result).toEqual(EXISTING_DOWNLOADS); done(); @@ -460,25 +466,36 @@ describe('BuildCleaner', () => { describe('removeUnnecessaryDownloads()', () => { + let shellRmSpy: jasmine.Spy; + beforeEach(() => { - spyOn(shell, 'rm'); + shellRmSpy = spyOn(shell, 'rm'); + }); + + + it('should log the number of existing downloads and downloads to be removed', () => { + cleaner.removeUnnecessaryDownloads(EXISTING_DOWNLOADS, OPEN_PRS); + + expect(loggerLogSpy).toHaveBeenCalledWith('Existing downloads: 4'); + expect(loggerLogSpy).toHaveBeenCalledWith('Removing 2 download(s): 20-ABCDEF0-build.zip, 20-1234567-build.zip'); + }); + + + it('should construct full paths to directories (by prepending \'downloadsDir\')', () => { + cleaner.removeUnnecessaryDownloads(['dl-1', 'dl-2', 'dl-3'], []); + + expect(shellRmSpy).toHaveBeenCalledWith(normalize('/downloads/dl-1')); + expect(shellRmSpy).toHaveBeenCalledWith(normalize('/downloads/dl-2')); + expect(shellRmSpy).toHaveBeenCalledWith(normalize('/downloads/dl-3')); }); it('should remove the downloads that do not correspond to open PRs', () => { cleaner.removeUnnecessaryDownloads(EXISTING_DOWNLOADS, OPEN_PRS); - expect(shell.rm).toHaveBeenCalledTimes(2); - expect(shell.rm).toHaveBeenCalledWith('downloads/20-ABCDEF0-build.zip'); - expect(shell.rm).toHaveBeenCalledWith('downloads/20-1234567-build.zip'); + expect(shellRmSpy).toHaveBeenCalledTimes(2); + expect(shellRmSpy).toHaveBeenCalledWith(normalize('/downloads/20-ABCDEF0-build.zip')); + expect(shellRmSpy).toHaveBeenCalledWith(normalize('/downloads/20-1234567-build.zip')); }); - - it('should log the number of existing builds and builds to be removed', () => { - cleaner.removeUnnecessaryDownloads(EXISTING_DOWNLOADS, OPEN_PRS); - - expect(loggerLogSpy).toHaveBeenCalledWith('Existing downloads: 4'); - expect(loggerLogSpy).toHaveBeenCalledWith( - 'Removing 2 download(s): downloads/20-ABCDEF0-build.zip, downloads/20-1234567-build.zip'); - }); }); }); diff --git a/aio/aio-builds-setup/dockerbuild/scripts-sh/clean-up.sh b/aio/aio-builds-setup/dockerbuild/scripts-sh/clean-up.sh index 951f88f1c1..49525ea46d 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-sh/clean-up.sh +++ b/aio/aio-builds-setup/dockerbuild/scripts-sh/clean-up.sh @@ -2,6 +2,7 @@ set -eu -o pipefail # Set up env variables +export AIO_CIRCLE_CI_TOKEN=UNUSED_CIRCLE_CI_TOKEN export AIO_GITHUB_TOKEN=$(head -c -1 /aio-secrets/GITHUB_TOKEN 2>/dev/null) # Run the clean-up