From 1f3c1dc8205fae3fe260d0211f9c7fb77bfdea60 Mon Sep 17 00:00:00 2001 From: Vincent Hardouin Date: Wed, 8 Jan 2025 17:00:58 +0100 Subject: [PATCH] feat: send status when unmanage pr --- build/controllers/github.js | 10 +++--- build/controllers/merge.js | 8 +++-- test/acceptance/build/github_test.js | 28 ++++++++++------ test/acceptance/build/merge_test.js | 10 +++++- test/unit/build/controllers/github_test.js | 38 +++++++++++++++++++++- test/unit/build/controllers/merge_test.js | 2 ++ 6 files changed, 78 insertions(+), 18 deletions(-) diff --git a/build/controllers/github.js b/build/controllers/github.js index e61ac2ec..311b9d6e 100644 --- a/build/controllers/github.js +++ b/build/controllers/github.js @@ -7,7 +7,7 @@ import { logger } from '../../common/services/logger.js'; import ScalingoClient from '../../common/services/scalingo-client.js'; import { config } from '../../config.js'; import * as reviewAppRepo from '../repositories/review-app-repository.js'; -import { mergeQueue as _mergeQueue } from '../services/merge-queue.js'; +import { MERGE_STATUS, mergeQueue as _mergeQueue } from '../services/merge-queue.js'; const repositoryToScalingoAppsReview = { 'pix-api-data': ['pix-api-data-integration'], @@ -260,7 +260,9 @@ async function processWebhook( } if (request.payload.action === 'closed') { const repositoryName = request.payload.repository.full_name; - await mergeQueue.unmanagePullRequest({ repositoryName, number: request.payload.number }); + const isMerged = request.payload.pull_request.merged; + const status = isMerged ? MERGE_STATUS.MERGED : MERGE_STATUS.ABORTED; + await mergeQueue.unmanagePullRequest({ repositoryName, number: request.payload.number, status }); return handleCloseRA(request); } if (request.payload.action === 'labeled' && request.payload.label.name == 'no-review-app') { @@ -279,7 +281,7 @@ async function processWebhook( } if (request.payload.action === 'unlabeled' && request.payload.label.name === ':rocket: Ready to Merge') { const repositoryName = request.payload.repository.full_name; - await mergeQueue.unmanagePullRequest({ repositoryName, number: request.payload.number }); + await mergeQueue.unmanagePullRequest({ repositoryName, number: request.payload.number, status: MERGE_STATUS.ABORTED }); } return `Ignoring ${request.payload.action} action`; } else if (eventName === 'check_suite') { @@ -292,7 +294,7 @@ async function processWebhook( const prNumber = request.payload.check_suite.pull_requests[0].number; if (request.payload.check_suite.conclusion !== 'success') { - await mergeQueue.unmanagePullRequest({ repositoryName, number: prNumber }); + await mergeQueue.unmanagePullRequest({ repositoryName, number: prNumber, status: MERGE_STATUS.ABORTED }); } else { const hasReadyToMergeLabel = await githubService.isPrLabelledWith({ repositoryName, diff --git a/build/controllers/merge.js b/build/controllers/merge.js index 5c389bb5..587f0c71 100644 --- a/build/controllers/merge.js +++ b/build/controllers/merge.js @@ -1,4 +1,4 @@ -import { mergeQueue } from '../services/merge-queue.js'; +import { MERGE_STATUS, mergeQueue } from '../services/merge-queue.js'; import { config } from '../../config.js'; import Boom from '@hapi/boom'; @@ -12,7 +12,11 @@ const mergeController = { const { pullRequest } = request.payload; const [organisation, repository, pullRequestNumber] = pullRequest.split('/'); const repositoryName = `${organisation}/${repository}`; - await dependencies.mergeQueue.unmanagePullRequest({ repositoryName, number: Number(pullRequestNumber) }); + await dependencies.mergeQueue.unmanagePullRequest({ + repositoryName, + number: Number(pullRequestNumber), + status: MERGE_STATUS.ERROR, + }); return h.response().code(204); }, }; diff --git a/test/acceptance/build/github_test.js b/test/acceptance/build/github_test.js index 7e5cfe4e..7b58a22e 100644 --- a/test/acceptance/build/github_test.js +++ b/test/acceptance/build/github_test.js @@ -66,6 +66,17 @@ describe('Acceptance | Build | Github', function () { .reply(returnCode); } + function getMergeCheckStatusNock({ repositoryFullName }) { + const prHeadCommit = 'sha1'; + nock('https://api.github.com') + .get(`/repos/1024pix/pix/pulls/123`) + .reply(200, { head: { sha: prHeadCommit } }); + + return nock('https://api.github.com') + .post(`/repos/${repositoryFullName}/statuses/${prHeadCommit}`) + .reply(201, { started_at: '2024-01-01' }); + } + let body; ['opened', 'reopened'].forEach((action) => { @@ -556,9 +567,12 @@ describe('Acceptance | Build | Github', function () { fork: false, }, }, + merged: false, }, }; + getMergeCheckStatusNock({ repositoryFullName: body.repository.full_name }); + // when const response = await server.inject({ method: 'POST', @@ -682,14 +696,7 @@ describe('Acceptance | Build | Github', function () { }) .reply(200, {}); - const prHeadCommit = 'aaaa'; - const getPullRequestNock = nock('https://api.github.com') - .get(`/repos/1024pix/pix/pulls/123`) - .reply(200, { head: { sha: prHeadCommit } }); - - const createCheckNock = nock('https://api.github.com') - .post(`/repos/${body.repository.full_name}/statuses/${prHeadCommit}`) - .reply(201, { started_at: '2024-01-01' }); + const mergeCheckNock = getMergeCheckStatusNock({ repositoryFullName: body.repository.full_name }); const response = await server.inject({ method: 'POST', @@ -703,8 +710,7 @@ describe('Acceptance | Build | Github', function () { expect(checkUserBelongsToPixNock.isDone()).to.be.true; expect(callGitHubAction.isDone()).to.be.true; - expect(getPullRequestNock.isDone()).to.be.true; - expect(createCheckNock.isDone()).to.be.true; + expect(mergeCheckNock.isDone()).to.be.true; expect(response.statusCode).equal(200); }); @@ -1059,6 +1065,7 @@ describe('Acceptance | Build | Github', function () { ], }, }; + getMergeCheckStatusNock({ repositoryFullName: body.repository.full_name }); const response = await server.inject({ method: 'POST', @@ -1070,6 +1077,7 @@ describe('Acceptance | Build | Github', function () { payload: body, }); + expect(response.statusCode).equal(200); expect(response.payload).equal('check_suite event handle'); }); diff --git a/test/acceptance/build/merge_test.js b/test/acceptance/build/merge_test.js index db33adab..6948a6f8 100644 --- a/test/acceptance/build/merge_test.js +++ b/test/acceptance/build/merge_test.js @@ -1,5 +1,5 @@ import server from '../../../server.js'; -import { expect } from '../../test-helper.js'; +import { expect, nock } from '../../test-helper.js'; import { knex } from '../../../db/knex-database-connection.js'; import { describe } from 'mocha'; import { config } from '../../../config.js'; @@ -28,6 +28,14 @@ describe('Acceptance | Build | Merge', function () { it('responds with 200 and remove PR in database', async function () { await knex('pull_requests').insert({ repositoryName: '1024pix/pix-test', number: 1 }); const body = { pullRequest: '1024pix/pix-test/1' }; + const prHeadCommit = 'sha1'; + nock('https://api.github.com') + .get(`/repos/1024pix/pix-test/pulls/1`) + .reply(200, { head: { sha: prHeadCommit } }); + + return nock('https://api.github.com') + .post(`/repos/1024pix/pix-test/statuses/${prHeadCommit}`) + .reply(201, { started_at: '2024-01-01' }); const res = await server.inject({ method: 'POST', diff --git a/test/unit/build/controllers/github_test.js b/test/unit/build/controllers/github_test.js index 23f4455e..e92d9f75 100644 --- a/test/unit/build/controllers/github_test.js +++ b/test/unit/build/controllers/github_test.js @@ -5,6 +5,7 @@ import url from 'node:url'; import * as githubController from '../../../../build/controllers/github.js'; import { config } from '../../../../config.js'; import { catchErr, expect, sinon } from '../../../test-helper.js'; +import { MERGE_STATUS } from '../../../../build/services/merge-queue.js'; describe('Unit | Controller | Github', function () { describe('#getMessageTemplate', function () { @@ -262,12 +263,13 @@ Les variables d'environnement seront accessibles sur scalingo https://dashboard. expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({ number: 123, repositoryName: repositoryName, + status: MERGE_STATUS.ABORTED, }); }); }); }); - describe('when action is `closed`', function () { + describe('when action is `closed` and pr is not merged', function () { it('should call pullRequestRepository.remove() method', async function () { // given const repositoryName = '1024pix/pix-sample-repo'; @@ -275,6 +277,38 @@ Les variables d'environnement seront accessibles sur scalingo https://dashboard. action: 'closed', number: 123, repository: { full_name: repositoryName }, + pull_request: { + merged: false, + }, + }); + + const handleCloseRA = sinon.stub(); + const mergeQueue = { unmanagePullRequest: sinon.stub() }; + + // when + await githubController.processWebhook(request, { handleCloseRA, mergeQueue }); + + // then + expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({ + number: 123, + repositoryName, + status: MERGE_STATUS.ABORTED, + }); + expect(handleCloseRA.calledOnceWithExactly(request)).to.be.true; + }); + }); + + describe('when action is `closed` and pr is merged', function () { + it('should call pullRequestRepository.remove() method', async function () { + // given + const repositoryName = '1024pix/pix-sample-repo'; + sinon.stub(request, 'payload').value({ + action: 'closed', + number: 123, + repository: { full_name: repositoryName }, + pull_request: { + merged: true, + }, }); const handleCloseRA = sinon.stub(); @@ -287,6 +321,7 @@ Les variables d'environnement seront accessibles sur scalingo https://dashboard. expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({ number: 123, repositoryName, + status: MERGE_STATUS.MERGED, }); expect(handleCloseRA.calledOnceWithExactly(request)).to.be.true; }); @@ -328,6 +363,7 @@ Les variables d'environnement seront accessibles sur scalingo https://dashboard. expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({ number: 123, repositoryName, + status: MERGE_STATUS.ABORTED, }); }); }); diff --git a/test/unit/build/controllers/merge_test.js b/test/unit/build/controllers/merge_test.js index 5b70e04d..0321e210 100644 --- a/test/unit/build/controllers/merge_test.js +++ b/test/unit/build/controllers/merge_test.js @@ -1,6 +1,7 @@ import { catchErr, expect, sinon } from '../../../test-helper.js'; import mergeController from '../../../../build/controllers/merge.js'; import { config } from '../../../../config.js'; +import { MERGE_STATUS } from '../../../../build/services/merge-queue.js'; describe('Unit | Controller | Merge', function () { describe('#handle', function () { @@ -39,6 +40,7 @@ describe('Unit | Controller | Merge', function () { expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({ number: 123, repositoryName: '1024pix/pix', + status: MERGE_STATUS.ERROR, }); expect(hStub.response).to.be.calledOnce;