diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts index 4be03034242475..a451bd020d9d46 100644 --- a/lib/modules/platform/gerrit/client.spec.ts +++ b/lib/modules/platform/gerrit/client.spec.ts @@ -399,10 +399,20 @@ describe('modules/platform/gerrit/client', () => { describe('approveChange()', () => { it('already approved - do nothing', async () => { - const change = partial({}); + const change = partial({ + 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(); }); @@ -411,17 +421,27 @@ describe('modules/platform/gerrit/client', () => { const change = partial({ 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({ labels: { 'Code-Review': {} } }); + const change = partial({ + labels: { 'Code-Review': { all: [] } }, + }); 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) @@ -433,62 +453,32 @@ describe('modules/platform/gerrit/client', () => { await expect(client.approveChange(123456)).toResolve(); expect(approveMock.isDone()).toBeTrue(); }); - }); - - describe('wasApprovedBy()', () => { - it('label not exists', () => { - expect( - client.wasApprovedBy(partial({}), 'user'), - ).toBeUndefined(); - }); - - it('not approved by anyone', () => { - expect( - client.wasApprovedBy( - partial({ - labels: { - 'Code-Review': {}, - }, - }), - 'user', - ), - ).toBeUndefined(); - }); - it('approved by given user', () => { - expect( - client.wasApprovedBy( - partial({ - labels: { - 'Code-Review': { - approved: { - _account_id: 1, - username: 'user', - }, - }, - }, - }), - 'user', - ), - ).toBeTrue(); - }); - - it('approved by given other', () => { - expect( - client.wasApprovedBy( - partial({ - labels: { - 'Code-Review': { - approved: { - _account_id: 1, - username: 'other', - }, - }, - }, - }), - 'user', - ), - ).toBeFalse(); + it('not already approved because of +1 - approve now', async () => { + const change = partial({ + labels: { + 'Code-Review': { + all: [{ value: 1 }], + }, + }, + }); + 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 e058f0b703f791..bba67df32d87e9 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, @@ -73,10 +74,14 @@ class GerritClient { return changes.body; } - async getChange(changeNumber: number): Promise { + async getChange( + changeNumber: number, + extraOptions?: string[], + ): Promise { + const options = [...this.requestDetails, ...(extraOptions ?? [])]; + const queryString = getQueryString({ o: options }); const changes = await this.gerritHttp.getJsonUnchecked( - `a/changes/${changeNumber}?` + - this.requestDetails.map((det) => `o=${det}`).join('&'), + `a/changes/${changeNumber}?${queryString}`, ); return changes.body; } @@ -197,15 +202,11 @@ class GerritClient { } async checkIfApproved(changeId: number): Promise { - const change = await client.getChange(changeId); - const reviewLabels = change?.labels?.['Code-Review']; - return reviewLabels === undefined || reviewLabels.approved !== undefined; - } - - wasApprovedBy(change: GerritChange, username: string): boolean | undefined { + const change = await client.getChange(changeId, ['DETAILED_LABELS']); + const reviewLabel = change?.labels?.['Code-Review']; return ( - change.labels?.['Code-Review'].approved && - change.labels['Code-Review'].approved.username === username + reviewLabel === undefined || + reviewLabel.all?.some((label) => label.value === 2) === true ); } diff --git a/lib/modules/platform/gerrit/index.spec.ts b/lib/modules/platform/gerrit/index.spec.ts index 3b9c456f1852e4..17fa9e54deb747 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({ - current_revision: 'some-revision', - revisions: { - 'some-revision': partial({ - 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({}); 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); }); }); @@ -750,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( diff --git a/lib/modules/platform/gerrit/index.ts b/lib/modules/platform/gerrit/index.ts index 234d9965631f56..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 { 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 { prConfig.prBody, TAG_PULL_REQUEST_BODY, ); - if (prConfig.platformPrOptions?.autoApprove) { - await client.approveChange(pr._number); - } return getPr(pr._number); } @@ -442,3 +436,14 @@ export function findIssue(title: string): Promise { export function getIssueList(): Promise { return Promise.resolve([]); } + +/** + * 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 + * vote with +2 again on the change, if needed, before Renovate attempt to + * automerge it. + */ +export async function approvePrForAutomerge(number: number): Promise { + await client.approveChange(number); +} diff --git a/lib/modules/platform/gerrit/scm.spec.ts b/lib/modules/platform/gerrit/scm.spec.ts index b82661014934af..1d50b6e43ef069 100644 --- a/lib/modules/platform/gerrit/scm.spec.ts +++ b/lib/modules/platform/gerrit/scm.spec.ts @@ -369,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, @@ -385,6 +384,7 @@ describe('modules/platform/gerrit/scm', () => { message: 'commit msg', files: [], prTitle: 'pr title', + autoApprove: true, }), ).toBe('commitSha'); expect(git.prepareCommit).toHaveBeenCalledWith({ @@ -396,19 +396,15 @@ 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'); 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.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 1e95a2a55af44c..d0966fac380a2b 100644 --- a/lib/modules/platform/gerrit/scm.ts +++ b/lib/modules/platform/gerrit/scm.ts @@ -130,19 +130,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) { - //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) - ) { - await client.approveChange(existingChange._number); - } return commitSha; } } diff --git a/lib/modules/platform/gerrit/types.ts b/lib/modules/platform/gerrit/types.ts index a6ddf07fb4ab3f..b65038100b3182 100644 --- a/lib/modules/platform/gerrit/types.ts +++ b/lib/modules/platform/gerrit/types.ts @@ -79,6 +79,8 @@ export interface GerritLabelInfo { rejected?: GerritAccountInfo; /** If true, the label blocks submit operation. If not set, the default is false. */ blocking?: boolean; + /** List of votes. Only set when o=DETAILED_LABELS. */ + all?: GerritApprovalInfo[]; } export interface GerritActionInfo { @@ -101,3 +103,8 @@ export interface GerritMergeableInfo { | 'CHERRY_PICK'; mergeable: boolean; } + +export interface GerritApprovalInfo { + value?: number; + username?: string; +} diff --git a/lib/modules/platform/types.ts b/lib/modules/platform/types.ts index 81a8a84994fcb7..08384988355e6b 100644 --- a/lib/modules/platform/types.ts +++ b/lib/modules/platform/types.ts @@ -283,6 +283,11 @@ export interface Platform { maxBodyLength(): number; labelCharLimit?(): number; + /** + * Platforms that support `autoApprove` can implement this function. It will be + * invoked during automerge before the PR branch status is checked. + */ + approvePrForAutomerge?(number: number): Promise; } export interface PlatformScm { diff --git a/lib/util/git/types.ts b/lib/util/git/types.ts index 91cf358eccb795..58a66b4106e5c6 100644 --- a/lib/util/git/types.ts +++ b/lib/util/git/types.ts @@ -86,6 +86,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, }); } diff --git a/lib/workers/repository/update/pr/automerge.spec.ts b/lib/workers/repository/update/pr/automerge.spec.ts index 71d389ad126816..b89690d773784d 100644 --- a/lib/workers/repository/update/pr/automerge.spec.ts +++ b/lib/workers/repository/update/pr/automerge.spec.ts @@ -124,6 +124,18 @@ 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.getBranchStatus.mockResolvedValueOnce('green'); + platform.mergePr.mockResolvedValueOnce(true); + const res = await prAutomerge.checkAutoMerge(pr, config); + expect(res).toEqual({ automerged: true, branchRemoved: true }); + expect(platform.approvePrForAutomerge).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); diff --git a/lib/workers/repository/update/pr/automerge.ts b/lib/workers/repository/update/pr/automerge.ts index 2ace410741b3b8..255f185cd6a23e 100644 --- a/lib/workers/repository/update/pr/automerge.ts +++ b/lib/workers/repository/update/pr/automerge.ts @@ -69,6 +69,10 @@ export async function checkAutoMerge( prAutomergeBlockReason: 'PlatformNotReady', }; } + if (config.autoApprove && platform.approvePrForAutomerge) { + logger.debug('Auto-approving PR for automerge'); + await platform.approvePrForAutomerge(pr.number); + } const branchStatus = await resolveBranchStatus( branchName, !!config.internalChecksAsSuccess,