From 4268c8289822653f3b0404df03ff10470fc8ab88 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Sun, 25 Jun 2017 22:13:03 +0300 Subject: [PATCH] feat(aio): use shorter URLs for previews Use the 7 first characters of the 40-chars long SHAs for shorter/cleaner URLs. The collision probability is extremely low (since all SHAs are further "namespaced" under the corresponding PR). In case of a collision, the second PR will not be deployed, in order to avoid overwriting the original build. (This is a design decision to keep the implementation simple. It can be changed later if necessary.) --- .../dockerbuild/nginx/aio-builds.conf | 2 +- .../scripts-js/lib/common/constants.ts | 1 + .../lib/upload-server/build-creator.ts | 5 +- .../scripts-js/lib/verify-setup/helper.ts | 33 ++++-- .../scripts-js/lib/verify-setup/nginx.e2e.ts | 101 +++++++++++++----- .../verify-setup/server-integration.e2e.ts | 2 +- .../lib/verify-setup/upload-server.e2e.ts | 51 ++++++++- .../test/upload-server/build-creator.spec.ts | 8 +- .../docs/overview--general.md | 8 +- 9 files changed, 161 insertions(+), 50 deletions(-) diff --git a/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf b/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf index ce740f144e..2dcb3c5bb2 100644 --- a/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf +++ b/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf @@ -15,7 +15,7 @@ server { # Serve PR-preview requests server { - server_name "~^pr(?[1-9][0-9]*)-(?[0-9a-f]{40})\."; + server_name "~^pr(?[1-9][0-9]*)-(?[0-9a-f]{7,40})\."; listen {{$AIO_NGINX_PORT_HTTPS}} ssl http2; listen [::]:{{$AIO_NGINX_PORT_HTTPS}} ssl http2; 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 index b586f634ac..c5064c5dfc 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/constants.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/constants.ts @@ -1,2 +1,3 @@ // Constants export const HIDDEN_DIR_PREFIX = 'hidden--'; +export const SHORT_SHA_LEN = 7; 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 c0f1ce29cb..8b292544fb 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,7 +4,7 @@ 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 {HIDDEN_DIR_PREFIX, SHORT_SHA_LEN} from '../common/constants'; import {assertNotMissingOrEmpty} from '../common/utils'; import {ChangedPrVisibilityEvent, CreatedBuildEvent} from './build-events'; import {UploadError} from './upload-error'; @@ -46,6 +46,9 @@ export class BuildCreator extends EventEmitter { } public create(pr: string, sha: string, archivePath: string, isPublic: boolean): Promise { + // Use only part of the SHA for more readable URLs. + sha = sha.substr(0, SHORT_SHA_LEN); + const {oldPrDir: otherVisPrDir, newPrDir: prDir} = this.getCandidatePrDirs(pr, isPublic); const shaDir = path.join(prDir, sha); let dirToRemoveOnError: string; 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 62af299416..2fcbb05960 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,7 +4,7 @@ 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 {HIDDEN_DIR_PREFIX, SHORT_SHA_LEN} from '../common/constants'; import {getEnvVar} from '../common/utils'; // Constans @@ -51,8 +51,9 @@ class Helper { } // Methods - Public - public buildExists(pr: string, sha = '', isPublic = true): boolean { - const dir = path.join(this.getPrDir(pr, isPublic), sha); + public buildExists(pr: string, sha = '', isPublic = true, legacy = false): boolean { + const prDir = this.getPrDir(pr, isPublic); + const dir = !sha ? prDir : this.getShaDir(prDir, sha, legacy); return fs.existsSync(dir); } @@ -68,7 +69,7 @@ class Helper { } public createDummyArchive(pr: string, sha: string, archivePath: string): CleanUpFn { - const inputDir = path.join(this.buildsDir, 'uploaded', pr, sha); + const inputDir = this.getShaDir(this.getPrDir(`uploaded/${pr}`, true), sha); const cmd1 = `tar --create --gzip --directory "${inputDir}" --file "${archivePath}" .`; const cmd2 = `chown ${this.wwwUser} ${archivePath}`; @@ -80,9 +81,9 @@ class Helper { return this.createCleanUpFn(() => shell.rm('-rf', archivePath)); } - public createDummyBuild(pr: string, sha: string, isPublic = true, force = false): CleanUpFn { + public createDummyBuild(pr: string, sha: string, isPublic = true, force = false, legacy = false): CleanUpFn { const prDir = this.getPrDir(pr, isPublic); - const shaDir = path.join(prDir, sha); + const shaDir = this.getShaDir(prDir, sha, legacy); const idxPath = path.join(shaDir, 'index.html'); const barPath = path.join(shaDir, 'foo', 'bar.js'); @@ -108,9 +109,17 @@ class Helper { return path.join(this.buildsDir, prDirName); } - public readBuildFile(pr: string, sha: string, relFilePath: string, isPublic = true): string { - const prDir = this.getPrDir(pr, isPublic); - const absFilePath = path.join(prDir, sha, relFilePath); + public getShaDir(prDir: string, sha: string, legacy = false): string { + return path.join(prDir, legacy ? sha : this.getShordSha(sha)); + } + + public getShordSha(sha: string): string { + return sha.substr(0, SHORT_SHA_LEN); + } + + public readBuildFile(pr: string, sha: string, relFilePath: string, isPublic = true, legacy = false): string { + const shaDir = this.getShaDir(this.getPrDir(pr, isPublic), sha, legacy); + const absFilePath = path.join(shaDir, relFilePath); return fs.readFileSync(absFilePath, 'utf8'); } @@ -156,8 +165,10 @@ class Helper { }; } - public writeBuildFile(pr: string, sha: string, relFilePath: string, content: string, isPublic = true): CleanUpFn { - const absFilePath = path.join(this.getPrDir(pr, isPublic), sha, relFilePath); + public writeBuildFile(pr: string, sha: string, relFilePath: string, content: string, isPublic = true, + legacy = false): CleanUpFn { + const shaDir = this.getShaDir(this.getPrDir(pr, isPublic), sha, legacy); + const absFilePath = path.join(shaDir, relFilePath); return this.writeFile(absFilePath, {content}, true); } diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/nginx.e2e.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/nginx.e2e.ts index ba6a253d07..659a692578 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/nginx.e2e.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/nginx.e2e.ts @@ -37,6 +37,8 @@ describe(`nginx`, () => { const pr = '9'; const sha9 = '9'.repeat(40); const sha0 = '0'.repeat(40); + const shortSha9 = h.getShordSha(sha9); + const shortSha0 = h.getShordSha(sha0); describe(`pr-.${host}/*`, () => { @@ -50,9 +52,23 @@ describe(`nginx`, () => { it('should return /index.html', done => { + const origin = `${scheme}://pr${pr}-${shortSha9}.${host}`; + const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /index\\.html$`); + + Promise.all([ + h.runCmd(`curl -iL ${origin}/index.html`).then(h.verifyResponse(200, bodyRegex)), + h.runCmd(`curl -iL ${origin}/`).then(h.verifyResponse(200, bodyRegex)), + h.runCmd(`curl -iL ${origin}`).then(h.verifyResponse(200, bodyRegex)), + ]).then(done); + }); + + + it('should return /index.html (for legacy builds)', done => { const origin = `${scheme}://pr${pr}-${sha9}.${host}`; const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /index\\.html$`); + h.createDummyBuild(pr, sha9, true, false, true); + Promise.all([ h.runCmd(`curl -iL ${origin}/index.html`).then(h.verifyResponse(200, bodyRegex)), h.runCmd(`curl -iL ${origin}/`).then(h.verifyResponse(200, bodyRegex)), @@ -64,7 +80,19 @@ describe(`nginx`, () => { it('should return /foo/bar.js', done => { const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /foo/bar\\.js$`); - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/bar.js`). + h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}/foo/bar.js`). + then(h.verifyResponse(200, bodyRegex)). + then(done); + }); + + + it('should return /foo/bar.js (for legacy builds)', done => { + const origin = `${scheme}://pr${pr}-${sha9}.${host}`; + const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /foo/bar\\.js$`); + + h.createDummyBuild(pr, sha9, true, false, true); + + h.runCmd(`curl -iL ${origin}/foo/bar.js`). then(h.verifyResponse(200, bodyRegex)). then(done); }); @@ -72,58 +100,59 @@ describe(`nginx`, () => { it('should respond with 403 for directories', done => { Promise.all([ - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/`).then(h.verifyResponse(403)), - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo`).then(h.verifyResponse(403)), + h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}/foo/`).then(h.verifyResponse(403)), + h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}/foo`).then(h.verifyResponse(403)), ]).then(done); }); it('should respond with 404 for unknown paths to files', done => { - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/baz.css`). + h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}/foo/baz.css`). then(h.verifyResponse(404)). then(done); }); it('should rewrite to \'index.html\' for unknown paths that don\'t look like files', done => { + const origin = `${scheme}://pr${pr}-${shortSha9}.${host}`; const bodyRegex = new RegExp(`^PR: ${pr} | SHA: ${sha9} | File: /index\\.html$`); Promise.all([ - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/baz`).then(h.verifyResponse(200, bodyRegex)), - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}/foo/baz/`).then(h.verifyResponse(200, bodyRegex)), + h.runCmd(`curl -iL ${origin}/foo/baz`).then(h.verifyResponse(200, bodyRegex)), + h.runCmd(`curl -iL ${origin}/foo/baz/`).then(h.verifyResponse(200, bodyRegex)), ]).then(done); }); it('should respond with 404 for unknown PRs/SHAs', done => { const otherPr = 54321; - const otherSha = '8'.repeat(40); + const otherShortSha = h.getShordSha('8'.repeat(40)); Promise.all([ - h.runCmd(`curl -iL ${scheme}://pr${pr}9-${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://pr${otherPr}-${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}9.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://pr${pr}-${otherSha}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://pr${pr}9-${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://pr${otherPr}-${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}9.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://pr${pr}-${otherShortSha}.${host}`).then(h.verifyResponse(404)), ]).then(done); }); it('should respond with 404 if the subdomain format is wrong', done => { Promise.all([ - h.runCmd(`curl -iL ${scheme}://xpr${pr}-${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://prx${pr}-${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://xx${pr}-${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://p${pr}-${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://r${pr}-${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://${pr}-${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://pr${pr}${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://pr${pr}_${sha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://xpr${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://prx${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://xx${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://p${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://r${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://${pr}-${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://pr${pr}${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://pr${pr}_${shortSha9}.${host}`).then(h.verifyResponse(404)), ]).then(done); }); it('should reject PRs with leading zeros', done => { - h.runCmd(`curl -iL ${scheme}://pr0${pr}-${sha9}.${host}`). + h.runCmd(`curl -iL ${scheme}://pr0${pr}-${shortSha9}.${host}`). then(h.verifyResponse(404)). then(done); }); @@ -134,9 +163,9 @@ describe(`nginx`, () => { const bodyRegex0 = new RegExp(`^PR: ${pr} | SHA: ${sha0} | File: /index\\.html$`); Promise.all([ - h.runCmd(`curl -iL ${scheme}://pr${pr}-0${sha9}.${host}`).then(h.verifyResponse(404)), - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha9}.${host}`).then(h.verifyResponse(200, bodyRegex9)), - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha0}.${host}`).then(h.verifyResponse(200, bodyRegex0)), + h.runCmd(`curl -iL ${scheme}://pr${pr}-0${shortSha9}.${host}`).then(h.verifyResponse(404)), + h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha9}.${host}`).then(h.verifyResponse(200, bodyRegex9)), + h.runCmd(`curl -iL ${scheme}://pr${pr}-${shortSha0}.${host}`).then(h.verifyResponse(200, bodyRegex0)), ]).then(done); }); @@ -145,13 +174,31 @@ describe(`nginx`, () => { describe('(for hidden builds)', () => { - beforeEach(() => h.createDummyBuild(pr, sha9, false)); - - it('should respond with 404 for any file or directory', done => { + const origin = `${scheme}://pr${pr}-${shortSha9}.${host}`; + const assert404 = h.verifyResponse(404); + + h.createDummyBuild(pr, sha9, false); + expect(h.buildExists(pr, sha9, false)).toBe(true); + + Promise.all([ + h.runCmd(`curl -iL ${origin}/index.html`).then(assert404), + h.runCmd(`curl -iL ${origin}/`).then(assert404), + h.runCmd(`curl -iL ${origin}`).then(assert404), + h.runCmd(`curl -iL ${origin}/foo/bar.js`).then(assert404), + h.runCmd(`curl -iL ${origin}/foo/`).then(assert404), + h.runCmd(`curl -iL ${origin}/foo`).then(assert404), + ]).then(done); + }); + + + it('should respond with 404 for any file or directory (for legacy builds)', done => { const origin = `${scheme}://pr${pr}-${sha9}.${host}`; const assert404 = h.verifyResponse(404); + h.createDummyBuild(pr, sha9, false, false, true); + expect(h.buildExists(pr, sha9, false, true)).toBe(true); + Promise.all([ h.runCmd(`curl -iL ${origin}/index.html`).then(assert404), h.runCmd(`curl -iL ${origin}/`).then(assert404), @@ -272,7 +319,7 @@ describe(`nginx`, () => { describe(`${host}/*`, () => { - it('should respond with 404 for unkown URLs (even if the resource exists)', done => { + it('should respond with 404 for unknown URLs (even if the resource exists)', done => { ['index.html', 'foo.js', 'foo/index.html'].forEach(relFilePath => { const absFilePath = path.join(h.buildsDir, relFilePath); h.writeFile(absFilePath, {content: `File: /${relFilePath}`}); diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/server-integration.e2e.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/server-integration.e2e.ts index a77a030680..151b6adbac 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/server-integration.e2e.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/server-integration.e2e.ts @@ -12,7 +12,7 @@ h.runForAllSupportedSchemes((scheme, port) => describe(`integration (on ${scheme const archivePath = path.join(h.buildsDir, 'snapshot.tar.gz'); const getFile = (pr: string, sha: string, file: string) => - h.runCmd(`curl -iL ${scheme}://pr${pr}-${sha}.${host}/${file}`); + h.runCmd(`curl -iL ${scheme}://pr${pr}-${h.getShordSha(sha)}.${host}/${file}`); const uploadBuild = (pr: string, sha: string, archive: string, authHeader = 'Token FOO') => { // Using `FAKE_VERIFICATION_ERROR` or `FAKE_VERIFIED_NOT_TRUSTED` as `authHeader`, // we can fake the response of the overwritten `BuildVerifier.verify()` method. diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/upload-server.e2e.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/upload-server.e2e.ts index 7f2a37e9a9..4bc4621555 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/upload-server.e2e.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/upload-server.e2e.ts @@ -125,6 +125,27 @@ describe('upload-server (on HTTP)', () => { }); + it('should not overwrite existing builds (even if the SHA is different)', done => { + // Since only the first few characters of the SHA are used, it is possible for two different + // SHAs to correspond to the same directory. In that case, we don't want the second SHA to + // overwrite the first. + + const sha9Almost = sha9.replace(/.$/, '8'); + expect(sha9Almost).not.toBe(sha9); + + h.createDummyBuild(pr, sha9, isPublic); + expect(h.readBuildFile(pr, sha9, 'index.html', isPublic)).toContain('index.html'); + + h.writeBuildFile(pr, sha9, 'index.html', 'My content', isPublic); + expect(h.readBuildFile(pr, sha9, 'index.html', isPublic)).toBe('My content'); + + h.runCmd(`${cmdPrefix} http://${host}/create-build/${pr}/${sha9Almost}`). + then(h.verifyResponse(409, /^Request to overwrite existing directory/)). + then(() => expect(h.readBuildFile(pr, sha9, 'index.html', isPublic)).toBe('My content')). + then(done); + }); + + it('should delete the PR directory on error (for new PR)', done => { h.runCmd(`${cmdPrefix} http://${host}/create-build/${pr}/${sha9}`). then(h.verifyResponse(500)). @@ -175,7 +196,7 @@ describe('upload-server (on HTTP)', () => { it(`should create files/directories owned by '${h.wwwUser}'`, done => { const prDir = h.getPrDir(pr, isPublic); - const shaDir = path.join(prDir, sha9); + const shaDir = h.getShaDir(prDir, sha9); const idxPath = path.join(shaDir, 'index.html'); const barPath = path.join(shaDir, 'foo', 'bar.js'); @@ -204,7 +225,7 @@ describe('upload-server (on HTTP)', () => { it('should make the build directory non-writable', done => { const prDir = h.getPrDir(pr, isPublic); - const shaDir = path.join(prDir, sha9); + const shaDir = h.getShaDir(prDir, sha9); const idxPath = path.join(shaDir, 'index.html'); const barPath = path.join(shaDir, 'foo', 'bar.js'); @@ -224,6 +245,30 @@ describe('upload-server (on HTTP)', () => { then(done); }); + + it('should ignore a legacy 40-chars long build directory (even if it starts with the same chars)', done => { + // It is possible that 40-chars long build directories exist, if they had been deployed + // before implementing the shorter build directory names. In that case, we don't want the + // second (shorter) name to be considered the same as the old one (even if they originate + // from the same SHA). + + h.createDummyBuild(pr, sha9, isPublic, false, true); + expect(h.readBuildFile(pr, sha9, 'index.html', isPublic, true)).toContain('index.html'); + + h.writeBuildFile(pr, sha9, 'index.html', 'My content', isPublic, true); + expect(h.readBuildFile(pr, sha9, 'index.html', isPublic, true)).toBe('My content'); + + h.runCmd(`${cmdPrefix} http://${host}/create-build/${pr}/${sha9}`). + then(h.verifyResponse(statusCode)). + then(() => { + expect(h.buildExists(pr, sha9, isPublic)).toBe(true); + expect(h.buildExists(pr, sha9, isPublic, true)).toBe(true); + expect(h.readBuildFile(pr, sha9, 'index.html', isPublic)).toContain('index.html'); + expect(h.readBuildFile(pr, sha9, 'index.html', isPublic, true)).toBe('My content'); + }). + then(done); + }); + }); @@ -277,7 +322,7 @@ describe('upload-server (on HTTP)', () => { it('should reject the request if it fails to update the PR\'s visibility', done => { - // One way to cause an error is to have both a public and a hidden directory for the sme PR. + // One way to cause an error is to have both a public and a hidden directory for the same PR. h.createDummyBuild(pr, sha0, isPublic); expect(h.buildExists(pr, sha0, isPublic)).toBe(true); diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-creator.spec.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-creator.spec.ts index 7f3841b145..c665e3b1a4 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-creator.spec.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-creator.spec.ts @@ -4,6 +4,7 @@ import {EventEmitter} from 'events'; import * as fs from 'fs'; import * as path from 'path'; import * as shell from 'shelljs'; +import {SHORT_SHA_LEN} from '../../lib/common/constants'; import {BuildCreator} from '../../lib/upload-server/build-creator'; import {ChangedPrVisibilityEvent, CreatedBuildEvent} from '../../lib/upload-server/build-events'; import {UploadError} from '../../lib/upload-server/upload-error'; @@ -13,12 +14,13 @@ import {expectToBeUploadError} from './helpers'; describe('BuildCreator', () => { const pr = '9'; const sha = '9'.repeat(40); + const shortSha = sha.substr(0, SHORT_SHA_LEN); const archive = 'snapshot.tar.gz'; const buildsDir = 'builds/dir'; const hiddenPrDir = path.join(buildsDir, `hidden--${pr}`); const publicPrDir = path.join(buildsDir, pr); - const hiddenShaDir = path.join(hiddenPrDir, sha); - const publicShaDir = path.join(publicPrDir, sha); + const hiddenShaDir = path.join(hiddenPrDir, shortSha); + const publicShaDir = path.join(publicPrDir, shortSha); let bc: BuildCreator; beforeEach(() => bc = new BuildCreator(buildsDir)); @@ -267,7 +269,7 @@ describe('BuildCreator', () => { expect(type).toBe(CreatedBuildEvent.type); expect(evt).toEqual(jasmine.any(CreatedBuildEvent)); expect(evt.pr).toBe(+pr); - expect(evt.sha).toBe(sha); + expect(evt.sha).toBe(shortSha); expect(evt.isPublic).toBe(isPublic); emitted = true; diff --git a/aio/aio-builds-setup/docs/overview--general.md b/aio/aio-builds-setup/docs/overview--general.md index 9ec2c1be41..85704e4f7f 100644 --- a/aio/aio-builds-setup/docs/overview--general.md +++ b/aio/aio-builds-setup/docs/overview--general.md @@ -65,12 +65,14 @@ More info on how to set things up on CI can be found [here](misc--integrate-with found [here](overview--security-model.md)). - The upload-server changes the "visibility" of the associated PR, if necessary. For example, if builds for the same PR had been previously deployed as non-public and the current build has been - automatically verified, all previous builds are made public. + automatically verified, all previous builds are made public as well. If the PR transitions from "non-public" to "public", the upload-server posts a comment on the corresponding PR on GitHub mentioning the SHAs and the links where the previews can be found. - The upload-server verifies that the uploaded file is not trying to overwrite an existing build. -- The upload-server deploys the artifacts to a sub-directory named after the PR number and SHA: - `//` (Non-publicly accessible PRs will be stored in a different location.) +- The upload-server deploys the artifacts to a sub-directory named after the PR number and the first + few characters of the SHA: `//` + (Non-publicly accessible PRs will be stored in a different location, but again derived from the PR + number and SHA.) - If the PR is publicly accessible, the upload-server posts a comment on the corresponding PR on GitHub mentioning the SHA and the link where the preview can be found.