From 2e0de01372fccbd96a8ccc217efdef0d2aeb788c Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Sun, 26 Aug 2018 00:29:14 +0300 Subject: [PATCH] feat(docs-infra): add API endpoint for checking if PR can have preview (#25671) There several reasons why PRs cannot have (public) previews: - The PR did not affect any relevant files (e.g. non-spec files in `aio/` or `packages/`). - The PR cannot be automatically verified as "trusted" (based on its author or labels). Note: The endpoint does not check whether there currently is a (public) preview for the specified PR; only whether there can be one. PR Close #25671 --- .../dockerbuild/nginx/aio-builds.conf | 15 +++ .../preview-server/preview-server-factory.ts | 28 +++++- .../scripts-js/lib/verify-setup/helper.ts | 30 ++++-- .../scripts-js/lib/verify-setup/nginx.e2e.ts | 38 +++++++ .../lib/verify-setup/preview-server.e2e.ts | 86 ++++++++++++++++ .../preview-server-factory.spec.ts | 99 ++++++++++++++++++- .../docs/overview--http-status-codes.md | 17 ++++ 7 files changed, 304 insertions(+), 9 deletions(-) diff --git a/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf b/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf index 74b38efa69..f2fadcafb5 100644 --- a/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf +++ b/aio/aio-builds-setup/dockerbuild/nginx/aio-builds.conf @@ -66,6 +66,21 @@ server { return 200 ''; } + # Check PRs previewability + location "~^/can-have-public-preview/\d+/?$" { + if ($request_method != "GET") { + add_header Allow "GET"; + return 405; + } + + proxy_pass_request_headers on; + proxy_redirect off; + proxy_method GET; + proxy_pass http://{{$AIO_PREVIEW_SERVER_HOSTNAME}}:{{$AIO_PREVIEW_SERVER_PORT}}$request_uri; + + resolver 127.0.0.1; + } + # Notify about CircleCI builds location "~^/circle-build/?$" { if ($request_method != "POST") { diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/preview-server/preview-server-factory.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/preview-server/preview-server-factory.ts index f8c8fe7ddc..21186f4539 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/preview-server/preview-server-factory.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/preview-server/preview-server-factory.ts @@ -64,10 +64,36 @@ export class PreviewServerFactory { buildCreator: BuildCreator, cfg: PreviewServerConfig): express.Express { const middleware = express(); const jsonParser = bodyParser.json(); + const significantFilesRe = new RegExp(cfg.significantFilesPattern); // RESPOND TO IS-ALIVE PING middleware.get(/^\/health-check\/?$/, (_req, res) => res.sendStatus(200)); + // RESPOND TO CAN-HAVE-PUBLIC-PREVIEW CHECK + const canHavePublicPreviewRe = /^\/can-have-public-preview\/(\d+)\/?$/; + middleware.get(canHavePublicPreviewRe, async (req, res) => { + try { + const pr = +canHavePublicPreviewRe.exec(req.url)![1]; + + if (!await buildVerifier.getSignificantFilesChanged(pr, significantFilesRe)) { + // Cannot have preview: PR did not touch relevant files: `aio/` or `packages/` (except for spec files). + res.send({canHavePublicPreview: false, reason: 'No significant files touched.'}); + logger.log(`PR:${pr} - Cannot have a public preview, because it did not touch any significant files.`); + } else if (!await buildVerifier.getPrIsTrusted(pr)) { + // Cannot have preview: PR not automatically verifiable as "trusted". + res.send({canHavePublicPreview: false, reason: 'Not automatically verifiable as "trusted".'}); + logger.log(`PR:${pr} - Cannot have a public preview, because not automatically verifiable as "trusted".`); + } else { + // Can have preview. + res.send({canHavePublicPreview: true, reason: null}); + logger.log(`PR:${pr} - Can have a public preview.`); + } + } catch (err) { + logger.error('Previewability check error', err); + respondWithError(res, err); + } + }); + // CIRCLE_CI BUILD COMPLETE WEBHOOK middleware.post(/^\/circle-build\/?$/, jsonParser, async (req, res) => { try { @@ -108,7 +134,7 @@ export class PreviewServerFactory { `Invalid webhook: expected "githubRepo" property to equal "${cfg.githubRepo}" but got "${repo}".`); // Do not deploy unless this PR has touched relevant files: `aio/` or `packages/` (except for spec files) - if (!await buildVerifier.getSignificantFilesChanged(pr, new RegExp(cfg.significantFilesPattern))) { + if (!await buildVerifier.getSignificantFilesChanged(pr, significantFilesRe)) { res.sendStatus(204); logger.log(`PR:${pr}, Build:${buildNum} - ` + `Skipping preview processing because this PR did not touch any significant files.`); 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 c1bd4772ad..f267e608dd 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 @@ -105,7 +105,7 @@ class Helper { Object.keys(this.portPerScheme).forEach(scheme => suiteFactory(scheme, this.portPerScheme[scheme])); } - public verifyResponse(status: number | [number, string], regex = /^/): VerifyCmdResultFn { + public verifyResponse(status: number | [number, string], regex: string | RegExp = /^/): VerifyCmdResultFn { let statusCode: number; let statusText: string; @@ -180,26 +180,42 @@ class Helper { } } +interface DefaultCurlOptions { + defaultMethod?: CurlOptions['method']; + defaultOptions?: CurlOptions['options']; + defaultHeaders?: CurlOptions['headers']; + defaultData?: CurlOptions['data']; + defaultExtraPath?: CurlOptions['extraPath']; +} + interface CurlOptions { method?: string; options?: string; + headers?: string[]; data?: any; url?: string; extraPath?: string; } -export function makeCurl(baseUrl: string) { +export function makeCurl(baseUrl: string, { + defaultMethod = 'POST', + defaultOptions = '', + defaultHeaders = ['Content-Type: application/json'], + defaultData = {}, + defaultExtraPath = '', +}: DefaultCurlOptions = {}) { return function curl({ - method = 'POST', - options = '', - data = {}, + method = defaultMethod, + options = defaultOptions, + headers = defaultHeaders, + data = defaultData, url = baseUrl, - extraPath = '', + extraPath = defaultExtraPath, }: CurlOptions) { const dataString = data ? JSON.stringify(data) : ''; const cmd = `curl -iLX ${method} ` + `${options} ` + - `--header "Content-Type: application/json" ` + + headers.map(header => `--header "${header}" `).join('') + `--data '${dataString}' ` + `${url}${extraPath}`; return helper.runCmd(cmd); 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 1979a621de..656973b205 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 @@ -3,6 +3,7 @@ import * as path from 'path'; import {rm} from 'shelljs'; import {AIO_BUILDS_DIR, AIO_NGINX_HOSTNAME, AIO_NGINX_PORT_HTTP, AIO_NGINX_PORT_HTTPS} from '../common/env-variables'; import {computeShortSha} from '../common/utils'; +import {PrNums} from './constants'; import {helper as h} from './helper'; import {customMatchers} from './jasmine-custom-matchers'; @@ -252,6 +253,42 @@ describe(`nginx`, () => { }); + describe(`${host}/can-have-public-preview`, () => { + const baseUrl = `${scheme}://${host}/can-have-public-preview`; + + + it('should disallow non-GET requests', async () => { + await Promise.all([ + h.runCmd(`curl -iLX POST ${baseUrl}/42`).then(h.verifyResponse([405, 'Not Allowed'])), + h.runCmd(`curl -iLX PUT ${baseUrl}/42`).then(h.verifyResponse([405, 'Not Allowed'])), + h.runCmd(`curl -iLX PATCH ${baseUrl}/42`).then(h.verifyResponse([405, 'Not Allowed'])), + h.runCmd(`curl -iLX DELETE ${baseUrl}/42`).then(h.verifyResponse([405, 'Not Allowed'])), + ]); + }); + + + it('should pass requests through to the preview server', async () => { + await h.runCmd(`curl -iLX GET ${baseUrl}/${PrNums.CHANGED_FILES_ERROR}`). + then(h.verifyResponse(500, /CHANGED_FILES_ERROR/)); + }); + + + it('should respond with 404 for unknown paths', async () => { + const cmdPrefix = `curl -iLX GET ${baseUrl}`; + + await Promise.all([ + h.runCmd(`${cmdPrefix}/foo/42`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}-foo/42`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}nfoo/42`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/42/foo`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/f00`).then(h.verifyResponse(404)), + h.runCmd(`${cmdPrefix}/`).then(h.verifyResponse(404)), + ]); + }); + + }); + + describe(`${host}/circle-build`, () => { it('should disallow non-POST requests', done => { @@ -287,6 +324,7 @@ describe(`nginx`, () => { h.runCmd(`${cmdPrefix}/circle-build/42`).then(h.verifyResponse(404)), ]).then(done); }); + }); diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/preview-server.e2e.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/preview-server.e2e.ts index 8ae9bf77e7..f9667a9d2d 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/preview-server.e2e.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/preview-server.e2e.ts @@ -18,6 +18,92 @@ describe('preview-server', () => { afterEach(() => h.cleanUp()); + describe(`${host}/can-have-public-preview`, () => { + const curl = makeCurl(`${host}/can-have-public-preview`, { + defaultData: null, + defaultExtraPath: `/${PrNums.TRUST_CHECK_ACTIVE_TRUSTED_USER}`, + defaultHeaders: [], + defaultMethod: 'GET', + }); + + + it('should disallow non-GET requests', async () => { + const bodyRegex = /^Unknown resource in request/; + + await Promise.all([ + curl({method: 'POST'}).then(h.verifyResponse(404, bodyRegex)), + curl({method: 'PUT'}).then(h.verifyResponse(404, bodyRegex)), + curl({method: 'PATCH'}).then(h.verifyResponse(404, bodyRegex)), + curl({method: 'DELETE'}).then(h.verifyResponse(404, bodyRegex)), + ]); + }); + + + it('should respond with 404 for unknown paths', async () => { + const bodyRegex = /^Unknown resource in request/; + + await Promise.all([ + curl({extraPath: `/foo/${PrNums.TRUST_CHECK_ACTIVE_TRUSTED_USER}`}).then(h.verifyResponse(404, bodyRegex)), + curl({extraPath: `-foo/${PrNums.TRUST_CHECK_ACTIVE_TRUSTED_USER}`}).then(h.verifyResponse(404, bodyRegex)), + curl({extraPath: `nfoo/${PrNums.TRUST_CHECK_ACTIVE_TRUSTED_USER}`}).then(h.verifyResponse(404, bodyRegex)), + curl({extraPath: `/${PrNums.TRUST_CHECK_ACTIVE_TRUSTED_USER}/foo`}).then(h.verifyResponse(404, bodyRegex)), + curl({extraPath: '/f00'}).then(h.verifyResponse(404, bodyRegex)), + curl({extraPath: '/'}).then(h.verifyResponse(404, bodyRegex)), + ]); + }); + + + it('should respond with 500 if checking for significant file changes fails', async () => { + await Promise.all([ + curl({extraPath: `/${PrNums.CHANGED_FILES_404}`}).then(h.verifyResponse(500, /CHANGED_FILES_404/)), + curl({extraPath: `/${PrNums.CHANGED_FILES_ERROR}`}).then(h.verifyResponse(500, /CHANGED_FILES_ERROR/)), + ]); + }); + + + it('should respond with 200 (false) if no significant files were touched', async () => { + const expectedResponse = JSON.stringify({ + canHavePublicPreview: false, + reason: 'No significant files touched.', + }); + + await curl({extraPath: `/${PrNums.CHANGED_FILES_NONE}`}).then(h.verifyResponse(200, expectedResponse)); + }); + + + it('should respond with 500 if checking "trusted" status fails', async () => { + await curl({extraPath: `/${PrNums.TRUST_CHECK_ERROR}`}).then(h.verifyResponse(500, 'TRUST_CHECK_ERROR')); + }); + + + it('should respond with 200 (false) if the PR is not automatically verifiable as "trusted"', async () => { + const expectedResponse = JSON.stringify({ + canHavePublicPreview: false, + reason: 'Not automatically verifiable as \\"trusted\\".', + }); + + await Promise.all([ + curl({extraPath: `/${PrNums.TRUST_CHECK_INACTIVE_TRUSTED_USER}`}).then(h.verifyResponse(200, expectedResponse)), + curl({extraPath: `/${PrNums.TRUST_CHECK_UNTRUSTED}`}).then(h.verifyResponse(200, expectedResponse)), + ]); + }); + + + it('should respond with 200 (true) if the PR can have a public preview', async () => { + const expectedResponse = JSON.stringify({ + canHavePublicPreview: true, + reason: null, + }); + + await Promise.all([ + curl({extraPath: `/${PrNums.TRUST_CHECK_ACTIVE_TRUSTED_USER}`}).then(h.verifyResponse(200, expectedResponse)), + curl({extraPath: `/${PrNums.TRUST_CHECK_TRUSTED_LABEL}`}).then(h.verifyResponse(200, expectedResponse)), + ]); + }); + + }); + + describe(`${host}/circle-build`, () => { const curl = makeCurl(`${host}/circle-build`); diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/test/preview-server/preview-server-factory.spec.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/test/preview-server/preview-server-factory.spec.ts index eb8c9aef2a..eb1ed3e314 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/test/preview-server/preview-server-factory.spec.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/test/preview-server/preview-server-factory.spec.ts @@ -39,6 +39,7 @@ describe('PreviewServerFactory', () => { significantFilesPattern: '^(?:aio|packages)\\/(?!.*[._]spec\\.[jt]s$)', trustedPrLabel: 'trusted: pr-label', }; + let loggerErrorSpy: jasmine.Spy; let loggerInfoSpy: jasmine.Spy; let loggerLogSpy: jasmine.Spy; @@ -47,9 +48,9 @@ describe('PreviewServerFactory', () => { PreviewServerFactory.create({...defaultConfig, ...partialConfig}); beforeEach(() => { + loggerErrorSpy = spyOn(Logger.prototype, 'error'); loggerInfoSpy = spyOn(Logger.prototype, 'info'); loggerLogSpy = spyOn(Logger.prototype, 'log'); - spyOn(Logger.prototype, 'error'); }); describe('create()', () => { @@ -296,6 +297,102 @@ describe('PreviewServerFactory', () => { }); + + describe('GET /can-have-public-preview/', () => { + const baseUrl = '/can-have-public-preview'; + const pr = 777; + const url = `${baseUrl}/${pr}`; + let bvGetPrIsTrustedSpy: jasmine.Spy; + let bvGetSignificantFilesChangedSpy: jasmine.Spy; + + beforeEach(() => { + bvGetPrIsTrustedSpy = spyOn(buildVerifier, 'getPrIsTrusted').and.returnValue(Promise.resolve(true)); + bvGetSignificantFilesChangedSpy = spyOn(buildVerifier, 'getSignificantFilesChanged'). + and.returnValue(Promise.resolve(true)); + }); + + + it('should respond with 404 for non-GET requests', async () => { + await Promise.all([ + agent.put(url).expect(404), + agent.post(url).expect(404), + agent.patch(url).expect(404), + agent.delete(url).expect(404), + ]); + }); + + + it('should respond with 404 if the path does not match exactly', async () => { + await Promise.all([ + agent.get('/can-have-public-preview/42/foo').expect(404), + agent.get('/can-have-public-preview-foo/42').expect(404), + agent.get('/can-have-public-previewnfoo/42').expect(404), + agent.get('/foo/can-have-public-preview/42').expect(404), + agent.get('/foo-can-have-public-preview/42').expect(404), + agent.get('/fooncan-have-public-preview/42').expect(404), + ]); + }); + + + it('should respond appropriately if the PR did not touch any significant files', async () => { + bvGetSignificantFilesChangedSpy.and.returnValue(Promise.resolve(false)); + + const expectedResponse = {canHavePublicPreview: false, reason: 'No significant files touched.'}; + const expectedLog = `PR:${pr} - Cannot have a public preview, because it did not touch any significant files.`; + + await agent.get(url).expect(200, expectedResponse); + + expect(bvGetSignificantFilesChangedSpy).toHaveBeenCalledWith(pr, jasmine.any(RegExp)); + expect(bvGetPrIsTrustedSpy).not.toHaveBeenCalled(); + expect(loggerLogSpy).toHaveBeenCalledWith(expectedLog); + }); + + + it('should respond appropriately if the PR is not automatically verifiable as "trusted"', async () => { + bvGetPrIsTrustedSpy.and.returnValue(Promise.resolve(false)); + + const expectedResponse = {canHavePublicPreview: false, reason: 'Not automatically verifiable as "trusted".'}; + const expectedLog = + `PR:${pr} - Cannot have a public preview, because not automatically verifiable as "trusted".`; + + await agent.get(url).expect(200, expectedResponse); + + expect(bvGetSignificantFilesChangedSpy).toHaveBeenCalledWith(pr, jasmine.any(RegExp)); + expect(bvGetPrIsTrustedSpy).toHaveBeenCalledWith(pr); + expect(loggerLogSpy).toHaveBeenCalledWith(expectedLog); + }); + + + it('should respond appropriately if the PR can have a preview', async () => { + const expectedResponse = {canHavePublicPreview: true, reason: null}; + const expectedLog = `PR:${pr} - Can have a public preview.`; + + await agent.get(url).expect(200, expectedResponse); + + expect(bvGetSignificantFilesChangedSpy).toHaveBeenCalledWith(pr, jasmine.any(RegExp)); + expect(bvGetPrIsTrustedSpy).toHaveBeenCalledWith(pr); + expect(loggerLogSpy).toHaveBeenCalledWith(expectedLog); + }); + + + it('should respond with error if `getSignificantFilesChanged()` fails', async () => { + bvGetSignificantFilesChangedSpy.and.callFake(() => Promise.reject('getSignificantFilesChanged error')); + + await agent.get(url).expect(500, 'getSignificantFilesChanged error'); + expect(loggerErrorSpy).toHaveBeenCalledWith('Previewability check error', 'getSignificantFilesChanged error'); + }); + + + it('should respond with error if `getPrIsTrusted()` fails', async () => { + const error = new Error('getPrIsTrusted error'); + bvGetPrIsTrustedSpy.and.callFake(() => { throw error; }); + + await agent.get(url).expect(500, 'getPrIsTrusted error'); + expect(loggerErrorSpy).toHaveBeenCalledWith('Previewability check error', error); + }); + + }); + describe('/circle-build', () => { let getGithubInfoSpy: jasmine.Spy; let getSignificantFilesChangedSpy: jasmine.Spy; 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 f2b0b62502..505b34b0db 100644 --- a/aio/aio-builds-setup/docs/overview--http-status-codes.md +++ b/aio/aio-builds-setup/docs/overview--http-status-codes.md @@ -25,6 +25,23 @@ with a brief explanation of what they mean: File not found. +## `https://ngbuilds.io/can-have-public-preview/` + +- **200 (OK)**: + Whether the PR can have a public preview (based on its author, label, changed files). + _Response type:_ JSON + _Response format:_ + ```ts + { + canHavePublicPreview: boolean, + reason: string | null, + } + ``` + +- **405 (Method Not Allowed)**: + Request method other than GET. + + ## `https://ngbuilds.io/circle-build` - **201 (Created)**: