From aea7b98a563c0d03d48b352ddd175b904bba98a9 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Wed, 11 May 2022 01:04:50 +0300 Subject: [PATCH] fix(docs-infra): do not detect a running job as failed (#45948) Previously, the preview server would incorrectly identify a running `aio_preview` CI job as failed and therefore skip creating a preview. This happened because it only checked whether the job's status is `success`, failing to account for the fact that the job would have a `running` status. (This bug was accidentally introduced in #45934.) This commit avoids the problem by getting rid of the job status check altogether. This check does not offer any benefit, since the CI job will always be in a `running` state (i.e. neither successfully completed nor failed). PR Close #45948 --- .../dockerbuild/scripts-js/lib/common/circle-ci-api.ts | 3 --- .../scripts-js/lib/preview-server/build-retriever.ts | 2 -- .../lib/preview-server/preview-server-factory.ts | 8 +------- .../scripts-js/lib/verify-setup/constants.ts | 1 - .../scripts-js/lib/verify-setup/mock-external-apis.ts | 2 -- .../scripts-js/lib/verify-setup/preview-server.e2e.ts | 4 ---- .../test/preview-server/build-retriever.spec.ts | 3 +-- .../test/preview-server/preview-server-factory.spec.ts | 10 ---------- 8 files changed, 2 insertions(+), 31 deletions(-) diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/circle-ci-api.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/circle-ci-api.ts index efc8a5f1301..860795e1b68 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/circle-ci-api.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/circle-ci-api.ts @@ -33,9 +33,6 @@ export interface BuildInfo { /** The job name (e.g. `'aio_preview'`). */ name: string; - /** The job status (e.g. `'success'` or `'failed'`). */ - status: string; - /** Info about the organization which the project related to this job belongs to. */ organization: { name: string; diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/preview-server/build-retriever.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/preview-server/build-retriever.ts index 19c154dbce6..9f187104288 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/preview-server/build-retriever.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/preview-server/build-retriever.ts @@ -13,7 +13,6 @@ export interface GithubInfo { pr: number; repo: string; sha: string; - success: boolean; } /** @@ -42,7 +41,6 @@ export class BuildRetriever { pr: +pipelineInfo.vcs.review_id, repo: buildInfo.project.name, sha: pipelineInfo.vcs.revision, - success: buildInfo.status === 'success', }; return githubInfo; } 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 8058a64597b..24175a61b6a 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 @@ -120,13 +120,7 @@ export class PreviewServerFactory { return; } - const { pr, sha, org, repo, success } = await buildRetriever.getGithubInfo(buildNum); - - if (!success) { - res.sendStatus(204); - logger.log(`PR:${pr}, Build:${buildNum} - Skipping preview processing because this build did not succeed.`); - return; - } + const { pr, sha, org, repo } = await buildRetriever.getGithubInfo(buildNum); assert(cfg.githubOrg === org, `Invalid webhook: expected "githubOrg" property to equal "${cfg.githubOrg}" but got "${org}".`); 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 dfa47863761..c0a528044d2 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 @@ -1,7 +1,6 @@ export const enum BuildNums { BUILD_INFO_ERROR = 1, BUILD_INFO_404, - BUILD_INFO_BUILD_FAILED, BUILD_INFO_INVALID_GH_ORG, BUILD_INFO_INVALID_GH_REPO, PIPELINE_INFO_ERROR, diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/mock-external-apis.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/mock-external-apis.ts index 9ff9ac312cb..4481f1ce2c8 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/mock-external-apis.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/mock-external-apis.ts @@ -31,7 +31,6 @@ const UNTRUSTED_USER = 'untrusted-user'; const BASIC_BUILD_INFO = { name: 'test_job', - status: 'success', organization: {name: AIO_GITHUB_ORGANIZATION}, project: {name: AIO_GITHUB_REPO}, pipeline: {id: PipelineIds.PIPELINE_INFO_OK}, @@ -114,7 +113,6 @@ circleCiApi.get(pipelineInfoUrl(PipelineIds.PIPELINE_INFO_OK)).reply(200, BASIC_ // BUILD INFO errors circleCiApi.get(buildInfoUrl(BuildNums.BUILD_INFO_ERROR)).replyWithError('BUILD_INFO_ERROR'); circleCiApi.get(buildInfoUrl(BuildNums.BUILD_INFO_404)).reply(404, 'BUILD_INFO_404'); -circleCiApi.get(buildInfoUrl(BuildNums.BUILD_INFO_BUILD_FAILED)).reply(200, { ...BASIC_BUILD_INFO, status: 'failed' }); circleCiApi.get(buildInfoUrl(BuildNums.BUILD_INFO_INVALID_GH_ORG)).reply(200, { ...BASIC_BUILD_INFO, organization: { name: 'bad' } }); circleCiApi.get(buildInfoUrl(BuildNums.BUILD_INFO_INVALID_GH_REPO)).reply(200, { ...BASIC_BUILD_INFO, project: { name: 'bad' } }); 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 148dec5893f..7237707cc30 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 @@ -152,10 +152,6 @@ describe('preview-server', () => { await curl(payload(BuildNums.PIPELINE_INFO_404)).then(h.verifyResponse(500)); }); - it('should respond with 204 if the build on CircleCI failed', async () => { - await curl(payload(BuildNums.BUILD_INFO_BUILD_FAILED)).then(h.verifyResponse(204)); - }); - it('should respond with 500 if the github org from CircleCI does not match what is configured', async () => { await curl(payload(BuildNums.BUILD_INFO_INVALID_GH_ORG)).then(h.verifyResponse(500)); }); diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/test/preview-server/build-retriever.spec.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/test/preview-server/build-retriever.spec.ts index 9377abb9d4f..d980ea876ea 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/test/preview-server/build-retriever.spec.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/test/preview-server/build-retriever.spec.ts @@ -24,7 +24,6 @@ describe('BuildRetriever', () => { BUILD_INFO = { number: 12345, name: 'test_job', - status: 'success', organization: {name: 'ORG'}, project: {name: 'REPO'}, pipeline: {id: 'test_pipeline'}, @@ -77,7 +76,7 @@ describe('BuildRetriever', () => { const info = await retriever.getGithubInfo(12345); expect(api.getBuildInfo).toHaveBeenCalledWith(12345); expect(api.getPipelineInfo).toHaveBeenCalledWith('test_pipeline'); - expect(info).toEqual({org: 'ORG', pr: 777, repo: 'REPO', sha: 'COMMIT', success: true}); + expect(info).toEqual({org: 'ORG', pr: 777, repo: 'REPO', sha: 'COMMIT'}); }); }); 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 1db3aa2685a..d479653134b 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 @@ -410,7 +410,6 @@ describe('PreviewServerFactory', () => { pr: PR, repo: defaultConfig.githubRepo, sha: SHA, - success: true, }; BASIC_PAYLOAD = { payload: { build_num: BUILD_NUM, build_parameters: { CIRCLE_JOB: 'aio_preview' } } }; AFFECTS_SIGNIFICANT_FILES = true; @@ -481,15 +480,6 @@ describe('PreviewServerFactory', () => { await agent.post(URL).send(BASIC_PAYLOAD).expect(202); }); - it('should not create a preview if the build was not successful', async () => { - BUILD_INFO.success = false; - await agent.post(URL).send(BASIC_PAYLOAD).expect(204); - expect(getGithubInfoSpy).toHaveBeenCalledWith(BUILD_NUM); - expect(downloadBuildArtifactSpy).not.toHaveBeenCalled(); - expect(getPrIsTrustedSpy).not.toHaveBeenCalled(); - expect(createBuildSpy).not.toHaveBeenCalled(); - }); - it('should fail if the CircleCI request fails', async () => { // Note it is important to put the `reject` into `and.callFake`; // If you just `and.returnValue` the rejected promise