diff --git a/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf b/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf index 2dcb3c5bb2..ec2a244f48 100644 --- a/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf +++ b/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf @@ -88,6 +88,21 @@ server { resolver 127.0.0.1; } + # Notify about PR changes + location "~^/pr-updated/?$" { + if ($request_method != "POST") { + add_header Allow "POST"; + return 405; + } + + proxy_pass_request_headers on; + proxy_redirect off; + proxy_method POST; + proxy_pass http://{{$AIO_UPLOAD_HOSTNAME}}:{{$AIO_UPLOAD_PORT}}$request_uri; + + resolver 127.0.0.1; + } + # Everything else location / { return 404; diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts index c13a177330..48e60e3cc1 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts @@ -1,4 +1,5 @@ // Imports +import * as bodyParser from 'body-parser'; import * as express from 'express'; import * as http from 'http'; import {GithubPullRequests} from '../common/github-pull-requests'; @@ -84,6 +85,7 @@ class UploadServerFactory { protected createMiddleware(buildVerifier: BuildVerifier, buildCreator: BuildCreator): express.Express { const middleware = express(); + const jsonParser = bodyParser.json(); middleware.get(/^\/create-build\/([1-9][0-9]*)\/([0-9a-f]{40})\/?$/, (req, res) => { const pr = req.params[0]; @@ -96,8 +98,8 @@ class UploadServerFactory { } else if (!archive) { this.throwRequestError(400, `Missing or empty '${X_FILE_HEADER}' header`, req); } else { - buildVerifier. - verify(+pr, authHeader). + Promise.resolve(). + then(() => buildVerifier.verify(+pr, authHeader)). then(verStatus => verStatus === BUILD_VERIFICATION_STATUS.verifiedAndTrusted). then(isPublic => buildCreator.create(pr, sha, archive, isPublic). then(() => res.sendStatus(isPublic ? 201 : 202))). @@ -105,6 +107,22 @@ class UploadServerFactory { } }); middleware.get(/^\/health-check\/?$/, (_req, res) => res.sendStatus(200)); + middleware.post(/^\/pr-updated\/?$/, jsonParser, (req, res) => { + const {action, number: prNo}: {action?: string, number?: number} = req.body; + const visMayHaveChanged = !action || (action === 'labeled') || (action === 'unlabeled'); + + if (!visMayHaveChanged) { + res.sendStatus(200); + } else if (!prNo) { + this.throwRequestError(400, `Missing or empty 'number' field`, req); + } else { + Promise.resolve(). + then(() => buildVerifier.getPrIsTrusted(prNo)). + then(isPublic => buildCreator.updatePrVisibility(String(prNo), isPublic)). + then(() => res.sendStatus(200)). + catch(err => this.respondWithError(res, err)); + } + }); middleware.all('*', req => this.throwRequestError(404, 'Unknown resource', req)); middleware.use((err: any, _req: any, res: express.Response, _next: any) => this.respondWithError(res, err)); @@ -124,7 +142,10 @@ class UploadServerFactory { } protected throwRequestError(status: number, error: string, req: express.Request) { - throw new UploadError(status, `${error} in request: ${req.method} ${req.originalUrl}`); + const message = `${error} in request: ${req.method} ${req.originalUrl}` + + (!req.body ? '' : ` ${JSON.stringify(req.body)}`); + + throw new UploadError(status, message); } } diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/constants.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/constants.ts index ab5acb07cb..92e7f15a31 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/constants.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/constants.ts @@ -3,8 +3,14 @@ // not have direct access to the code (e.g. for mocking). // (See also 'lib/verify-setup/start-test-upload-server.ts'.) -// Special values to be used as `authHeader` in `BuildVerifier#verify()`. /* tslint:disable: variable-name */ + +// Special values to be used as `authHeader` in `BuildVerifier#verify()`. export const BV_verify_error = 'FAKE_VERIFICATION_ERROR'; export const BV_verify_verifiedNotTrusted = 'FAKE_VERIFIED_NOT_TRUSTED'; + +// Special values to be used as `pr` in `BuildVerifier#getPrIsTrusted()`. +export const BV_getPrIsTrusted_error = 32203; +export const BV_getPrIsTrusted_notTrusted = 72457; + /* tslint:enable: variable-name */ 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 659a692578..a3e15992f3 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 @@ -317,6 +317,51 @@ describe(`nginx`, () => { }); + describe(`${host}/pr-updated`, () => { + const url = `${scheme}://${host}/pr-updated`; + + + it('should disallow non-POST requests', done => { + Promise.all([ + h.runCmd(`curl -iLX GET ${url}`).then(h.verifyResponse([405, 'Not Allowed'])), + h.runCmd(`curl -iLX PUT ${url}`).then(h.verifyResponse([405, 'Not Allowed'])), + h.runCmd(`curl -iLX PATCH ${url}`).then(h.verifyResponse([405, 'Not Allowed'])), + h.runCmd(`curl -iLX DELETE ${url}`).then(h.verifyResponse([405, 'Not Allowed'])), + ]).then(done); + }); + + + it('should pass requests through to the upload server', done => { + const cmdPrefix = `curl -iLX POST --header "Content-Type: application/json"`; + + const cmd1 = `${cmdPrefix} ${url}`; + const cmd2 = `${cmdPrefix} --data '{"number":${pr}}' ${url}`; + const cmd3 = `${cmdPrefix} --data '{"number":${pr},"action":"foo"}' ${url}`; + + Promise.all([ + h.runCmd(cmd1).then(h.verifyResponse(400, /Missing or empty 'number' field/)), + h.runCmd(cmd2).then(h.verifyResponse(200)), + h.runCmd(cmd3).then(h.verifyResponse(200)), + ]).then(done); + }); + + + it('should respond with 404 for unknown paths', done => { + const cmdPrefix = `curl -iLX POST ${scheme}://${host}`; + + Promise.all([ + h.runCmd(`${cmdPrefix}/foo/pr-updated`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/foo-pr-updated`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/foonpr-updated`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/pr-updated/foo`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/pr-updated-foo`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/pr-updatednfoo`).then(h.verifyResponse(404)), + ]).then(done); + }); + + }); + + describe(`${host}/*`, () => { it('should respond with 404 for unknown URLs (even if the resource exists)', done => { 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 880084da76..6e9467e837 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 @@ -18,6 +18,11 @@ h.runForAllSupportedSchemes((scheme, port) => describe(`integration (on ${scheme const curlPost = `curl -iLX POST --header "Authorization: ${authHeader}"`; return h.runCmd(`${curlPost} --data-binary "@${archive}" ${scheme}://${host}/create-build/${pr}/${sha}`); }; + const prUpdated = (pr: number, action?: string) => { + const url = `${scheme}://${host}/pr-updated`; + const payloadStr = JSON.stringify({number: pr, action}); + return h.runCmd(`curl -iLX POST --header "Content-Type: application/json" --data '${payloadStr}' ${url}`); + }; beforeEach(() => jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000); afterEach(() => { @@ -27,7 +32,7 @@ h.runForAllSupportedSchemes((scheme, port) => describe(`integration (on ${scheme }); - describe('for a new PR', () => { + describe('for a new/non-existing PR', () => { it('should be able to upload and serve a public build', done => { const regexPrefix9 = `^PR: uploaded\\/${pr9} \\| SHA: ${sha9} \\| File:`; @@ -81,6 +86,18 @@ h.runForAllSupportedSchemes((scheme, port) => describe(`integration (on ${scheme then(done); }); + + it('should be able to notify that a PR has been updated (and do nothing)', done => { + prUpdated(+pr9). + then(h.verifyResponse(200)). + then(() => { + // The PR should still not exist. + expect(h.buildExists(pr9, '', false)).toBe(false); + expect(h.buildExists(pr9, '', true)).toBe(false); + }). + then(done); + }); + }); @@ -193,6 +210,110 @@ h.runForAllSupportedSchemes((scheme, port) => describe(`integration (on ${scheme then(done); }); + + it('should be able to request re-checking visibility (if outdated)', done => { + const publicPr = pr9; + const hiddenPr = String(c.BV_getPrIsTrusted_notTrusted); + + h.createDummyBuild(publicPr, sha9, false); + h.createDummyBuild(hiddenPr, sha9, true); + + // PR visibilities are outdated (i.e. the opposte of what the should). + expect(h.buildExists(publicPr, '', false)).toBe(true); + expect(h.buildExists(publicPr, '', true)).toBe(false); + expect(h.buildExists(hiddenPr, '', false)).toBe(false); + expect(h.buildExists(hiddenPr, '', true)).toBe(true); + + Promise. + all([ + prUpdated(+publicPr).then(h.verifyResponse(200)), + prUpdated(+hiddenPr).then(h.verifyResponse(200)), + ]). + then(() => { + // PR visibilities should have been updated. + expect(h.buildExists(publicPr, '', false)).toBe(false); + expect(h.buildExists(publicPr, '', true)).toBe(true); + expect(h.buildExists(hiddenPr, '', false)).toBe(true); + expect(h.buildExists(hiddenPr, '', true)).toBe(false); + }). + then(() => { + h.deletePrDir(publicPr, true); + h.deletePrDir(hiddenPr, false); + }). + then(done); + }); + + + it('should be able to request re-checking visibility (if up-to-date)', done => { + const publicPr = pr9; + const hiddenPr = String(c.BV_getPrIsTrusted_notTrusted); + + h.createDummyBuild(publicPr, sha9, true); + h.createDummyBuild(hiddenPr, sha9, false); + + // PR visibilities are already up-to-date. + expect(h.buildExists(publicPr, '', false)).toBe(false); + expect(h.buildExists(publicPr, '', true)).toBe(true); + expect(h.buildExists(hiddenPr, '', false)).toBe(true); + expect(h.buildExists(hiddenPr, '', true)).toBe(false); + + Promise. + all([ + prUpdated(+publicPr).then(h.verifyResponse(200)), + prUpdated(+hiddenPr).then(h.verifyResponse(200)), + ]). + then(() => { + // PR visibilities are still up-to-date. + expect(h.buildExists(publicPr, '', false)).toBe(false); + expect(h.buildExists(publicPr, '', true)).toBe(true); + expect(h.buildExists(hiddenPr, '', false)).toBe(true); + expect(h.buildExists(hiddenPr, '', true)).toBe(false); + }). + then(done); + }); + + + it('should reject a request if re-checking visibility fails', done => { + const errorPr = String(c.BV_getPrIsTrusted_error); + + h.createDummyBuild(errorPr, sha9, true); + + expect(h.buildExists(errorPr, '', false)).toBe(false); + expect(h.buildExists(errorPr, '', true)).toBe(true); + + prUpdated(+errorPr). + then(h.verifyResponse(500, /Test/)). + then(() => { + // PR visibility should not have been updated. + expect(h.buildExists(errorPr, '', false)).toBe(false); + expect(h.buildExists(errorPr, '', true)).toBe(true); + }). + then(done); + }); + + + it('should reject a request if updating visibility fails', done => { + // One way to cause an error is to have both a public and a hidden directory for the same PR. + h.createDummyBuild(pr9, sha9, false); + h.createDummyBuild(pr9, sha9, true); + + const hiddenPrDir = h.getPrDir(pr9, false); + const publicPrDir = h.getPrDir(pr9, true); + const bodyRegex = new RegExp(`Request to move '${hiddenPrDir}' to existing directory '${publicPrDir}'`); + + expect(h.buildExists(pr9, '', false)).toBe(true); + expect(h.buildExists(pr9, '', true)).toBe(true); + + prUpdated(+pr9). + then(h.verifyResponse(409, bodyRegex)). + then(() => { + // PR visibility should not have been updated. + expect(h.buildExists(pr9, '', false)).toBe(true); + expect(h.buildExists(pr9, '', true)).toBe(true); + }). + then(done); + }); + }); })); diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/start-test-upload-server.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/start-test-upload-server.ts index d649a902d0..3450d1bc0c 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/start-test-upload-server.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/start-test-upload-server.ts @@ -7,6 +7,19 @@ import * as c from './constants'; // Run // TODO(gkalpak): Add e2e tests to cover these interactions as well. GithubPullRequests.prototype.addComment = () => Promise.resolve(); +BuildVerifier.prototype.getPrIsTrusted = (pr: number) => { + switch (pr) { + case c.BV_getPrIsTrusted_error: + // For e2e tests, fake an error. + return Promise.reject('Test'); + case c.BV_getPrIsTrusted_notTrusted: + // For e2e tests, fake an untrusted PR (`false`). + return Promise.resolve(false); + default: + // For e2e tests, default to trusted PRs (`true`). + return Promise.resolve(true); + } +}; BuildVerifier.prototype.verify = (expectedPr: number, authHeader: string) => { switch (authHeader) { case c.BV_verify_error: 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 ab058a3303..6191a495f4 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 @@ -374,6 +374,181 @@ describe('upload-server (on HTTP)', () => { }); + describe(`${host}/pr-updated`, () => { + const url = `http://${host}/pr-updated`; + + // Helpers + const curl = (payload?: {number: number, action?: string}) => { + const payloadStr = payload && JSON.stringify(payload) || ''; + return `curl -iLX POST --header "Content-Type: application/json" --data '${payloadStr}' ${url}`; + }; + + + it('should disallow non-POST requests', done => { + const bodyRegex = /^Unknown resource in request/; + + Promise.all([ + h.runCmd(`curl -iLX GET ${url}`).then(h.verifyResponse(404, bodyRegex)), + h.runCmd(`curl -iLX PUT ${url}`).then(h.verifyResponse(404, bodyRegex)), + h.runCmd(`curl -iLX PATCH ${url}`).then(h.verifyResponse(404, bodyRegex)), + h.runCmd(`curl -iLX DELETE ${url}`).then(h.verifyResponse(404, bodyRegex)), + ]).then(done); + }); + + + it('should respond with 400 for requests without a payload', done => { + const bodyRegex = /^Missing or empty 'number' field in request/; + + h.runCmd(curl()). + then(h.verifyResponse(400, bodyRegex)). + then(done); + }); + + + it('should respond with 400 for requests without a \'number\' field', done => { + const bodyRegex = /^Missing or empty 'number' field in request/; + + Promise.all([ + h.runCmd(curl({} as any)).then(h.verifyResponse(400, bodyRegex)), + h.runCmd(curl({number: null} as any)).then(h.verifyResponse(400, bodyRegex)), + ]).then(done); + }); + + + it('should reject requests for which checking the PR visibility fails', done => { + h.runCmd(curl({number: c.BV_getPrIsTrusted_error})). + then(h.verifyResponse(500, /Test/)). + then(done); + }); + + + it('should respond with 404 for unknown paths', done => { + const mockPayload = JSON.stringify({number: +pr}); + const cmdPrefix = `curl -iLX POST --data "${mockPayload}" http://${host}`; + + Promise.all([ + h.runCmd(`${cmdPrefix}/foo/pr-updated`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/foo-pr-updated`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/foonpr-updated`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/pr-updated/foo`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/pr-updated-foo`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/pr-updatednfoo`).then(h.verifyResponse(404)), + ]).then(done); + }); + + + it('should do nothing if PR\'s visibility is already up-to-date', done => { + const publicPr = pr; + const hiddenPr = String(c.BV_getPrIsTrusted_notTrusted); + const checkVisibilities = () => { + // Public build is already public. + expect(h.buildExists(publicPr, '', false)).toBe(false); + expect(h.buildExists(publicPr, '', true)).toBe(true); + // Hidden build is already hidden. + expect(h.buildExists(hiddenPr, '', false)).toBe(true); + expect(h.buildExists(hiddenPr, '', true)).toBe(false); + }; + + h.createDummyBuild(publicPr, sha9, true); + h.createDummyBuild(hiddenPr, sha9, false); + checkVisibilities(); + + Promise. + all([ + h.runCmd(curl({number: +publicPr, action: 'foo'})).then(h.verifyResponse(200)), + h.runCmd(curl({number: +hiddenPr, action: 'foo'})).then(h.verifyResponse(200)), + ]). + // Visibilities should not have changed, because the specified action could not have triggered a change. + then(checkVisibilities). + then(done); + }); + + + it('should do nothing if \'action\' implies no visibility change', done => { + const publicPr = pr; + const hiddenPr = String(c.BV_getPrIsTrusted_notTrusted); + const checkVisibilities = () => { + // Public build is hidden atm. + expect(h.buildExists(publicPr, '', false)).toBe(true); + expect(h.buildExists(publicPr, '', true)).toBe(false); + // Hidden build is public atm. + expect(h.buildExists(hiddenPr, '', false)).toBe(false); + expect(h.buildExists(hiddenPr, '', true)).toBe(true); + }; + + h.createDummyBuild(publicPr, sha9, false); + h.createDummyBuild(hiddenPr, sha9, true); + checkVisibilities(); + + Promise. + all([ + h.runCmd(curl({number: +publicPr, action: 'foo'})).then(h.verifyResponse(200)), + h.runCmd(curl({number: +hiddenPr, action: 'foo'})).then(h.verifyResponse(200)), + ]). + // Visibilities should not have changed, because the specified action could not have triggered a change. + then(checkVisibilities). + then(done); + }); + + + describe('when the visiblity has changed', () => { + const publicPr = pr; + const hiddenPr = String(c.BV_getPrIsTrusted_notTrusted); + + beforeEach(() => { + // Create initial PR builds with opposite visibilities as the ones that will be reported: + // - The now public PR was previously hidden. + // - The now hidden PR was previously public. + h.createDummyBuild(publicPr, sha9, false); + h.createDummyBuild(hiddenPr, sha9, true); + + expect(h.buildExists(publicPr, '', false)).toBe(true); + expect(h.buildExists(publicPr, '', true)).toBe(false); + expect(h.buildExists(hiddenPr, '', false)).toBe(false); + expect(h.buildExists(hiddenPr, '', true)).toBe(true); + }); + afterEach(() => { + // Expect PRs' visibility to have been updated: + // - The public PR should be actually public (previously it was hidden). + // - The hidden PR should be actually hidden (previously it was public). + expect(h.buildExists(publicPr, '', false)).toBe(false); + expect(h.buildExists(publicPr, '', true)).toBe(true); + expect(h.buildExists(hiddenPr, '', false)).toBe(true); + expect(h.buildExists(hiddenPr, '', true)).toBe(false); + + h.deletePrDir(publicPr, true); + h.deletePrDir(hiddenPr, false); + }); + + + it('should update the PR\'s visibility (action: undefined)', done => { + Promise.all([ + h.runCmd(curl({number: +publicPr})).then(h.verifyResponse(200)), + h.runCmd(curl({number: +hiddenPr})).then(h.verifyResponse(200)), + ]).then(done); + }); + + + it('should update the PR\'s visibility (action: labeled)', done => { + Promise.all([ + h.runCmd(curl({number: +publicPr, action: 'labeled'})).then(h.verifyResponse(200)), + h.runCmd(curl({number: +hiddenPr, action: 'labeled'})).then(h.verifyResponse(200)), + ]).then(done); + }); + + + it('should update the PR\'s visibility (action: unlabeled)', done => { + Promise.all([ + h.runCmd(curl({number: +publicPr, action: 'unlabeled'})).then(h.verifyResponse(200)), + h.runCmd(curl({number: +hiddenPr, action: 'unlabeled'})).then(h.verifyResponse(200)), + ]).then(done); + }); + + }); + + }); + + describe(`${host}/*`, () => { it('should respond with 404 for requests to unknown URLs', done => { diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/package.json b/aio/aio-builds-setup/dockerbuild/scripts-js/package.json index c196c187c2..bd9ee17b04 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/package.json +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/package.json @@ -20,12 +20,14 @@ "test-watch": "nodemon --exec \"yarn ~~test-only\" --watch dist" }, "dependencies": { + "body-parser": "^1.17.2", "express": "^4.14.1", "jasmine": "^2.5.3", "jsonwebtoken": "^7.3.0", "shelljs": "^0.7.6" }, "devDependencies": { + "@types/body-parser": "^1.16.4", "@types/express": "^4.0.35", "@types/jasmine": "^2.5.43", "@types/jsonwebtoken": "^7.2.0", diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/upload-server-factory.spec.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/upload-server-factory.spec.ts index 2e0c9eae99..c2b406b3e0 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/upload-server-factory.spec.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/upload-server-factory.spec.ts @@ -442,6 +442,146 @@ describe('uploadServerFactory', () => { }); + describe('POST /pr-updated', () => { + const pr = '9'; + const url = '/pr-updated'; + let bvGetPrIsTrustedSpy: jasmine.Spy; + let bcUpdatePrVisibilitySpy: jasmine.Spy; + + // Helpers + const createRequest = (num: number, action?: string) => + agent.post(url).send({number: num, action}); + + beforeEach(() => { + bvGetPrIsTrustedSpy = spyOn(buildVerifier, 'getPrIsTrusted'); + bcUpdatePrVisibilitySpy = spyOn(buildCreator, 'updatePrVisibility'); + }); + + + it('should respond with 404 for non-POST requests', done => { + verifyRequests([ + agent.get(url).expect(404), + agent.put(url).expect(404), + agent.patch(url).expect(404), + agent.delete(url).expect(404), + ], done); + }); + + + it('should respond with 400 for requests without a payload', done => { + const responseBody = `Missing or empty 'number' field in request: POST ${url} {}`; + + const request1 = agent.post(url); + const request2 = agent.post(url).send(); + + verifyRequests([ + request1.expect(400, responseBody), + request2.expect(400, responseBody), + ], done); + }); + + + it('should respond with 400 for requests without a \'number\' field', done => { + const responseBodyPrefix = `Missing or empty 'number' field in request: POST ${url}`; + + const request1 = agent.post(url).send({}); + const request2 = agent.post(url).send({number: null}); + + verifyRequests([ + request1.expect(400, `${responseBodyPrefix} {}`), + request2.expect(400, `${responseBodyPrefix} {"number":null}`), + ], done); + }); + + + it('should call \'BuildVerifier#gtPrIsTrusted()\' with the correct arguments', done => { + const req = createRequest(+pr); + + promisifyRequest(req). + then(() => expect(bvGetPrIsTrustedSpy).toHaveBeenCalledWith(9)). + then(done, done.fail); + }); + + + it('should propagate errors from BuildVerifier', done => { + bvGetPrIsTrustedSpy.and.callFake(() => Promise.reject('Test')); + + const req = createRequest(+pr).expect(500, 'Test'); + + promisifyRequest(req). + then(() => { + expect(bvGetPrIsTrustedSpy).toHaveBeenCalledWith(9); + expect(bcUpdatePrVisibilitySpy).not.toHaveBeenCalled(); + }). + then(done, done.fail); + }); + + + it('should call \'BuildCreator#updatePrVisibility()\' with the correct arguments', done => { + bvGetPrIsTrustedSpy.and.callFake((pr2: number) => Promise.resolve(pr2 === 42)); + + const req1 = createRequest(24); + const req2 = createRequest(42); + + Promise.all([ + promisifyRequest(req1).then(() => expect(bcUpdatePrVisibilitySpy).toHaveBeenCalledWith('24', false)), + promisifyRequest(req2).then(() => expect(bcUpdatePrVisibilitySpy).toHaveBeenCalledWith('42', true)), + ]).then(done, done.fail); + }); + + + it('should propagate errors from BuildCreator', done => { + bcUpdatePrVisibilitySpy.and.callFake(() => Promise.reject('Test')); + + const req = createRequest(+pr).expect(500, 'Test'); + verifyRequests([req], done); + }); + + + describe('on success', () => { + + it('should respond with 200 (action: undefined)', done => { + bvGetPrIsTrustedSpy.and.returnValues(Promise.resolve(true), Promise.resolve(false)); + + const reqs = [4, 2].map(num => createRequest(num).expect(200, http.STATUS_CODES[200])); + verifyRequests(reqs, done); + }); + + + it('should respond with 200 (action: labeled)', done => { + bvGetPrIsTrustedSpy.and.returnValues(Promise.resolve(true), Promise.resolve(false)); + + const reqs = [4, 2].map(num => createRequest(num, 'labeled').expect(200, http.STATUS_CODES[200])); + verifyRequests(reqs, done); + }); + + + it('should respond with 200 (action: unlabeled)', done => { + bvGetPrIsTrustedSpy.and.returnValues(Promise.resolve(true), Promise.resolve(false)); + + const reqs = [4, 2].map(num => createRequest(num, 'unlabeled').expect(200, http.STATUS_CODES[200])); + verifyRequests(reqs, done); + }); + + + it('should respond with 200 (and do nothing) if \'action\' implies no visibility change', done => { + const promises = ['foo', 'notlabeled']. + map(action => createRequest(+pr, action).expect(200, http.STATUS_CODES[200])). + map(promisifyRequest); + + Promise.all(promises). + then(() => { + expect(bvGetPrIsTrustedSpy).not.toHaveBeenCalled(); + expect(bcUpdatePrVisibilitySpy).not.toHaveBeenCalled(); + }). + then(done, done.fail); + }); + + }); + + }); + + describe('ALL *', () => { it('should respond with 404', done => { diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/yarn.lock b/aio/aio-builds-setup/dockerbuild/scripts-js/yarn.lock index ff830563f3..37590de7f2 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/yarn.lock +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/yarn.lock @@ -2,13 +2,20 @@ # yarn lockfile v1 +"@types/body-parser@^1.16.4": + version "1.16.4" + resolved "https://registry.yarnpkg.com/@types/body-parser/-/body-parser-1.16.4.tgz#96f3660e6f88a677fee7250f5a5e6d6bda3c76bb" + dependencies: + "@types/express" "*" + "@types/node" "*" + "@types/express-serve-static-core@*": version "4.0.48" resolved "https://registry.yarnpkg.com/@types/express-serve-static-core/-/express-serve-static-core-4.0.48.tgz#b4fa06b0fce282e582b4535ff7fac85cc90173e9" dependencies: "@types/node" "*" -"@types/express@^4.0.35": +"@types/express@*", "@types/express@^4.0.35": version "4.0.36" resolved "https://registry.yarnpkg.com/@types/express/-/express-4.0.36.tgz#14eb47de7ecb10319f0a2fb1cf971aa8680758c2" dependencies: @@ -236,6 +243,21 @@ block-stream@*: dependencies: inherits "~2.0.0" +body-parser@^1.17.2: + version "1.17.2" + resolved "https://registry.yarnpkg.com/body-parser/-/body-parser-1.17.2.tgz#f8892abc8f9e627d42aedafbca66bf5ab99104ee" + dependencies: + bytes "2.4.0" + content-type "~1.0.2" + debug "2.6.7" + depd "~1.1.0" + http-errors "~1.6.1" + iconv-lite "0.4.15" + on-finished "~2.3.0" + qs "6.4.0" + raw-body "~2.2.0" + type-is "~1.6.15" + boom@2.x.x: version "2.10.1" resolved "https://registry.yarnpkg.com/boom/-/boom-2.10.1.tgz#39c8918ceff5799f83f9492a848f625add0c766f" @@ -273,6 +295,10 @@ buffer-equal-constant-time@1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/buffer-equal-constant-time/-/buffer-equal-constant-time-1.0.1.tgz#f8e71132f7ffe6e01a5c9697a4c6f3e48d5cc819" +bytes@2.4.0: + version "2.4.0" + resolved "https://registry.yarnpkg.com/bytes/-/bytes-2.4.0.tgz#7d97196f9d5baf7f6935e25985549edd2a6c2339" + caller-path@^0.1.0: version "0.1.0" resolved "https://registry.yarnpkg.com/caller-path/-/caller-path-0.1.0.tgz#94085ef63581ecd3daa92444a8fe94e82577751f" @@ -1158,6 +1184,10 @@ http-signature@~1.1.0: jsprim "^1.2.2" sshpk "^1.7.0" +iconv-lite@0.4.15: + version "0.4.15" + resolved "https://registry.yarnpkg.com/iconv-lite/-/iconv-lite-0.4.15.tgz#fe265a218ac6a57cfe854927e9d04c19825eddeb" + ignore-by-default@^1.0.0: version "1.0.1" resolved "https://registry.yarnpkg.com/ignore-by-default/-/ignore-by-default-1.0.1.tgz#48ca6d72f6c6a3af00a9ad4ae6876be3889e2b09" @@ -1958,6 +1988,14 @@ range-parser@~1.2.0: version "1.2.0" resolved "https://registry.yarnpkg.com/range-parser/-/range-parser-1.2.0.tgz#f49be6b487894ddc40dcc94a322f611092e00d5e" +raw-body@~2.2.0: + version "2.2.0" + resolved "https://registry.yarnpkg.com/raw-body/-/raw-body-2.2.0.tgz#994976cf6a5096a41162840492f0bdc5d6e7fb96" + dependencies: + bytes "2.4.0" + iconv-lite "0.4.15" + unpipe "1.0.0" + rc@^1.0.1, rc@^1.1.6, rc@^1.1.7: version "1.2.1" resolved "https://registry.yarnpkg.com/rc/-/rc-1.2.1.tgz#2e03e8e42ee450b8cb3dce65be1bf8974e1dfd95" @@ -2477,7 +2515,7 @@ unique-string@^1.0.0: dependencies: crypto-random-string "^1.0.0" -unpipe@~1.0.0: +unpipe@1.0.0, unpipe@~1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/unpipe/-/unpipe-1.0.0.tgz#b2bf4ee8514aae6165b4817829d21b2ef49904ec" diff --git a/aio/aio-builds-setup/docs/overview--general.md b/aio/aio-builds-setup/docs/overview--general.md index 85704e4f7f..0637e5235c 100644 --- a/aio/aio-builds-setup/docs/overview--general.md +++ b/aio/aio-builds-setup/docs/overview--general.md @@ -80,13 +80,31 @@ More info on the possible HTTP status codes and their meaning can be found [here](overview--http-status-codes.md). +### Updating PR visibility +- nginx receives a natification that a PR has been updated and passes it through to the + upload-server. This could, for example, be sent by a GitHub webhook every time a PR's labels + change. + E.g.: `ngbuilds.io/pr-updated` (payload: `{"number":,"action":"labeled"}`) +- The request contains the PR number (as `number`) and optionally the action that triggered the + request (as `action`) in the payload. +- The upload-server verifies the payload and determines whether the `action` (if specified) could + have led to PR visibility changes. Only requests that omit the `action` field altogether or + specify an action that can affect visibility are further processed. + (Currently, the only actions that are considered capable of affecting visibility are `labeled` and + `unlabeled`.) +- The upload-server re-checks and if necessary updates the PR's visibility. + +More info on the possible HTTP status codes and their meaning can be found +[here](overview--http-status-codes.md). + + ### Serving build artifacts - nginx receives a request for an uploaded resource on a subdomain corresponding to the PR and SHA. E.g.: `pr-.ngbuilds.io/path/to/resource` - nginx maps the subdomain to the correct sub-directory and serves the resource. E.g.: `///path/to/resource` -Again, more info on the possible HTTP status codes and their meaning can be found +More info on the possible HTTP status codes and their meaning can be found [here](overview--http-status-codes.md). diff --git a/aio/aio-builds-setup/docs/overview--http-status-codes.md b/aio/aio-builds-setup/docs/overview--http-status-codes.md index 1fcd6eda35..54b06f66e8 100644 --- a/aio/aio-builds-setup/docs/overview--http-status-codes.md +++ b/aio/aio-builds-setup/docs/overview--http-status-codes.md @@ -53,6 +53,28 @@ with a bried explanation of what they mean: Payload larger than size specified in `AIO_UPLOAD_MAX_SIZE`. +## `https://ngbuilds.io/health-check` + +- **200 (OK)**: + The server is healthy (i.e. up and running and processing requests). + + +## `https://ngbuilds.io/pr-updated` + +- **200 (OK)**: + Request processed successfully. Processing may or may not have resulted in further actions. + +- **400 (Bad Request)**: + No payload or no `number` field in payload. + +- **405 (Method Not Allowed)**: + Request method other than POST. + +- **409 (Conflict)**: + Request to overwrite existing directory (i.e. directories for both visibilities exist). + (Normally, this should not happen.) + + ## `https://*.ngbuilds.io/*` - **404 (Not Found)**: