Skip to content

Commit

Permalink
feat(gitlab,azure): try approving before auto-merge
Browse files Browse the repository at this point in the history
  • Loading branch information
@felipecrs
felipecrs committed Dec 1, 2024
1 parent 1d8daf5 commit 45763e9
Show file tree
Hide file tree
Showing 6 changed files with 168 additions and 35 deletions.
2 changes: 2 additions & 0 deletions lib/modules/platform/azure/__snapshots__/index.spec.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`] = `
Expand Down
42 changes: 40 additions & 2 deletions lib/modules/platform/azure/index.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand All @@ -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', () => {
Expand Down
61 changes: 36 additions & 25 deletions lib/modules/platform/azure/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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<void> {
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!,
);
}
57 changes: 57 additions & 0 deletions lib/modules/platform/gitlab/index.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1900,6 +1900,7 @@ describe('modules/platform/gitlab/index', () => {
.get('/api/v4/user')
.reply(200, {
email: '[email protected]',
id: 1,
name: 'Renovate Bot',
})
.get('/api/v4/version')
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand Down
37 changes: 29 additions & 8 deletions lib/modules/platform/gitlab/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import { getMR, updateMR } from './merge-request';
import { LastPipelineId } from './schema';
import type {
GitLabMergeRequest,
GitLabMergeRequestApprovals,
GitlabComment,
GitlabIssue,
GitlabPr,
Expand Down Expand Up @@ -94,6 +95,7 @@ const defaults = {
export const id = 'gitlab';

let draftPrefix = DRAFT_PREFIX;
let renovateUserId: number;

export async function initPlatform({
endpoint,
Expand All @@ -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;
Expand Down Expand Up @@ -711,6 +716,22 @@ async function tryPrAutomerge(
}

async function approvePr(pr: number): Promise<void> {
try {
const { body: approvals } =
await gitlabApi.getJson<GitLabMergeRequestApprovals>(
`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`,
Expand Down
4 changes: 4 additions & 0 deletions lib/modules/platform/gitlab/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,7 @@ export interface GitlabUserStatus {
emoji?: string;
availability: 'not_set' | 'busy';
}

export interface GitLabMergeRequestApprovals {
approved_by?: { user?: GitLabUser }[];
}

0 comments on commit 45763e9

Please sign in to comment.