From 46f8eed01fc5e7a822e2855720612f3e165646c2 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Fri, 29 Nov 2024 19:42:54 -0300
Subject: [PATCH 01/18] fix(gerrit): not auto-approving if change already had a
Code-Review +1
---
lib/modules/platform/gerrit/client.spec.ts | 89 ++++++++++++++++++----
lib/modules/platform/gerrit/client.ts | 32 +++++---
lib/modules/platform/gerrit/scm.spec.ts | 1 +
lib/modules/platform/gerrit/scm.ts | 2 +-
lib/modules/platform/gerrit/types.ts | 7 ++
5 files changed, 103 insertions(+), 28 deletions(-)
diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts
index 2d5ff5cddbf9fe..72073d96bb1912 100644
--- a/lib/modules/platform/gerrit/client.spec.ts
+++ b/lib/modules/platform/gerrit/client.spec.ts
@@ -393,10 +393,20 @@ describe('modules/platform/gerrit/client', () => {
describe('approveChange()', () => {
it('already approved - do nothing', async () => {
- const change = partial<GerritChange>({});
+ const change = partial<GerritChange>({
+ labels: {
+ 'Code-Review': {
+ all: [{ value: 2 }],
+ },
+ },
+ });
httpMock
.scope(gerritEndpointUrl)
- .get((url) => url.includes('/a/changes/123456?o='))
+ .get(
+ (url) =>
+ url.includes('/a/changes/123456?o=') &&
+ url.includes('o=DETAILED_LABELS'),
+ )
.reply(200, gerritRestResponse(change), jsonResultHeader);
await expect(client.approveChange(123456)).toResolve();
});
@@ -405,17 +415,53 @@ describe('modules/platform/gerrit/client', () => {
const change = partial<GerritChange>({ labels: {} });
httpMock
.scope(gerritEndpointUrl)
- .get((url) => url.includes('/a/changes/123456?o='))
+ .get(
+ (url) =>
+ url.includes('/a/changes/123456?o=') &&
+ url.includes('o=DETAILED_LABELS'),
+ )
.reply(200, gerritRestResponse(change), jsonResultHeader);
await expect(client.approveChange(123456)).toResolve();
});
it('not already approved - approve now', async () => {
- const change = partial<GerritChange>({ labels: { 'Code-Review': {} } });
+ const change = partial<GerritChange>({
+ labels: { 'Code-Review': { all: [] } },
+ });
+ httpMock
+ .scope(gerritEndpointUrl)
+ .get(
+ (url) =>
+ url.includes('/a/changes/123456?o=') &&
+ url.includes('o=DETAILED_LABELS'),
+ )
+ .reply(200, gerritRestResponse(change), jsonResultHeader);
+ const approveMock = httpMock
+ .scope(gerritEndpointUrl)
+ .post('/a/changes/123456/revisions/current/review', {
+ labels: { 'Code-Review': +2 },
+ })
+ .reply(200, gerritRestResponse(''), jsonResultHeader);
+ await expect(client.approveChange(123456)).toResolve();
+ expect(approveMock.isDone()).toBeTrue();
+ });
+
+ it('not already approved because of +1 - approve now', async () => {
+ const change = partial<GerritChange>({
+ labels: {
+ 'Code-Review': {
+ all: [{ value: 1 }],
+ },
+ },
+ });
httpMock
.scope(gerritEndpointUrl)
- .get((url) => url.includes('/a/changes/123456?o='))
+ .get(
+ (url) =>
+ url.includes('/a/changes/123456?o=') &&
+ url.includes('o=DETAILED_LABELS'),
+ )
.reply(200, gerritRestResponse(change), jsonResultHeader);
const approveMock = httpMock
.scope(gerritEndpointUrl)
@@ -432,7 +478,7 @@ describe('modules/platform/gerrit/client', () => {
it('label not exists', () => {
expect(
client.wasApprovedBy(partial<GerritChange>({}), 'user'),
- ).toBeUndefined();
+ ).toBeFalse();
});
it('not approved by anyone', () => {
@@ -440,12 +486,29 @@ describe('modules/platform/gerrit/client', () => {
client.wasApprovedBy(
partial<GerritChange>({
labels: {
- 'Code-Review': {},
+ 'Code-Review': {
+ all: [],
+ },
+ },
+ }),
+ 'user',
+ ),
+ ).toBeFalse();
+ });
+
+ it('not approved but with +1', () => {
+ expect(
+ client.wasApprovedBy(
+ partial<GerritChange>({
+ labels: {
+ 'Code-Review': {
+ all: [{ value: 1, username: 'user' }],
+ },
},
}),
'user',
),
- ).toBeUndefined();
+ ).toBeFalse();
});
it('approved by given user', () => {
@@ -454,10 +517,7 @@ describe('modules/platform/gerrit/client', () => {
partial<GerritChange>({
labels: {
'Code-Review': {
- approved: {
- _account_id: 1,
- username: 'user',
- },
+ all: [{ value: 2, username: 'user' }],
},
},
}),
@@ -472,10 +532,7 @@ describe('modules/platform/gerrit/client', () => {
partial<GerritChange>({
labels: {
'Code-Review': {
- approved: {
- _account_id: 1,
- username: 'other',
- },
+ all: [{ value: 2, username: 'other' }],
},
},
}),
diff --git a/lib/modules/platform/gerrit/client.ts b/lib/modules/platform/gerrit/client.ts
index 89b4ebf3e66e3c..efe7081ffc9f74 100644
--- a/lib/modules/platform/gerrit/client.ts
+++ b/lib/modules/platform/gerrit/client.ts
@@ -58,12 +58,14 @@ class GerritClient {
repository: string,
findPRConfig: GerritFindPRConfig,
refreshCache?: boolean,
+ extraOptions?: string[],
): Promise<GerritChange[]> {
const filters = GerritClient.buildSearchFilters(repository, findPRConfig);
+ const options = [...this.requestDetails, ...(extraOptions ?? [])];
const changes = await this.gerritHttp.getJson<GerritChange[]>(
`a/changes/?q=` +
filters.join('+') +
- this.requestDetails.map((det) => `&o=${det}`).join(''),
+ options.map((det) => `&o=${det}`).join(''),
{ memCache: !refreshCache },
);
logger.trace(
@@ -72,10 +74,13 @@ class GerritClient {
return changes.body;
}
- async getChange(changeNumber: number): Promise<GerritChange> {
+ async getChange(
+ changeNumber: number,
+ extraOptions?: string[],
+ ): Promise<GerritChange> {
+ const options = [...this.requestDetails, ...(extraOptions ?? [])];
const changes = await this.gerritHttp.getJson<GerritChange>(
- `a/changes/${changeNumber}?` +
- this.requestDetails.map((det) => `o=${det}`).join('&'),
+ `a/changes/${changeNumber}?` + options.map((det) => `o=${det}`).join('&'),
);
return changes.body;
}
@@ -189,15 +194,20 @@ class GerritClient {
}
async checkIfApproved(changeId: number): Promise<boolean> {
- const change = await client.getChange(changeId);
- const reviewLabels = change?.labels?.['Code-Review'];
- return reviewLabels === undefined || reviewLabels.approved !== undefined;
+ const change = await client.getChange(changeId, ['DETAILED_LABELS']);
+ const reviewLabel = change?.labels?.['Code-Review'];
+ return (
+ reviewLabel === undefined ||
+ Boolean(reviewLabel.all?.some((label) => label.value === 2))
+ );
}
- wasApprovedBy(change: GerritChange, username: string): boolean | undefined {
- return (
- change.labels?.['Code-Review'].approved &&
- change.labels['Code-Review'].approved.username === username
+ wasApprovedBy(change: GerritChange, username: string): boolean {
+ const reviewLabel = change?.labels?.['Code-Review'];
+ return Boolean(
+ reviewLabel?.all?.some(
+ (label) => label.value === 2 && label.username === username,
+ ),
);
}
diff --git a/lib/modules/platform/gerrit/scm.spec.ts b/lib/modules/platform/gerrit/scm.spec.ts
index f2041ef7f365c4..b654183fafd506 100644
--- a/lib/modules/platform/gerrit/scm.spec.ts
+++ b/lib/modules/platform/gerrit/scm.spec.ts
@@ -277,6 +277,7 @@ describe('modules/platform/gerrit/scm', () => {
targetBranch: 'main',
},
true,
+ ['DETAILED_LABELS'],
);
});
diff --git a/lib/modules/platform/gerrit/scm.ts b/lib/modules/platform/gerrit/scm.ts
index 6289f1f1f24047..646e975f1d93a8 100644
--- a/lib/modules/platform/gerrit/scm.ts
+++ b/lib/modules/platform/gerrit/scm.ts
@@ -101,7 +101,7 @@ export class GerritScm extends DefaultGitScm {
targetBranch: commit.baseBranch,
};
const existingChange = await client
- .findChanges(repository, searchConfig, true)
+ .findChanges(repository, searchConfig, true, ['DETAILED_LABELS'])
.then((res) => res.pop());
let hasChanges = true;
diff --git a/lib/modules/platform/gerrit/types.ts b/lib/modules/platform/gerrit/types.ts
index 0d1b5d90fed354..7f8603d275f11b 100644
--- a/lib/modules/platform/gerrit/types.ts
+++ b/lib/modules/platform/gerrit/types.ts
@@ -77,6 +77,8 @@ export interface GerritChangeMessageInfo {
export interface GerritLabelInfo {
approved?: GerritAccountInfo;
rejected?: GerritAccountInfo;
+ /** List of votes. Only set when o=DETAILED_LABELS. */
+ all?: GerritApprovalInfo[];
}
export interface GerritActionInfo {
@@ -99,3 +101,8 @@ export interface GerritMergeableInfo {
| 'CHERRY_PICK';
mergeable: boolean;
}
+
+export interface GerritApprovalInfo {
+ value?: number;
+ username?: string;
+}
From 107a298054e91b67c3cbb68cff8a713ce546281a Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sat, 30 Nov 2024 01:48:22 +0000
Subject: [PATCH 02/18] chore(gerrit): improve auto-approve on new patchset
workaround
---
lib/modules/platform/gerrit/client.spec.ts | 68 -------------------
lib/modules/platform/gerrit/client.ts | 13 +---
lib/modules/platform/gerrit/scm.spec.ts | 8 +--
lib/modules/platform/gerrit/scm.ts | 9 +--
lib/util/git/types.ts | 2 +
.../repository/update/branch/commit.ts | 2 +
6 files changed, 10 insertions(+), 92 deletions(-)
diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts
index 72073d96bb1912..b2ddb8775cb96a 100644
--- a/lib/modules/platform/gerrit/client.spec.ts
+++ b/lib/modules/platform/gerrit/client.spec.ts
@@ -473,74 +473,6 @@ describe('modules/platform/gerrit/client', () => {
expect(approveMock.isDone()).toBeTrue();
});
});
-
- describe('wasApprovedBy()', () => {
- it('label not exists', () => {
- expect(
- client.wasApprovedBy(partial<GerritChange>({}), 'user'),
- ).toBeFalse();
- });
-
- it('not approved by anyone', () => {
- expect(
- client.wasApprovedBy(
- partial<GerritChange>({
- labels: {
- 'Code-Review': {
- all: [],
- },
- },
- }),
- 'user',
- ),
- ).toBeFalse();
- });
-
- it('not approved but with +1', () => {
- expect(
- client.wasApprovedBy(
- partial<GerritChange>({
- labels: {
- 'Code-Review': {
- all: [{ value: 1, username: 'user' }],
- },
- },
- }),
- 'user',
- ),
- ).toBeFalse();
- });
-
- it('approved by given user', () => {
- expect(
- client.wasApprovedBy(
- partial<GerritChange>({
- labels: {
- 'Code-Review': {
- all: [{ value: 2, username: 'user' }],
- },
- },
- }),
- 'user',
- ),
- ).toBeTrue();
- });
-
- it('approved by given other', () => {
- expect(
- client.wasApprovedBy(
- partial<GerritChange>({
- labels: {
- 'Code-Review': {
- all: [{ value: 2, username: 'other' }],
- },
- },
- }),
- 'user',
- ),
- ).toBeFalse();
- });
- });
});
function gerritRestResponse(body: any): any {
diff --git a/lib/modules/platform/gerrit/client.ts b/lib/modules/platform/gerrit/client.ts
index efe7081ffc9f74..96d68e985ac365 100644
--- a/lib/modules/platform/gerrit/client.ts
+++ b/lib/modules/platform/gerrit/client.ts
@@ -58,14 +58,12 @@ class GerritClient {
repository: string,
findPRConfig: GerritFindPRConfig,
refreshCache?: boolean,
- extraOptions?: string[],
): Promise<GerritChange[]> {
const filters = GerritClient.buildSearchFilters(repository, findPRConfig);
- const options = [...this.requestDetails, ...(extraOptions ?? [])];
const changes = await this.gerritHttp.getJson<GerritChange[]>(
`a/changes/?q=` +
filters.join('+') +
- options.map((det) => `&o=${det}`).join(''),
+ this.requestDetails.map((det) => `&o=${det}`).join(''),
{ memCache: !refreshCache },
);
logger.trace(
@@ -202,15 +200,6 @@ class GerritClient {
);
}
- wasApprovedBy(change: GerritChange, username: string): boolean {
- const reviewLabel = change?.labels?.['Code-Review'];
- return Boolean(
- reviewLabel?.all?.some(
- (label) => label.value === 2 && label.username === username,
- ),
- );
- }
-
normalizeMessage(message: string): string {
//the last \n was removed from gerrit after the comment was added...
return message.substring(0, 0x4000).trim();
diff --git a/lib/modules/platform/gerrit/scm.spec.ts b/lib/modules/platform/gerrit/scm.spec.ts
index b654183fafd506..43df55d3b8d783 100644
--- a/lib/modules/platform/gerrit/scm.spec.ts
+++ b/lib/modules/platform/gerrit/scm.spec.ts
@@ -277,7 +277,6 @@ describe('modules/platform/gerrit/scm', () => {
targetBranch: 'main',
},
true,
- ['DETAILED_LABELS'],
);
});
@@ -370,7 +369,6 @@ describe('modules/platform/gerrit/scm', () => {
},
});
clientMock.findChanges.mockResolvedValueOnce([existingChange]);
- clientMock.wasApprovedBy.mockReturnValueOnce(true);
git.prepareCommit.mockResolvedValueOnce({
commitSha: 'commitSha' as LongCommitSha,
parentCommitSha: 'parentSha' as LongCommitSha,
@@ -386,6 +384,7 @@ describe('modules/platform/gerrit/scm', () => {
message: 'commit msg',
files: [],
prTitle: 'pr title',
+ autoApprove: true,
}),
).toBe('commitSha');
expect(git.prepareCommit).toHaveBeenCalledWith({
@@ -397,6 +396,7 @@ describe('modules/platform/gerrit/scm', () => {
'Renovate-Branch: renovate/dependency-1.x\nChange-Id: ...',
],
prTitle: 'pr title',
+ autoApprove: true,
force: true,
});
expect(git.fetchRevSpec).toHaveBeenCalledWith('refs/changes/1/2');
@@ -405,10 +405,6 @@ describe('modules/platform/gerrit/scm', () => {
sourceRef: 'renovate/dependency-1.x',
targetRef: 'refs/for/main',
});
- expect(clientMock.wasApprovedBy).toHaveBeenCalledWith(
- existingChange,
- 'user',
- );
expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
});
});
diff --git a/lib/modules/platform/gerrit/scm.ts b/lib/modules/platform/gerrit/scm.ts
index 646e975f1d93a8..3e1e9672f7a55a 100644
--- a/lib/modules/platform/gerrit/scm.ts
+++ b/lib/modules/platform/gerrit/scm.ts
@@ -101,7 +101,7 @@ export class GerritScm extends DefaultGitScm {
targetBranch: commit.baseBranch,
};
const existingChange = await client
- .findChanges(repository, searchConfig, true, ['DETAILED_LABELS'])
+ .findChanges(repository, searchConfig, true)
.then((res) => res.pop());
let hasChanges = true;
@@ -135,11 +135,8 @@ export class GerritScm extends DefaultGitScm {
files: commit.files,
});
if (pushResult) {
- //existingChange was the old change before commit/push. we need to approve again, if it was previously approved from renovate
- if (
- existingChange &&
- client.wasApprovedBy(existingChange, username)
- ) {
+ // New patchsets dismisses the approval, so we need to approve it again
+ if (existingChange && commit.autoApprove) {
await client.approveChange(existingChange._number);
}
return commitSha;
diff --git a/lib/util/git/types.ts b/lib/util/git/types.ts
index 63544838059dfb..d4cb37b6118ae2 100644
--- a/lib/util/git/types.ts
+++ b/lib/util/git/types.ts
@@ -85,6 +85,8 @@ export interface CommitFilesConfig {
platformCommit?: PlatformCommitOptions;
/** Only needed by Gerrit platform */
prTitle?: string;
+ /** Only needed by Gerrit platform */
+ autoApprove?: boolean;
}
export interface PushFilesConfig {
diff --git a/lib/workers/repository/update/branch/commit.ts b/lib/workers/repository/update/branch/commit.ts
index cadafc9ca0e75c..cf7c118ada226f 100644
--- a/lib/workers/repository/update/branch/commit.ts
+++ b/lib/workers/repository/update/branch/commit.ts
@@ -62,5 +62,7 @@ export function commitFilesToBranch(
platformCommit: config.platformCommit,
// Only needed by Gerrit platform
prTitle: config.prTitle,
+ // Only needed by Gerrit platform
+ autoApprove: config.autoApprove,
});
}
From 6ac7b9e0896ace6ec29b846f26f882f5e46d172d Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sat, 30 Nov 2024 04:04:20 -0300
Subject: [PATCH 03/18] chore(gerrit): try auto-approve right before auto-merge
---
lib/modules/platform/gerrit/index.ts | 4 ++++
lib/modules/platform/types.ts | 1 +
lib/workers/repository/update/pr/automerge.ts | 6 ++++++
3 files changed, 11 insertions(+)
diff --git a/lib/modules/platform/gerrit/index.ts b/lib/modules/platform/gerrit/index.ts
index d4f5b3c8123780..43133bd9250304 100644
--- a/lib/modules/platform/gerrit/index.ts
+++ b/lib/modules/platform/gerrit/index.ts
@@ -436,3 +436,7 @@ export function findIssue(title: string): Promise<Issue | null> {
export function getIssueList(): Promise<Issue[]> {
return Promise.resolve([]);
}
+
+export async function approvePr(number: number): Promise<void> {
+ await client.approveChange(number);
+}
diff --git a/lib/modules/platform/types.ts b/lib/modules/platform/types.ts
index cc770c1d8e43ee..67e95722462fa4 100644
--- a/lib/modules/platform/types.ts
+++ b/lib/modules/platform/types.ts
@@ -282,6 +282,7 @@ export interface Platform {
maxBodyLength(): number;
labelCharLimit?(): number;
+ approvePr?(number: number): Promise<void>;
}
export interface PlatformScm {
diff --git a/lib/workers/repository/update/pr/automerge.ts b/lib/workers/repository/update/pr/automerge.ts
index 2ace410741b3b8..4bbe2aaab1d53e 100644
--- a/lib/workers/repository/update/pr/automerge.ts
+++ b/lib/workers/repository/update/pr/automerge.ts
@@ -69,6 +69,12 @@ export async function checkAutoMerge(
prAutomergeBlockReason: 'PlatformNotReady',
};
}
+ // Usually the PR will already be approved, this is a last resort in case the
+ // approval was lost for some reason
+ if (config.autoApprove && platform.approvePr) {
+ logger.debug('Auto-approving PR if needed before automerge');
+ await platform.approvePr(pr.number);
+ }
const branchStatus = await resolveBranchStatus(
branchName,
!!config.internalChecksAsSuccess,
From 1d8daf50953601b93aecaa3838ecdd984a296978 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sun, 1 Dec 2024 16:40:11 +0000
Subject: [PATCH 04/18] test(gerrit): fix approvePr coverage
---
lib/modules/platform/gerrit/index.ts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/modules/platform/gerrit/index.ts b/lib/modules/platform/gerrit/index.ts
index 0d3989e148757b..a3246f5da38aa0 100644
--- a/lib/modules/platform/gerrit/index.ts
+++ b/lib/modules/platform/gerrit/index.ts
@@ -161,7 +161,7 @@ export async function updatePr(prConfig: UpdatePrConfig): Promise<void> {
);
}
if (prConfig.platformPrOptions?.autoApprove) {
- await client.approveChange(prConfig.number);
+ await approvePr(prConfig.number);
}
if (prConfig.state && prConfig.state === 'closed') {
await client.abandonChange(prConfig.number);
@@ -196,7 +196,7 @@ export async function createPr(prConfig: CreatePRConfig): Promise<Pr | null> {
TAG_PULL_REQUEST_BODY,
);
if (prConfig.platformPrOptions?.autoApprove) {
- await client.approveChange(pr._number);
+ await approvePr(pr._number);
}
return getPr(pr._number);
}
From 5eb284603f3afdcb0223d036c9d029482071e46b Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sun, 1 Dec 2024 17:02:52 +0000
Subject: [PATCH 05/18] chore(gerrit): still approve if change was approved
another user
---
lib/modules/platform/gerrit/client.spec.ts | 42 +++++++++++++++++++---
lib/modules/platform/gerrit/client.ts | 13 +++++--
lib/modules/platform/gerrit/types.ts | 1 +
3 files changed, 48 insertions(+), 8 deletions(-)
diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts
index a451bd020d9d46..7ac3193a68262d 100644
--- a/lib/modules/platform/gerrit/client.spec.ts
+++ b/lib/modules/platform/gerrit/client.spec.ts
@@ -5,10 +5,11 @@ import { setBaseUrl } from '../../../util/http/gerrit';
import type { FindPRConfig } from '../types';
import { client } from './client';
import type {
- GerritChange,
- GerritChangeMessageInfo,
- GerritFindPRConfig,
- GerritMergeableInfo,
+ GerritAccountInfo,
+ type GerritChange,
+ type GerritChangeMessageInfo,
+ type GerritFindPRConfig,
+ type GerritMergeableInfo,
} from './types';
const gerritEndpointUrl = 'https://dev.gerrit.com/renovate/';
@@ -399,12 +400,14 @@ describe('modules/platform/gerrit/client', () => {
describe('approveChange()', () => {
it('already approved - do nothing', async () => {
+ const owner = partial<GerritAccountInfo>({ username: 'user' });
const change = partial<GerritChange>({
labels: {
'Code-Review': {
- all: [{ value: 2 }],
+ all: [{ value: 2, username: owner.username }],
},
},
+ owner,
});
httpMock
.scope(gerritEndpointUrl)
@@ -480,6 +483,35 @@ describe('modules/platform/gerrit/client', () => {
await expect(client.approveChange(123456)).toResolve();
expect(approveMock.isDone()).toBeTrue();
});
+
+ it('already approved by another user - approve again', async () => {
+ const owner = partial<GerritAccountInfo>({ username: 'user' });
+ const change = partial<GerritChange>({
+ labels: {
+ 'Code-Review': {
+ all: [{ value: 2, username: 'other user' }],
+ },
+ },
+ owner,
+ });
+ httpMock
+ .scope(gerritEndpointUrl)
+ .get(
+ (url) =>
+ url.includes('/a/changes/123456?o=') &&
+ url.includes('o=DETAILED_LABELS'),
+ )
+ .reply(200, gerritRestResponse(change), jsonResultHeader);
+ const approveMock = httpMock
+ .scope(gerritEndpointUrl)
+ .post('/a/changes/123456/revisions/current/review', {
+ labels: { 'Code-Review': +2 },
+ notify: 'NONE',
+ })
+ .reply(200, gerritRestResponse(''), jsonResultHeader);
+ await expect(client.approveChange(123456)).toResolve();
+ expect(approveMock.isDone()).toBeTrue();
+ });
});
});
diff --git a/lib/modules/platform/gerrit/client.ts b/lib/modules/platform/gerrit/client.ts
index 83b82454ad141c..74221a497da71c 100644
--- a/lib/modules/platform/gerrit/client.ts
+++ b/lib/modules/platform/gerrit/client.ts
@@ -193,9 +193,11 @@ class GerritClient {
async approveChange(changeId: number): Promise<void> {
const isApproved = await this.checkIfApproved(changeId);
- if (!isApproved) {
- await this.setLabel(changeId, 'Code-Review', +2);
+ if (isApproved) {
+ logger.debug(`Change is already approved`);
+ return;
}
+ await this.setLabel(changeId, 'Code-Review', +2);
}
async checkIfApproved(changeId: number): Promise<boolean> {
@@ -203,7 +205,12 @@ class GerritClient {
const reviewLabel = change?.labels?.['Code-Review'];
return (
reviewLabel === undefined ||
- Boolean(reviewLabel.all?.some((label) => label.value === 2))
+ Boolean(
+ reviewLabel.all?.some(
+ (label) =>
+ label.value === 2 && label.username === change.owner.username,
+ ),
+ )
);
}
diff --git a/lib/modules/platform/gerrit/types.ts b/lib/modules/platform/gerrit/types.ts
index b65038100b3182..9cf35154a59468 100644
--- a/lib/modules/platform/gerrit/types.ts
+++ b/lib/modules/platform/gerrit/types.ts
@@ -52,6 +52,7 @@ export interface GerritChange {
*/
revisions: Record<string, GerritRevisionInfo>;
problems: unknown[];
+ owner: GerritAccountInfo;
}
export interface GerritCommitInfo {
From c679c28d01c8513adee1feea63ef4b3a53491270 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sun, 1 Dec 2024 16:38:36 +0000
Subject: [PATCH 06/18] feat(gitlab,azure): try approving before auto-merge
---
.../azure/__snapshots__/index.spec.ts.snap | 4 +
lib/modules/platform/azure/index.spec.ts | 80 +++++++++++-
lib/modules/platform/azure/index.ts | 61 +++++----
lib/modules/platform/gitlab/index.spec.ts | 120 +++++++++++++++++-
lib/modules/platform/gitlab/index.ts | 41 ++++--
lib/modules/platform/gitlab/types.ts | 4 +
6 files changed, 272 insertions(+), 38 deletions(-)
diff --git a/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap b/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap
index e782de620ace85..4fed47efa74ab4 100644
--- a/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap
+++ b/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap
@@ -217,6 +217,10 @@ 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 if the PR was approved by another user 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 73308cdfbfa98e..efc394cac94843 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,82 @@ describe('modules/platform/azure/index', () => {
isRequired: false,
};
const updateFn = jest.fn(() => prUpdateResult);
- azureApi.gitApi.mockImplementationOnce(
+ azureApi.gitApi.mockImplementation(
+ () =>
+ ({
+ updatePullRequest: jest.fn(() => prResult),
+ createPullRequestReviewer: updateFn,
+ getPullRequestById: jest.fn(() => ({
+ pullRequestId: prResult.pullRequestId,
+ createdBy: prResult.createdBy,
+ })),
+ }) as any,
+ );
+ const pr = await azure.updatePr({
+ number: prResult.pullRequestId,
+ prTitle: 'The Title',
+ prBody: 'Hello world',
+ platformPrOptions: { autoApprove: true },
+ });
+ 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();
+ });
+
+ it('should re-approve if the PR was approved by another user', 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),
@@ -1301,6 +1376,7 @@ describe('modules/platform/azure/index', () => {
getPullRequestById: jest.fn(() => ({
pullRequestId: prResult.pullRequestId,
createdBy: prResult.createdBy,
+ reviewers: [{ vote: AzurePrVote.Approved, id: 321 }],
})),
}) as any,
);
diff --git a/lib/modules/platform/azure/index.ts b/lib/modules/platform/azure/index.ts
index 21bd44eaa3f58c..039e7e18f7badc 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<void> {
+ const azureApiGit = await azureApi.gitApi();
+ const pr =
+ typeof prNumberOrPr === 'number'
+ ? await azureApiGit.getPullRequestById(prNumberOrPr, config.project)
+ : prNumberOrPr;
+
+ const isApproved = pr.reviewers?.some(
+ (reviewer) =>
+ reviewer.vote === AzurePrVote.Approved &&
+ reviewer.id === pr.createdBy?.id,
+ );
+
+ if (isApproved) {
+ 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 6eb62b886f7c40..e586ed9e49070d 100644
--- a/lib/modules/platform/gitlab/index.spec.ts
+++ b/lib/modules/platform/gitlab/index.spec.ts
@@ -128,9 +128,18 @@ describe('modules/platform/gitlab/index', () => {
});
it(`should reuse existing gitAuthor`, async () => {
- httpMock.scope(gitlabApiHost).get('/api/v4/version').reply(200, {
- version: '13.3.6-ee',
- });
+ httpMock
+ .scope(gitlabApiHost)
+ .get('/api/v4/user')
+ .reply(200, {
+ email: 'different@email.com',
+ name: 'Different Name',
+ id: 1,
+ })
+ .get('/api/v4/version')
+ .reply(200, {
+ version: '13.3.6-ee',
+ });
expect(
await gitlab.initPlatform({
token: 'some-token',
@@ -1900,6 +1909,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 +2788,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 +2825,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 +3155,104 @@ 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(
+ gitlab.updatePr({
+ number: 1,
+ prTitle: 'title',
+ prBody: 'body',
+ platformPrOptions: {
+ autoApprove: true,
+ },
+ }),
+ ).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('auto-approves if approved by another user', 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: 2,
+ },
+ },
+ ],
+ })
.post('/api/v4/projects/undefined/merge_requests/1/approve')
.reply(200);
await expect(
diff --git a/lib/modules/platform/gitlab/index.ts b/lib/modules/platform/gitlab/index.ts
index 30965b96308505..f2a013dd7d585f 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,26 @@ 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`,
+ );
+
+ const isApproved = approvals.approved_by?.some(
+ (approval) => approval.user?.id === renovateUserId,
+ );
+
+ if (isApproved) {
+ logger.debug('MR is already approved');
+ return;
+ }
+ } catch (err) {
+ logger.warn(
+ { err },
+ 'GitLab: Error checking if the 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 244ec8366361e6..c37243b8a44bec 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 }[];
+}
From dbdf41a9366a5d68478804fad40ff514ba1f2707 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sun, 1 Dec 2024 17:27:27 +0000
Subject: [PATCH 07/18] chore: add better jsdoc for approvePr
---
lib/modules/platform/types.ts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/modules/platform/types.ts b/lib/modules/platform/types.ts
index 67e95722462fa4..8336682efdcfbb 100644
--- a/lib/modules/platform/types.ts
+++ b/lib/modules/platform/types.ts
@@ -282,6 +282,10 @@ export interface Platform {
maxBodyLength(): number;
labelCharLimit?(): number;
+ /**
+ * For platforms that support `autoApprove`. It should handle when the PR
+ * is already be approved.
+ */
approvePr?(number: number): Promise<void>;
}
From bd05816dd1e873725d52f5ca312368df5238da4f Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sun, 1 Dec 2024 17:28:48 +0000
Subject: [PATCH 08/18] test(gerrit): fix lint in client.spec.ts
---
lib/modules/platform/gerrit/client.spec.ts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts
index 7ac3193a68262d..0c5ef3f265e204 100644
--- a/lib/modules/platform/gerrit/client.spec.ts
+++ b/lib/modules/platform/gerrit/client.spec.ts
@@ -4,8 +4,8 @@ import { REPOSITORY_ARCHIVED } from '../../../constants/error-messages';
import { setBaseUrl } from '../../../util/http/gerrit';
import type { FindPRConfig } from '../types';
import { client } from './client';
-import type {
- GerritAccountInfo,
+import {
+ type GerritAccountInfo,
type GerritChange,
type GerritChangeMessageInfo,
type GerritFindPRConfig,
From ac30f2b204b2cb8acca3b0ebf534b6ee5726d543 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sun, 1 Dec 2024 17:32:33 +0000
Subject: [PATCH 09/18] test(automerge): add test for auto approve before
merging
---
lib/workers/repository/update/pr/automerge.spec.ts | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/lib/workers/repository/update/pr/automerge.spec.ts b/lib/workers/repository/update/pr/automerge.spec.ts
index 71d389ad126816..a8f68181dd57ab 100644
--- a/lib/workers/repository/update/pr/automerge.spec.ts
+++ b/lib/workers/repository/update/pr/automerge.spec.ts
@@ -124,6 +124,19 @@ describe('workers/repository/update/pr/automerge', () => {
expect(platform.mergePr).toHaveBeenCalledTimes(0);
});
+ it('should attempt to auto-approve if supported', async () => {
+ config.autoApprove = true;
+ config.automerge = true;
+ config.pruneBranchAfterAutomerge = true;
+ platform.approvePr.mockResolvedValueOnce();
+ platform.getBranchStatus.mockResolvedValueOnce('green');
+ platform.mergePr.mockResolvedValueOnce(true);
+ const res = await prAutomerge.checkAutoMerge(pr, config);
+ expect(res).toEqual({ automerged: true, branchRemoved: true });
+ expect(platform.approvePr).toHaveBeenCalledTimes(1);
+ expect(platform.mergePr).toHaveBeenCalledTimes(1);
+ });
+
it('should not automerge if enabled and pr is unmergeable', async () => {
config.automerge = true;
scm.isBranchConflicted.mockResolvedValueOnce(true);
From 7923d5b955e3d07c544b4736a83b06c1e0d27d12 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Mon, 2 Dec 2024 17:02:59 +0000
Subject: [PATCH 10/18] Revert "feat(gitlab,azure): try approving before
auto-merge"
This reverts commit c679c28d01c8513adee1feea63ef4b3a53491270.
---
.../azure/__snapshots__/index.spec.ts.snap | 4 -
lib/modules/platform/azure/index.spec.ts | 80 +-----------
lib/modules/platform/azure/index.ts | 61 ++++-----
lib/modules/platform/gitlab/index.spec.ts | 120 +-----------------
lib/modules/platform/gitlab/index.ts | 41 ++----
lib/modules/platform/gitlab/types.ts | 4 -
6 files changed, 38 insertions(+), 272 deletions(-)
diff --git a/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap b/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap
index 4fed47efa74ab4..e782de620ace85 100644
--- a/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap
+++ b/lib/modules/platform/azure/__snapshots__/index.spec.ts.snap
@@ -217,10 +217,6 @@ 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 if the PR was approved by another user 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 efc394cac94843..73308cdfbfa98e 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.mockImplementation(
+ azureApi.gitApi.mockImplementationOnce(
() =>
({
createPullRequest: jest.fn(() => prResult),
@@ -1293,82 +1293,7 @@ describe('modules/platform/azure/index', () => {
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,
- })),
- }) as any,
- );
- const pr = await azure.updatePr({
- number: prResult.pullRequestId,
- prTitle: 'The Title',
- prBody: 'Hello world',
- platformPrOptions: { autoApprove: true },
- });
- 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();
- });
-
- it('should re-approve if the PR was approved by another user', 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(
+ azureApi.gitApi.mockImplementationOnce(
() =>
({
updatePullRequest: jest.fn(() => prResult),
@@ -1376,7 +1301,6 @@ describe('modules/platform/azure/index', () => {
getPullRequestById: jest.fn(() => ({
pullRequestId: prResult.pullRequestId,
createdBy: prResult.createdBy,
- reviewers: [{ vote: AzurePrVote.Approved, id: 321 }],
})),
}) as any,
);
diff --git a/lib/modules/platform/azure/index.ts b/lib/modules/platform/azure/index.ts
index 039e7e18f7badc..21bd44eaa3f58c 100644
--- a/lib/modules/platform/azure/index.ts
+++ b/lib/modules/platform/azure/index.ts
@@ -513,7 +513,18 @@ export async function createPr({
);
}
if (platformPrOptions?.autoApprove) {
- await approvePr(pr);
+ await azureApiGit.createPullRequestReviewer(
+ {
+ reviewerUrl: pr.createdBy!.url,
+ vote: AzurePrVote.Approved,
+ isFlagged: false,
+ isRequired: false,
+ },
+ config.repoId,
+ // TODO #22198
+ pr.pullRequestId!,
+ pr.createdBy!.id!,
+ );
}
await Promise.all(
labels!.map((label) =>
@@ -570,7 +581,19 @@ export async function updatePr({
objToUpdate.status = PullRequestStatus.Abandoned;
}
if (platformPrOptions?.autoApprove) {
- await approvePr(prNo);
+ 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!,
+ );
}
const updatedPr = await azureApiGit.updatePullRequest(
@@ -992,37 +1015,3 @@ 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;
-
- const isApproved = pr.reviewers?.some(
- (reviewer) =>
- reviewer.vote === AzurePrVote.Approved &&
- reviewer.id === pr.createdBy?.id,
- );
-
- if (isApproved) {
- 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 e586ed9e49070d..6eb62b886f7c40 100644
--- a/lib/modules/platform/gitlab/index.spec.ts
+++ b/lib/modules/platform/gitlab/index.spec.ts
@@ -128,18 +128,9 @@ describe('modules/platform/gitlab/index', () => {
});
it(`should reuse existing gitAuthor`, async () => {
- httpMock
- .scope(gitlabApiHost)
- .get('/api/v4/user')
- .reply(200, {
- email: 'different@email.com',
- name: 'Different Name',
- id: 1,
- })
- .get('/api/v4/version')
- .reply(200, {
- version: '13.3.6-ee',
- });
+ httpMock.scope(gitlabApiHost).get('/api/v4/version').reply(200, {
+ version: '13.3.6-ee',
+ });
expect(
await gitlab.initPlatform({
token: 'some-token',
@@ -1909,7 +1900,6 @@ describe('modules/platform/gitlab/index', () => {
.get('/api/v4/user')
.reply(200, {
email: 'a@b.com',
- id: 1,
name: 'Renovate Bot',
})
.get('/api/v4/version')
@@ -2788,10 +2778,6 @@ 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(
@@ -2825,8 +2811,6 @@ 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(
@@ -3155,104 +3139,6 @@ 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(
- gitlab.updatePr({
- number: 1,
- prTitle: 'title',
- prBody: 'body',
- platformPrOptions: {
- autoApprove: true,
- },
- }),
- ).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('auto-approves if approved by another user', 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: 2,
- },
- },
- ],
- })
.post('/api/v4/projects/undefined/merge_requests/1/approve')
.reply(200);
await expect(
diff --git a/lib/modules/platform/gitlab/index.ts b/lib/modules/platform/gitlab/index.ts
index f2a013dd7d585f..30965b96308505 100644
--- a/lib/modules/platform/gitlab/index.ts
+++ b/lib/modules/platform/gitlab/index.ts
@@ -65,7 +65,6 @@ import { getMR, updateMR } from './merge-request';
import { LastPipelineId } from './schema';
import type {
GitLabMergeRequest,
- GitLabMergeRequestApprovals,
GitlabComment,
GitlabIssue,
GitlabPr,
@@ -95,7 +94,6 @@ const defaults = {
export const id = 'gitlab';
let draftPrefix = DRAFT_PREFIX;
-let renovateUserId: number;
export async function initPlatform({
endpoint,
@@ -116,22 +114,19 @@ 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;
@@ -716,26 +711,6 @@ 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`,
- );
-
- const isApproved = approvals.approved_by?.some(
- (approval) => approval.user?.id === renovateUserId,
- );
-
- if (isApproved) {
- logger.debug('MR is already approved');
- return;
- }
- } catch (err) {
- logger.warn(
- { err },
- 'GitLab: Error checking if the 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 c37243b8a44bec..244ec8366361e6 100644
--- a/lib/modules/platform/gitlab/types.ts
+++ b/lib/modules/platform/gitlab/types.ts
@@ -77,7 +77,3 @@ export interface GitlabUserStatus {
emoji?: string;
availability: 'not_set' | 'busy';
}
-
-export interface GitLabMergeRequestApprovals {
- approved_by?: { user?: GitLabUser }[];
-}
From c4c81e30fc32f68f62dd1e9bc6a43d20e4d8cfbe Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Mon, 2 Dec 2024 17:18:17 +0000
Subject: [PATCH 11/18] chore: rename approvePr to approvePrForAutomerge
---
lib/modules/platform/gerrit/index.ts | 13 ++++++++++---
lib/modules/platform/types.ts | 6 +++---
lib/workers/repository/update/pr/automerge.spec.ts | 4 ++--
lib/workers/repository/update/pr/automerge.ts | 6 +++---
4 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/lib/modules/platform/gerrit/index.ts b/lib/modules/platform/gerrit/index.ts
index a3246f5da38aa0..206283afb99595 100644
--- a/lib/modules/platform/gerrit/index.ts
+++ b/lib/modules/platform/gerrit/index.ts
@@ -161,7 +161,7 @@ export async function updatePr(prConfig: UpdatePrConfig): Promise<void> {
);
}
if (prConfig.platformPrOptions?.autoApprove) {
- await approvePr(prConfig.number);
+ await client.approveChange(prConfig.number);
}
if (prConfig.state && prConfig.state === 'closed') {
await client.abandonChange(prConfig.number);
@@ -196,7 +196,7 @@ export async function createPr(prConfig: CreatePRConfig): Promise<Pr | null> {
TAG_PULL_REQUEST_BODY,
);
if (prConfig.platformPrOptions?.autoApprove) {
- await approvePr(pr._number);
+ await client.approveChange(pr._number);
}
return getPr(pr._number);
}
@@ -443,6 +443,13 @@ export function getIssueList(): Promise<Issue[]> {
return Promise.resolve([]);
}
-export async function approvePr(number: number): Promise<void> {
+/**
+ * The Code-Review +2 posted when the change was created or updated in Gerrit
+ * may have been downgraded by a CI check utilizing the same account as
+ * Renovate (e.g. SonarQube which posts Code-Review +1). This function will
+ * post a +2 again on the change, if needed, before Renovate attempt to
+ * automerge it.
+ */
+export async function approvePrForAutomerge(number: number): Promise<void> {
await client.approveChange(number);
}
diff --git a/lib/modules/platform/types.ts b/lib/modules/platform/types.ts
index 8336682efdcfbb..cfd3126068e2d4 100644
--- a/lib/modules/platform/types.ts
+++ b/lib/modules/platform/types.ts
@@ -283,10 +283,10 @@ export interface Platform {
maxBodyLength(): number;
labelCharLimit?(): number;
/**
- * For platforms that support `autoApprove`. It should handle when the PR
- * is already be approved.
+ * Platforms that support `autoApprove` can implement this function. It will be
+ * invoked during automerge before the PR branch status is checked.
*/
- approvePr?(number: number): Promise<void>;
+ approvePrForAutomerge?(number: number): Promise<void>;
}
export interface PlatformScm {
diff --git a/lib/workers/repository/update/pr/automerge.spec.ts b/lib/workers/repository/update/pr/automerge.spec.ts
index a8f68181dd57ab..1d5e0a70127b13 100644
--- a/lib/workers/repository/update/pr/automerge.spec.ts
+++ b/lib/workers/repository/update/pr/automerge.spec.ts
@@ -128,12 +128,12 @@ describe('workers/repository/update/pr/automerge', () => {
config.autoApprove = true;
config.automerge = true;
config.pruneBranchAfterAutomerge = true;
- platform.approvePr.mockResolvedValueOnce();
+ platform.approvePrForAutomerge.mockResolvedValueOnce();
platform.getBranchStatus.mockResolvedValueOnce('green');
platform.mergePr.mockResolvedValueOnce(true);
const res = await prAutomerge.checkAutoMerge(pr, config);
expect(res).toEqual({ automerged: true, branchRemoved: true });
- expect(platform.approvePr).toHaveBeenCalledTimes(1);
+ expect(platform.approvePrForAutomerge).toHaveBeenCalledTimes(1);
expect(platform.mergePr).toHaveBeenCalledTimes(1);
});
diff --git a/lib/workers/repository/update/pr/automerge.ts b/lib/workers/repository/update/pr/automerge.ts
index 4bbe2aaab1d53e..9803adc7f8c242 100644
--- a/lib/workers/repository/update/pr/automerge.ts
+++ b/lib/workers/repository/update/pr/automerge.ts
@@ -71,9 +71,9 @@ export async function checkAutoMerge(
}
// Usually the PR will already be approved, this is a last resort in case the
// approval was lost for some reason
- if (config.autoApprove && platform.approvePr) {
- logger.debug('Auto-approving PR if needed before automerge');
- await platform.approvePr(pr.number);
+ if (config.autoApprove && platform.approvePrForAutomerge) {
+ logger.debug('Auto-approving PR for automerge');
+ await platform.approvePrForAutomerge(pr.number);
}
const branchStatus = await resolveBranchStatus(
branchName,
From 4dfc8177933d80e08152e0dcb88690159495d33b Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Mon, 2 Dec 2024 17:20:47 +0000
Subject: [PATCH 12/18] Revert "chore(gerrit): still approve if change was
approved another user"
This reverts commit 5eb284603f3afdcb0223d036c9d029482071e46b.
---
lib/modules/platform/gerrit/client.spec.ts | 34 +---------------------
lib/modules/platform/gerrit/client.ts | 13 ++-------
lib/modules/platform/gerrit/types.ts | 1 -
3 files changed, 4 insertions(+), 44 deletions(-)
diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts
index 0c5ef3f265e204..a74830ea610932 100644
--- a/lib/modules/platform/gerrit/client.spec.ts
+++ b/lib/modules/platform/gerrit/client.spec.ts
@@ -5,7 +5,6 @@ import { setBaseUrl } from '../../../util/http/gerrit';
import type { FindPRConfig } from '../types';
import { client } from './client';
import {
- type GerritAccountInfo,
type GerritChange,
type GerritChangeMessageInfo,
type GerritFindPRConfig,
@@ -400,14 +399,12 @@ describe('modules/platform/gerrit/client', () => {
describe('approveChange()', () => {
it('already approved - do nothing', async () => {
- const owner = partial<GerritAccountInfo>({ username: 'user' });
const change = partial<GerritChange>({
labels: {
'Code-Review': {
- all: [{ value: 2, username: owner.username }],
+ all: [{ value: 2 }],
},
},
- owner,
});
httpMock
.scope(gerritEndpointUrl)
@@ -483,35 +480,6 @@ describe('modules/platform/gerrit/client', () => {
await expect(client.approveChange(123456)).toResolve();
expect(approveMock.isDone()).toBeTrue();
});
-
- it('already approved by another user - approve again', async () => {
- const owner = partial<GerritAccountInfo>({ username: 'user' });
- const change = partial<GerritChange>({
- labels: {
- 'Code-Review': {
- all: [{ value: 2, username: 'other user' }],
- },
- },
- owner,
- });
- httpMock
- .scope(gerritEndpointUrl)
- .get(
- (url) =>
- url.includes('/a/changes/123456?o=') &&
- url.includes('o=DETAILED_LABELS'),
- )
- .reply(200, gerritRestResponse(change), jsonResultHeader);
- const approveMock = httpMock
- .scope(gerritEndpointUrl)
- .post('/a/changes/123456/revisions/current/review', {
- labels: { 'Code-Review': +2 },
- notify: 'NONE',
- })
- .reply(200, gerritRestResponse(''), jsonResultHeader);
- await expect(client.approveChange(123456)).toResolve();
- expect(approveMock.isDone()).toBeTrue();
- });
});
});
diff --git a/lib/modules/platform/gerrit/client.ts b/lib/modules/platform/gerrit/client.ts
index 74221a497da71c..83b82454ad141c 100644
--- a/lib/modules/platform/gerrit/client.ts
+++ b/lib/modules/platform/gerrit/client.ts
@@ -193,11 +193,9 @@ class GerritClient {
async approveChange(changeId: number): Promise<void> {
const isApproved = await this.checkIfApproved(changeId);
- if (isApproved) {
- logger.debug(`Change is already approved`);
- return;
+ if (!isApproved) {
+ await this.setLabel(changeId, 'Code-Review', +2);
}
- await this.setLabel(changeId, 'Code-Review', +2);
}
async checkIfApproved(changeId: number): Promise<boolean> {
@@ -205,12 +203,7 @@ class GerritClient {
const reviewLabel = change?.labels?.['Code-Review'];
return (
reviewLabel === undefined ||
- Boolean(
- reviewLabel.all?.some(
- (label) =>
- label.value === 2 && label.username === change.owner.username,
- ),
- )
+ Boolean(reviewLabel.all?.some((label) => label.value === 2))
);
}
diff --git a/lib/modules/platform/gerrit/types.ts b/lib/modules/platform/gerrit/types.ts
index 9cf35154a59468..b65038100b3182 100644
--- a/lib/modules/platform/gerrit/types.ts
+++ b/lib/modules/platform/gerrit/types.ts
@@ -52,7 +52,6 @@ export interface GerritChange {
*/
revisions: Record<string, GerritRevisionInfo>;
problems: unknown[];
- owner: GerritAccountInfo;
}
export interface GerritCommitInfo {
From bb317ce921bb4d7d27c56683908d8aa8f82933a4 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Mon, 2 Dec 2024 17:30:54 +0000
Subject: [PATCH 13/18] chore(gerrit): improve approve method to avoid some API
calls
---
lib/modules/platform/gerrit/index.spec.ts | 44 +----------------------
lib/modules/platform/gerrit/index.ts | 10 ++----
lib/modules/platform/gerrit/scm.spec.ts | 3 +-
lib/modules/platform/gerrit/scm.ts | 10 +++---
4 files changed, 9 insertions(+), 58 deletions(-)
diff --git a/lib/modules/platform/gerrit/index.spec.ts b/lib/modules/platform/gerrit/index.spec.ts
index 3b9c456f1852e4..692dad06d982fd 100644
--- a/lib/modules/platform/gerrit/index.spec.ts
+++ b/lib/modules/platform/gerrit/index.spec.ts
@@ -187,28 +187,6 @@ describe('modules/platform/gerrit/index', () => {
gerrit.writeToConfig({ labels: {} });
});
- it('updatePr() - auto approve enabled', async () => {
- const change = partial<GerritChange>({
- current_revision: 'some-revision',
- revisions: {
- 'some-revision': partial<GerritRevisionInfo>({
- commit: {
- message: 'some message',
- },
- }),
- },
- });
- clientMock.getChange.mockResolvedValueOnce(change);
- await gerrit.updatePr({
- number: 123456,
- prTitle: 'subject',
- platformPrOptions: {
- autoApprove: true,
- },
- });
- expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
- });
-
it('updatePr() - closed => abandon the change', async () => {
const change = partial<GerritChange>({});
clientMock.getChange.mockResolvedValueOnce(change);
@@ -309,7 +287,7 @@ describe('modules/platform/gerrit/index', () => {
]);
});
- it('createPr() - update body WITHOUT approve', async () => {
+ it('createPr() - update body', async () => {
const pr = await gerrit.createPr({
sourceBranch: 'source',
targetBranch: 'target',
@@ -325,26 +303,6 @@ describe('modules/platform/gerrit/index', () => {
'body',
TAG_PULL_REQUEST_BODY,
);
- expect(clientMock.approveChange).not.toHaveBeenCalled();
- });
-
- it('createPr() - update body and approve', async () => {
- const pr = await gerrit.createPr({
- sourceBranch: 'source',
- targetBranch: 'target',
- prTitle: change.subject,
- prBody: 'body',
- platformPrOptions: {
- autoApprove: true,
- },
- });
- expect(pr).toHaveProperty('number', 123456);
- expect(clientMock.addMessageIfNotAlreadyExists).toHaveBeenCalledWith(
- 123456,
- 'body',
- TAG_PULL_REQUEST_BODY,
- );
- expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
});
});
diff --git a/lib/modules/platform/gerrit/index.ts b/lib/modules/platform/gerrit/index.ts
index 206283afb99595..98afc1a271bdd6 100644
--- a/lib/modules/platform/gerrit/index.ts
+++ b/lib/modules/platform/gerrit/index.ts
@@ -160,9 +160,6 @@ export async function updatePr(prConfig: UpdatePrConfig): Promise<void> {
TAG_PULL_REQUEST_BODY,
);
}
- if (prConfig.platformPrOptions?.autoApprove) {
- await client.approveChange(prConfig.number);
- }
if (prConfig.state && prConfig.state === 'closed') {
await client.abandonChange(prConfig.number);
}
@@ -195,9 +192,6 @@ export async function createPr(prConfig: CreatePRConfig): Promise<Pr | null> {
prConfig.prBody,
TAG_PULL_REQUEST_BODY,
);
- if (prConfig.platformPrOptions?.autoApprove) {
- await client.approveChange(pr._number);
- }
return getPr(pr._number);
}
@@ -444,10 +438,10 @@ export function getIssueList(): Promise<Issue[]> {
}
/**
- * The Code-Review +2 posted when the change was created or updated in Gerrit
+ * The Code-Review +2 vote of when the change was created or updated in Gerrit
* may have been downgraded by a CI check utilizing the same account as
* Renovate (e.g. SonarQube which posts Code-Review +1). This function will
- * post a +2 again on the change, if needed, before Renovate attempt to
+ * vote with +2 again on the change, if needed, before Renovate attempt to
* automerge it.
*/
export async function approvePrForAutomerge(number: number): Promise<void> {
diff --git a/lib/modules/platform/gerrit/scm.spec.ts b/lib/modules/platform/gerrit/scm.spec.ts
index 77c4797bda18c6..1d50b6e43ef069 100644
--- a/lib/modules/platform/gerrit/scm.spec.ts
+++ b/lib/modules/platform/gerrit/scm.spec.ts
@@ -403,9 +403,8 @@ describe('modules/platform/gerrit/scm', () => {
expect(git.pushCommit).toHaveBeenCalledWith({
files: [],
sourceRef: 'renovate/dependency-1.x',
- targetRef: 'refs/for/main%notify=NONE',
+ targetRef: 'refs/for/main%notify=NONE,l=Code-Review+2',
});
- expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
});
});
});
diff --git a/lib/modules/platform/gerrit/scm.ts b/lib/modules/platform/gerrit/scm.ts
index 01e79df330dd69..a81bd5d68de1ea 100644
--- a/lib/modules/platform/gerrit/scm.ts
+++ b/lib/modules/platform/gerrit/scm.ts
@@ -129,16 +129,16 @@ export class GerritScm extends DefaultGitScm {
hasChanges = await git.hasDiff('HEAD', 'FETCH_HEAD'); //avoid empty patchsets
}
if (hasChanges || commit.force) {
+ const pushOptions = ['notify=NONE'];
+ if (commit.autoApprove) {
+ pushOptions.push('l=Code-Review+2');
+ }
const pushResult = await git.pushCommit({
sourceRef: commit.branchName,
- targetRef: `refs/for/${commit.baseBranch!}%notify=NONE`,
+ targetRef: `refs/for/${commit.baseBranch!}%${pushOptions.join(',')}`,
files: commit.files,
});
if (pushResult) {
- // New patchsets dismisses the approval, so we need to approve it again
- if (existingChange && commit.autoApprove) {
- await client.approveChange(existingChange._number);
- }
return commitSha;
}
}
From 53743b27bf837fe8109639872944a5b0d5906d22 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Mon, 2 Dec 2024 17:38:39 +0000
Subject: [PATCH 14/18] chore: fix type import
---
lib/modules/platform/gerrit/client.spec.ts | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts
index a74830ea610932..a451bd020d9d46 100644
--- a/lib/modules/platform/gerrit/client.spec.ts
+++ b/lib/modules/platform/gerrit/client.spec.ts
@@ -4,11 +4,11 @@ import { REPOSITORY_ARCHIVED } from '../../../constants/error-messages';
import { setBaseUrl } from '../../../util/http/gerrit';
import type { FindPRConfig } from '../types';
import { client } from './client';
-import {
- type GerritChange,
- type GerritChangeMessageInfo,
- type GerritFindPRConfig,
- type GerritMergeableInfo,
+import type {
+ GerritChange,
+ GerritChangeMessageInfo,
+ GerritFindPRConfig,
+ GerritMergeableInfo,
} from './types';
const gerritEndpointUrl = 'https://dev.gerrit.com/renovate/';
From e0b6f8d5ed95712431d5a5f833369528df7f9d1d Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Mon, 2 Dec 2024 17:41:35 +0000
Subject: [PATCH 15/18] chore: remove spurious comment
---
lib/workers/repository/update/pr/automerge.ts | 2 --
1 file changed, 2 deletions(-)
diff --git a/lib/workers/repository/update/pr/automerge.ts b/lib/workers/repository/update/pr/automerge.ts
index 9803adc7f8c242..255f185cd6a23e 100644
--- a/lib/workers/repository/update/pr/automerge.ts
+++ b/lib/workers/repository/update/pr/automerge.ts
@@ -69,8 +69,6 @@ export async function checkAutoMerge(
prAutomergeBlockReason: 'PlatformNotReady',
};
}
- // Usually the PR will already be approved, this is a last resort in case the
- // approval was lost for some reason
if (config.autoApprove && platform.approvePrForAutomerge) {
logger.debug('Auto-approving PR for automerge');
await platform.approvePrForAutomerge(pr.number);
From b93ee7a628c80f4eb8cfe339834c313b7fc75c2e Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sun, 29 Dec 2024 15:52:37 +0000
Subject: [PATCH 16/18] Address review comments
---
lib/modules/platform/gerrit/client.ts | 6 ++++--
lib/workers/repository/update/pr/automerge.spec.ts | 1 -
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/lib/modules/platform/gerrit/client.ts b/lib/modules/platform/gerrit/client.ts
index 83b82454ad141c..e255d44a6a9bb5 100644
--- a/lib/modules/platform/gerrit/client.ts
+++ b/lib/modules/platform/gerrit/client.ts
@@ -2,6 +2,7 @@ import { REPOSITORY_ARCHIVED } from '../../../constants/error-messages';
import { logger } from '../../../logger';
import { GerritHttp } from '../../../util/http/gerrit';
import { regEx } from '../../../util/regex';
+import { getQueryString } from '../../../util/url';
import type {
GerritAccountInfo,
GerritBranchInfo,
@@ -77,8 +78,9 @@ class GerritClient {
extraOptions?: string[],
): Promise<GerritChange> {
const options = [...this.requestDetails, ...(extraOptions ?? [])];
+ const queryString = getQueryString({ o: options });
const changes = await this.gerritHttp.getJson<GerritChange>(
- `a/changes/${changeNumber}?` + options.map((det) => `o=${det}`).join('&'),
+ `a/changes/${changeNumber}?${queryString}`,
);
return changes.body;
}
@@ -203,7 +205,7 @@ class GerritClient {
const reviewLabel = change?.labels?.['Code-Review'];
return (
reviewLabel === undefined ||
- Boolean(reviewLabel.all?.some((label) => label.value === 2))
+ reviewLabel.all?.some((label) => label.value === 2) === true
);
}
diff --git a/lib/workers/repository/update/pr/automerge.spec.ts b/lib/workers/repository/update/pr/automerge.spec.ts
index 1d5e0a70127b13..b89690d773784d 100644
--- a/lib/workers/repository/update/pr/automerge.spec.ts
+++ b/lib/workers/repository/update/pr/automerge.spec.ts
@@ -128,7 +128,6 @@ describe('workers/repository/update/pr/automerge', () => {
config.autoApprove = true;
config.automerge = true;
config.pruneBranchAfterAutomerge = true;
- platform.approvePrForAutomerge.mockResolvedValueOnce();
platform.getBranchStatus.mockResolvedValueOnce('green');
platform.mergePr.mockResolvedValueOnce(true);
const res = await prAutomerge.checkAutoMerge(pr, config);
From add6ca57200bdd28586078ac478c79946e2949f1 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sun, 29 Dec 2024 16:04:44 +0000
Subject: [PATCH 17/18] Add tests for approvePrForAutomerge
---
lib/modules/platform/gerrit/index.spec.ts | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/lib/modules/platform/gerrit/index.spec.ts b/lib/modules/platform/gerrit/index.spec.ts
index 692dad06d982fd..17fa9e54deb747 100644
--- a/lib/modules/platform/gerrit/index.spec.ts
+++ b/lib/modules/platform/gerrit/index.spec.ts
@@ -708,6 +708,13 @@ describe('modules/platform/gerrit/index', () => {
//TODO: add some tests for Gerrit-specific replacements..
});
+ describe('approvePrForAutomerge()', () => {
+ it('approvePrForAutomerge() - calls approveChange', async () => {
+ await expect(gerrit.approvePrForAutomerge(123456)).toResolve();
+ expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
+ });
+ });
+
describe('currently unused/not-implemented functions', () => {
it('deleteLabel()', async () => {
await expect(
From a6d9d910e6862ab2f8827e0d72351011e787ad89 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
Date: Sun, 29 Dec 2024 16:10:26 +0000
Subject: [PATCH 18/18] refactor(gerrit): remove deprecated source branch as
hashtags support
---
lib/modules/platform/gerrit/client.spec.ts | 1 -
lib/modules/platform/gerrit/client.ts | 11 +-----
lib/modules/platform/gerrit/readme.md | 6 ----
lib/modules/platform/gerrit/types.ts | 4 ---
lib/modules/platform/gerrit/utils.spec.ts | 41 ----------------------
lib/modules/platform/gerrit/utils.ts | 7 ----
6 files changed, 1 insertion(+), 69 deletions(-)
diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts
index a451bd020d9d46..8411e5f4405239 100644
--- a/lib/modules/platform/gerrit/client.spec.ts
+++ b/lib/modules/platform/gerrit/client.spec.ts
@@ -102,7 +102,6 @@ describe('modules/platform/gerrit/client', () => {
'footer:Renovate-Branch=dependency-xyz',
{ branchName: 'dependency-xyz' },
],
- ['hashtag:sourceBranch-dependency-xyz', { branchName: 'dependency-xyz' }], // for backwards compatibility
['label:Code-Review=-2', { branchName: 'dependency-xyz', label: '-2' }],
[
'branch:otherTarget',
diff --git a/lib/modules/platform/gerrit/client.ts b/lib/modules/platform/gerrit/client.ts
index e255d44a6a9bb5..07460066d132e2 100644
--- a/lib/modules/platform/gerrit/client.ts
+++ b/lib/modules/platform/gerrit/client.ts
@@ -221,16 +221,7 @@ class GerritClient {
const filterState = mapPrStateToGerritFilter(searchConfig.state);
const filters = ['owner:self', 'project:' + repository, filterState];
if (searchConfig.branchName) {
- filters.push(
- ...[
- '(',
- `footer:Renovate-Branch=${searchConfig.branchName}`,
- // for backwards compatibility
- 'OR',
- `hashtag:sourceBranch-${searchConfig.branchName}`,
- ')',
- ],
- );
+ filters.push(`footer:Renovate-Branch=${searchConfig.branchName}`);
}
if (searchConfig.targetBranch) {
filters.push(`branch:${searchConfig.targetBranch}`);
diff --git a/lib/modules/platform/gerrit/readme.md b/lib/modules/platform/gerrit/readme.md
index cdf994cf29fd4a..bb649c28fbbcdb 100644
--- a/lib/modules/platform/gerrit/readme.md
+++ b/lib/modules/platform/gerrit/readme.md
@@ -8,12 +8,6 @@ Support for Gerrit is currently _experimental_, meaning that it _might_ still ha
Renovate stores its metadata in the _commit message footer_.
-Previously Renovate stored metadata in Gerrit's _hashtags_.
-To keep backwards compatibility, Renovate still reads metadata from hashtags.
-But Renovate _always_ puts its metadata in the _commit message footer_!
-When the Renovate maintainers mark Gerrit support as stable, the maintainers will remove the "read metadata from hashtags" feature.
-This means changes without metadata in the commit message footer will be "forgotten" by Renovate.
-
## Authentication
<figure markdown>
diff --git a/lib/modules/platform/gerrit/types.ts b/lib/modules/platform/gerrit/types.ts
index b65038100b3182..07087105d4f6ca 100644
--- a/lib/modules/platform/gerrit/types.ts
+++ b/lib/modules/platform/gerrit/types.ts
@@ -34,10 +34,6 @@ export type GerritReviewersType = 'REVIEWER' | 'CC' | 'REMOVED';
export interface GerritChange {
branch: string;
- /**
- * for backwards compatibility
- */
- hashtags?: string[];
change_id: string;
subject: string;
status: GerritChangeStatus;
diff --git a/lib/modules/platform/gerrit/utils.spec.ts b/lib/modules/platform/gerrit/utils.spec.ts
index b609fdf7a88c0e..9505ed65624f11 100644
--- a/lib/modules/platform/gerrit/utils.spec.ts
+++ b/lib/modules/platform/gerrit/utils.spec.ts
@@ -186,47 +186,6 @@ describe('modules/platform/gerrit/utils', () => {
});
expect(utils.extractSourceBranch(change)).toBe('renovate/dependency-1.x');
});
-
- // for backwards compatibility
- it('no commit message but with hashtags', () => {
- const change = partial<GerritChange>({
- hashtags: ['sourceBranch-renovate/dependency-1.x'],
- });
- expect(utils.extractSourceBranch(change)).toBe('renovate/dependency-1.x');
- });
-
- // for backwards compatibility
- it('commit message with no footer but with hashtags', () => {
- const change = partial<GerritChange>({
- hashtags: ['sourceBranch-renovate/dependency-1.x'],
- current_revision: 'abc',
- revisions: {
- abc: partial<GerritRevisionInfo>({
- commit: {
- message: 'some message...',
- },
- }),
- },
- });
- expect(utils.extractSourceBranch(change)).toBe('renovate/dependency-1.x');
- });
-
- // for backwards compatibility
- it('prefers the footer over the hashtags', () => {
- const change = partial<GerritChange>({
- hashtags: ['sourceBranch-renovate/dependency-1.x'],
- current_revision: 'abc',
- revisions: {
- abc: partial<GerritRevisionInfo>({
- commit: {
- message:
- 'Some change\n\nRenovate-Branch: renovate/dependency-2.x\nChange-Id: ...',
- },
- }),
- },
- });
- expect(utils.extractSourceBranch(change)).toBe('renovate/dependency-2.x');
- });
});
describe('findPullRequestBody()', () => {
diff --git a/lib/modules/platform/gerrit/utils.ts b/lib/modules/platform/gerrit/utils.ts
index 3eb28e4b8ece47..a96debaf7c9b82 100644
--- a/lib/modules/platform/gerrit/utils.ts
+++ b/lib/modules/platform/gerrit/utils.ts
@@ -101,13 +101,6 @@ export function extractSourceBranch(change: GerritChange): string | undefined {
}
}
- // for backwards compatibility
- if (!sourceBranch) {
- sourceBranch = change.hashtags
- ?.find((tag) => tag.startsWith('sourceBranch-'))
- ?.replace('sourceBranch-', '');
- }
-
return sourceBranch ?? undefined;
}