From 107a298054e91b67c3cbb68cff8a713ce546281a Mon Sep 17 00:00:00 2001 From: Felipe Santos Date: Sat, 30 Nov 2024 01:48:22 +0000 Subject: [PATCH] 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({}), 'user'), - ).toBeFalse(); - }); - - it('not approved by anyone', () => { - expect( - client.wasApprovedBy( - partial({ - labels: { - 'Code-Review': { - all: [], - }, - }, - }), - 'user', - ), - ).toBeFalse(); - }); - - it('not approved but with +1', () => { - expect( - client.wasApprovedBy( - partial({ - labels: { - 'Code-Review': { - all: [{ value: 1, username: 'user' }], - }, - }, - }), - 'user', - ), - ).toBeFalse(); - }); - - it('approved by given user', () => { - expect( - client.wasApprovedBy( - partial({ - labels: { - 'Code-Review': { - all: [{ value: 2, username: 'user' }], - }, - }, - }), - 'user', - ), - ).toBeTrue(); - }); - - it('approved by given other', () => { - expect( - client.wasApprovedBy( - partial({ - 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 { const filters = GerritClient.buildSearchFilters(repository, findPRConfig); - const options = [...this.requestDetails, ...(extraOptions ?? [])]; const changes = await this.gerritHttp.getJson( `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, }); }