diff --git a/build/controllers/github.js b/build/controllers/github.js index e61ac2ec..f559e434 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,11 @@ 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 +298,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/build/services/merge-queue.js b/build/services/merge-queue.js index 5d27a09d..858ae702 100644 --- a/build/services/merge-queue.js +++ b/build/services/merge-queue.js @@ -3,6 +3,18 @@ import { config } from '../../config.js'; import * as pullRequestRepository from '../repositories/pull-request-repository.js'; import githubService from '../../common/services/github.js'; +export const MERGE_STATUS = { + ABORTED: 'ABORTED', + ERROR: 'ERROR', + MERGED: 'MERGED', +}; + +export const MERGE_STATUS_DETAILS = { + ABORTED: { description: 'Merge plus géré', checkStatus: 'success' }, + ERROR: { description: 'Error, merci de visiter la liste des essais', checkStatus: 'error' }, + MERGED: { description: '', checkStatus: 'success' }, +}; + export class MergeQueue { #pullRequestRepository; #githubService; @@ -64,7 +76,15 @@ export class MergeQueue { await this.manage({ repositoryName }); } - async unmanagePullRequest({ repositoryName, number }) { + async unmanagePullRequest({ repositoryName, number, status }) { + const statusDetails = MERGE_STATUS_DETAILS[status]; + await this.#githubService.setMergeQueueStatus({ + status: statusDetails.checkStatus, + description: statusDetails.description, + repositoryFullName: repositoryName, + prNumber: number, + }); + await this.#pullRequestRepository.remove({ repositoryName, number, diff --git a/common/services/github.js b/common/services/github.js index df8fdda1..b8d9368c 100644 --- a/common/services/github.js +++ b/common/services/github.js @@ -359,7 +359,7 @@ async function _getPullRequestsDetailsByCommitShas({ repoOwner, repoName, commit return pullRequestsForCommitShaFilteredDetails; } -async function createCommitStatus({ context, repo, pull_number, description, state }) { +async function createCommitStatus({ context, repo, pull_number, description, state, target_url }) { const owner = '1024pix'; const octokit = _createOctokit(); @@ -373,6 +373,7 @@ async function createCommitStatus({ context, repo, pull_number, description, sta repo, sha, state, + target_url, }); const startedAt = response.data.started_at; @@ -389,6 +390,7 @@ async function setMergeQueueStatus({ repositoryFullName, prNumber, status, descr pull_number: prNumber, state: status, description, + target_url: 'https://github.com/1024pix/pix-actions/actions/workflows/auto-merge-dispatch.yml', }); } diff --git a/test/acceptance/build/github_test.js b/test/acceptance/build/github_test.js index 7e5cfe4e..5652c06e 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', diff --git a/test/acceptance/build/merge_test.js b/test/acceptance/build/merge_test.js index db33adab..565cc1ee 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 } }); + + 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; diff --git a/test/unit/build/services/github_test.js b/test/unit/build/services/github_test.js index c88545e6..5f1b63fc 100644 --- a/test/unit/build/services/github_test.js +++ b/test/unit/build/services/github_test.js @@ -621,6 +621,7 @@ describe('Unit | Build | github-test', function () { context: 'merge-queue-status', state: 'pending', description: 'fuu', + target_url: 'https://github.com/1024pix/pix-actions/actions/workflows/auto-merge-dispatch.yml', }; const createCheckNock = nock('https://api.github.com') diff --git a/test/unit/run/services/merge-queue_test.js b/test/unit/run/services/merge-queue_test.js index 207ebf78..98c30c14 100644 --- a/test/unit/run/services/merge-queue_test.js +++ b/test/unit/run/services/merge-queue_test.js @@ -1,5 +1,5 @@ import { expect, sinon } from '../../../test-helper.js'; -import { MergeQueue } from '../../../../build/services/merge-queue.js'; +import { MERGE_STATUS, MERGE_STATUS_DETAILS, MergeQueue } from '../../../../build/services/merge-queue.js'; import { config } from '../../../../config.js'; describe('Unit | Build | Services | merge-queue', function () { @@ -133,17 +133,65 @@ describe('Unit | Build | Services | merge-queue', function () { const pullRequestRepository = { remove: sinon.stub(), }; + const githubService = { + setMergeQueueStatus: sinon.stub(), + }; - const mergeQueue = new MergeQueue({ pullRequestRepository }); + const mergeQueue = new MergeQueue({ pullRequestRepository, githubService }); mergeQueue.manage = sinon.stub(); - await mergeQueue.unmanagePullRequest({ repositoryName, number: pullRequestNumber }); + await mergeQueue.unmanagePullRequest({ repositoryName, number: pullRequestNumber, status: MERGE_STATUS.ERROR }); expect(pullRequestRepository.remove).to.have.been.calledOnceWithExactly({ repositoryName, number: pullRequestNumber, }); expect(mergeQueue.manage).to.have.been.calledOnceWithExactly({ repositoryName }); + expect(githubService.setMergeQueueStatus).to.have.been.calledOnce; + }); + + const testCases = [ + { + status: MERGE_STATUS.MERGED, + checkDescription: MERGE_STATUS_DETAILS.MERGED.description, + checkStatus: 'success', + }, + { status: MERGE_STATUS.ERROR, checkDescription: MERGE_STATUS_DETAILS.ERROR.description, checkStatus: 'error' }, + { + status: MERGE_STATUS.ABORTED, + checkDescription: MERGE_STATUS_DETAILS.ABORTED.description, + checkStatus: 'success', + }, + ]; + + testCases.forEach((testCase) => { + it(`should update check status when pr status is '${testCase.status}'`, async function () { + const repositoryName = Symbol('repository-name'); + const pullRequestNumber = Symbol('pull-request-number'); + + const pullRequestRepository = { + remove: sinon.stub(), + }; + const githubService = { + setMergeQueueStatus: sinon.stub(), + }; + + const mergeQueue = new MergeQueue({ pullRequestRepository, githubService }); + mergeQueue.manage = sinon.stub(); + + await mergeQueue.unmanagePullRequest({ + repositoryName, + number: pullRequestNumber, + status: testCase.status, + }); + + expect(githubService.setMergeQueueStatus).to.have.been.calledOnceWithExactly({ + status: testCase.checkStatus, + repositoryFullName: repositoryName, + prNumber: pullRequestNumber, + description: testCase.checkDescription, + }); + }); }); }); });