From 45763e98be7054a4170413e0b5c458825ef255ef Mon Sep 17 00:00:00 2001 From: Felipe Santos Date: Sun, 1 Dec 2024 16:38:36 +0000 Subject: [PATCH] feat(gitlab,azure): try approving before auto-merge --- .../azure/__snapshots__/index.spec.ts.snap | 2 + lib/modules/platform/azure/index.spec.ts | 42 ++++++++++++- lib/modules/platform/azure/index.ts | 61 +++++++++++-------- lib/modules/platform/gitlab/index.spec.ts | 57 +++++++++++++++++ lib/modules/platform/gitlab/index.ts | 37 ++++++++--- lib/modules/platform/gitlab/types.ts | 4 ++ 6 files changed, 168 insertions(+), 35 deletions(-) diff --git a/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap b/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap index e782de620ace85f..5d344c145c65943 100644 --- a/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap +++ b/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap @@ -217,6 +217,8 @@ exports[`modules/platform/azure/index updatePr(prNo, title, body, platformPrOpti ] `; +exports[`modules/platform/azure/index updatePr(prNo, title, body, platformPrOptions) should not re-approve if the PR is already approved 1`] = `undefined`; + exports[`modules/platform/azure/index updatePr(prNo, title, body, platformPrOptions) should re-approve the PR 1`] = `undefined`; exports[`modules/platform/azure/index updatePr(prNo, title, body, platformPrOptions) should reopen the PR 1`] = ` diff --git a/lib/modules/platform/azure/index.spec.ts b/lib/modules/platform/azure/index.spec.ts index 73308cdfbfa98e4..24630611e73c0f5 100644 --- a/lib/modules/platform/azure/index.spec.ts +++ b/lib/modules/platform/azure/index.spec.ts @@ -1149,7 +1149,7 @@ describe('modules/platform/azure/index', () => { const updateFn = jest .fn(() => prUpdateResult) .mockName('createPullRequestReviewer'); - azureApi.gitApi.mockImplementationOnce( + azureApi.gitApi.mockImplementation( () => ({ createPullRequest: jest.fn(() => prResult), @@ -1293,7 +1293,7 @@ describe('modules/platform/azure/index', () => { isRequired: false, }; const updateFn = jest.fn(() => prUpdateResult); - azureApi.gitApi.mockImplementationOnce( + azureApi.gitApi.mockImplementation( () => ({ updatePullRequest: jest.fn(() => prResult), @@ -1313,6 +1313,44 @@ describe('modules/platform/azure/index', () => { expect(updateFn).toHaveBeenCalled(); expect(pr).toMatchSnapshot(); }); + + it('should not re-approve if the PR is already approved', async () => { + await initRepo({ repository: 'some/repo' }); + const prResult = { + pullRequestId: 456, + createdBy: { + id: 123, + url: 'user-url', + }, + }; + const prUpdateResult = { + reviewerUrl: prResult.createdBy.url, + vote: AzurePrVote.Approved, + isFlagged: false, + isRequired: false, + }; + const updateFn = jest.fn(() => prUpdateResult); + azureApi.gitApi.mockImplementation( + () => + ({ + updatePullRequest: jest.fn(() => prResult), + createPullRequestReviewer: updateFn, + getPullRequestById: jest.fn(() => ({ + pullRequestId: prResult.pullRequestId, + createdBy: prResult.createdBy, + reviewers: [{ vote: AzurePrVote.Approved, id: 123 }], + })), + }) as any, + ); + const pr = await azure.updatePr({ + number: prResult.pullRequestId, + prTitle: 'The Title', + prBody: 'Hello world', + platformPrOptions: { autoApprove: true }, + }); + expect(updateFn).not.toHaveBeenCalled(); + expect(pr).toMatchSnapshot(); + }); }); describe('ensureComment', () => { diff --git a/lib/modules/platform/azure/index.ts b/lib/modules/platform/azure/index.ts index 21bd44eaa3f58cc..e585941f94316c5 100644 --- a/lib/modules/platform/azure/index.ts +++ b/lib/modules/platform/azure/index.ts @@ -513,18 +513,7 @@ export async function createPr({ ); } if (platformPrOptions?.autoApprove) { - await azureApiGit.createPullRequestReviewer( - { - reviewerUrl: pr.createdBy!.url, - vote: AzurePrVote.Approved, - isFlagged: false, - isRequired: false, - }, - config.repoId, - // TODO #22198 - pr.pullRequestId!, - pr.createdBy!.id!, - ); + await approvePr(pr); } await Promise.all( labels!.map((label) => @@ -581,19 +570,7 @@ export async function updatePr({ objToUpdate.status = PullRequestStatus.Abandoned; } if (platformPrOptions?.autoApprove) { - const pr = await azureApiGit.getPullRequestById(prNo, config.project); - await azureApiGit.createPullRequestReviewer( - { - reviewerUrl: pr.createdBy!.url, - vote: AzurePrVote.Approved, - isFlagged: false, - isRequired: false, - }, - config.repoId, - // TODO #22198 - pr.pullRequestId!, - pr.createdBy!.id!, - ); + await approvePr(prNo); } const updatedPr = await azureApiGit.updatePullRequest( @@ -1015,3 +992,37 @@ export async function deleteLabel( const azureApiGit = await azureApi.gitApi(); await azureApiGit.deletePullRequestLabels(config.repoId, prNumber, label); } + +export async function approvePr( + prNumberOrPr: number | GitPullRequest, +): Promise { + const azureApiGit = await azureApi.gitApi(); + const pr = + typeof prNumberOrPr === 'number' + ? await azureApiGit.getPullRequestById(prNumberOrPr, config.project) + : prNumberOrPr; + + if ( + pr.reviewers?.some( + (reviewer) => + reviewer.vote === AzurePrVote.Approved && + reviewer.id === pr.createdBy?.id, + ) + ) { + logger.debug('PR is already approved'); + return; + } + + await azureApiGit.createPullRequestReviewer( + { + reviewerUrl: pr.createdBy!.url, + vote: AzurePrVote.Approved, + isFlagged: false, + isRequired: false, + }, + config.repoId, + // TODO #22198 + pr.pullRequestId!, + pr.createdBy!.id!, + ); +} diff --git a/lib/modules/platform/gitlab/index.spec.ts b/lib/modules/platform/gitlab/index.spec.ts index 6eb62b886f7c406..b4d747857534527 100644 --- a/lib/modules/platform/gitlab/index.spec.ts +++ b/lib/modules/platform/gitlab/index.spec.ts @@ -1900,6 +1900,7 @@ describe('modules/platform/gitlab/index', () => { .get('/api/v4/user') .reply(200, { email: 'a@b.com', + id: 1, name: 'Renovate Bot', }) .get('/api/v4/version') @@ -2778,6 +2779,10 @@ describe('modules/platform/gitlab/index', () => { target_branch: 'master', description: 'the-body', }) + .get('/api/v4/projects/undefined/merge_requests/12345/approvals') + .reply(200, { + approved_by: [], + }) .post('/api/v4/projects/undefined/merge_requests/12345/approve') .reply(200); expect( @@ -2811,6 +2816,8 @@ describe('modules/platform/gitlab/index', () => { target_branch: 'master', description: 'the-body', }) + .get('/api/v4/projects/undefined/merge_requests/12345/approvals') + .replyWithError('some error') .post('/api/v4/projects/undefined/merge_requests/12345/approve') .replyWithError('some error'); await expect( @@ -3139,6 +3146,10 @@ describe('modules/platform/gitlab/index', () => { description: 'body', state: 'opened', }) + .get('/api/v4/projects/undefined/merge_requests/1/approvals') + .reply(200, { + approved_by: [], + }) .post('/api/v4/projects/undefined/merge_requests/1/approve') .reply(200); await expect( @@ -3153,6 +3164,52 @@ describe('modules/platform/gitlab/index', () => { ).toResolve(); }); + it('does not auto-approve if already approved', async () => { + await initPlatform('13.3.6-ee'); + httpMock + .scope(gitlabApiHost) + .get( + '/api/v4/projects/undefined/merge_requests?per_page=100&scope=created_by_me', + ) + .reply(200, [ + { + iid: 1, + source_branch: 'branch-a', + title: 'branch a pr', + description: 'a merge request', + state: 'opened', + }, + ]) + .put('/api/v4/projects/undefined/merge_requests/1') + .reply(200, { + iid: 1, + source_branch: 'branch-a', + title: 'title', + description: 'body', + state: 'opened', + }) + .get('/api/v4/projects/undefined/merge_requests/1/approvals') + .reply(200, { + approved_by: [ + { + user: { + id: 1, + }, + }, + ], + }); + await expect( + gitlab.updatePr({ + number: 1, + prTitle: 'title', + prBody: 'body', + platformPrOptions: { + autoApprove: true, + }, + }), + ).toResolve(); + }); + it('closes the PR', async () => { await initPlatform('13.3.6-ee'); httpMock diff --git a/lib/modules/platform/gitlab/index.ts b/lib/modules/platform/gitlab/index.ts index 30965b963085058..d32a8ef5bbe5625 100644 --- a/lib/modules/platform/gitlab/index.ts +++ b/lib/modules/platform/gitlab/index.ts @@ -65,6 +65,7 @@ import { getMR, updateMR } from './merge-request'; import { LastPipelineId } from './schema'; import type { GitLabMergeRequest, + GitLabMergeRequestApprovals, GitlabComment, GitlabIssue, GitlabPr, @@ -94,6 +95,7 @@ const defaults = { export const id = 'gitlab'; let draftPrefix = DRAFT_PREFIX; +let renovateUserId: number; export async function initPlatform({ endpoint, @@ -114,19 +116,22 @@ export async function initPlatform({ }; let gitlabVersion: string; try { + const user = ( + await gitlabApi.getJson<{ + email: string; + name: string; + id: number; + commit_email?: string; + }>(`user`, { token }) + ).body; + + renovateUserId = user.id; if (!gitAuthor) { - const user = ( - await gitlabApi.getJson<{ - email: string; - name: string; - id: number; - commit_email?: string; - }>(`user`, { token }) - ).body; platformConfig.gitAuthor = `${user.name} <${ user.commit_email ?? user.email }>`; } + // istanbul ignore if: experimental feature if (process.env.RENOVATE_X_PLATFORM_VERSION) { gitlabVersion = process.env.RENOVATE_X_PLATFORM_VERSION; @@ -711,6 +716,22 @@ async function tryPrAutomerge( } async function approvePr(pr: number): Promise { + try { + const { body: approvals } = + await gitlabApi.getJson( + `projects/${config.repository}/merge_requests/${pr}/approvals`, + ); + if ( + approvals.approved_by?.some( + (approval) => approval.user?.id === renovateUserId, + ) + ) { + logger.debug('MR already approved'); + return; + } + } catch (err) { + logger.warn({ err }, 'GitLab: Error checking if merge request is approved'); + } try { await gitlabApi.postJson( `projects/${config.repository}/merge_requests/${pr}/approve`, diff --git a/lib/modules/platform/gitlab/types.ts b/lib/modules/platform/gitlab/types.ts index 244ec8366361e6b..c37243b8a44bec5 100644 --- a/lib/modules/platform/gitlab/types.ts +++ b/lib/modules/platform/gitlab/types.ts @@ -77,3 +77,7 @@ export interface GitlabUserStatus { emoji?: string; availability: 'not_set' | 'busy'; } + +export interface GitLabMergeRequestApprovals { + approved_by?: { user?: GitLabUser }[]; +}