From f7e4668eff1086ff53cfbb28a477ab2e592e92c9 Mon Sep 17 00:00:00 2001 From: Felipe Santos Date: Tue, 12 Nov 2024 02:54:01 -0300 Subject: [PATCH] fix(gerrit): improve commit message vs pr title workaround (#32115) --- lib/modules/platform/gerrit/client.spec.ts | 33 ------------------- lib/modules/platform/gerrit/client.ts | 19 ----------- lib/modules/platform/gerrit/index.spec.ts | 31 +---------------- lib/modules/platform/gerrit/index.ts | 16 --------- lib/modules/platform/gerrit/readme.md | 12 ------- lib/modules/platform/gerrit/scm.spec.ts | 13 ++++++-- lib/modules/platform/gerrit/scm.ts | 12 +++++-- lib/util/git/types.ts | 2 ++ .../repository/update/branch/commit.ts | 2 ++ 9 files changed, 25 insertions(+), 115 deletions(-) diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts index 21c994e7d46ad4..2d5ff5cddbf9fe 100644 --- a/lib/modules/platform/gerrit/client.spec.ts +++ b/lib/modules/platform/gerrit/client.spec.ts @@ -208,39 +208,6 @@ describe('modules/platform/gerrit/client', () => { }); }); - describe('setCommitMessage()', () => { - it('setCommitMessage', async () => { - const change = partial({}); - httpMock - .scope(gerritEndpointUrl) - .put('/a/changes/123456/message', { message: 'new message' }) - .reply(200, gerritRestResponse(change), jsonResultHeader); - await expect(client.setCommitMessage(123456, 'new message')).toResolve(); - }); - }); - - describe('updateChangeSubject', () => { - it('updateChangeSubject - success', async () => { - const change = partial({}); - const newSubject = 'new subject'; - const previousSubject = 'some subject'; - const previousBody = `some body\n\nChange-Id: some-change-id\n`; - httpMock - .scope(gerritEndpointUrl) - .put('/a/changes/123456/message', { - message: `${newSubject}\n\n${previousBody}`, - }) - .reply(200, gerritRestResponse(change), jsonResultHeader); - await expect( - client.updateChangeSubject( - 123456, - `${previousSubject}\n\n${previousBody}`, - 'new subject', - ), - ).toResolve(); - }); - }); - describe('getMessages()', () => { it('no messages', async () => { httpMock diff --git a/lib/modules/platform/gerrit/client.ts b/lib/modules/platform/gerrit/client.ts index b4aeaa9748bd42..89b4ebf3e66e3c 100644 --- a/lib/modules/platform/gerrit/client.ts +++ b/lib/modules/platform/gerrit/client.ts @@ -98,25 +98,6 @@ class GerritClient { return change.body; } - async setCommitMessage(changeNumber: number, message: string): Promise { - await this.gerritHttp.putJson(`a/changes/${changeNumber}/message`, { - body: { message }, - }); - } - - async updateChangeSubject( - number: number, - currentMessage: string, - newSubject: string, - ): Promise { - // Replace first line of the commit message with the new subject - const newMessage = currentMessage.replace( - new RegExp(`^.*$`, 'm'), - newSubject, - ); - await this.setCommitMessage(number, newMessage); - } - async getMessages(changeNumber: number): Promise { const messages = await this.gerritHttp.getJson( `a/changes/${changeNumber}/messages`, diff --git a/lib/modules/platform/gerrit/index.spec.ts b/lib/modules/platform/gerrit/index.spec.ts index 025f1d1e2e3f7e..85e04bd02be48c 100644 --- a/lib/modules/platform/gerrit/index.spec.ts +++ b/lib/modules/platform/gerrit/index.spec.ts @@ -187,29 +187,6 @@ describe('modules/platform/gerrit/index', () => { gerrit.writeToConfig({ labels: {} }); }); - it('updatePr() - new prTitle => copy to commit msg', async () => { - const oldSubject = 'old title'; - const oldMessage = `${oldSubject}\n\nsome body\n\nChange-Id: ...`; - const change = partial({ - subject: oldSubject, - current_revision: 'some-revision', - revisions: { - 'some-revision': partial({ - commit: { - message: oldMessage, - }, - }), - }, - }); - clientMock.getChange.mockResolvedValueOnce(change); - await gerrit.updatePr({ number: 123456, prTitle: 'new title' }); - expect(clientMock.updateChangeSubject).toHaveBeenCalledWith( - 123456, - oldMessage, - 'new title', - ); - }); - it('updatePr() - auto approve enabled', async () => { const change = partial({ current_revision: 'some-revision', @@ -332,7 +309,7 @@ describe('modules/platform/gerrit/index', () => { ]); }); - it('createPr() - update body/title WITHOUT approve', async () => { + it('createPr() - update body WITHOUT approve', async () => { const pr = await gerrit.createPr({ sourceBranch: 'source', targetBranch: 'target', @@ -349,11 +326,6 @@ describe('modules/platform/gerrit/index', () => { TAG_PULL_REQUEST_BODY, ); expect(clientMock.approveChange).not.toHaveBeenCalled(); - expect(clientMock.updateChangeSubject).toHaveBeenCalledWith( - 123456, - message, - 'title', - ); }); it('createPr() - update body and approve', async () => { @@ -373,7 +345,6 @@ describe('modules/platform/gerrit/index', () => { TAG_PULL_REQUEST_BODY, ); expect(clientMock.approveChange).toHaveBeenCalledWith(123456); - expect(clientMock.setCommitMessage).not.toHaveBeenCalled(); }); }); diff --git a/lib/modules/platform/gerrit/index.ts b/lib/modules/platform/gerrit/index.ts index 20360a761d497d..d4f5b3c8123780 100644 --- a/lib/modules/platform/gerrit/index.ts +++ b/lib/modules/platform/gerrit/index.ts @@ -153,14 +153,6 @@ export async function getPr(number: number): Promise { export async function updatePr(prConfig: UpdatePrConfig): Promise { logger.debug(`updatePr(${prConfig.number}, ${prConfig.prTitle})`); - const change = await client.getChange(prConfig.number); - if (change.subject !== prConfig.prTitle) { - await client.updateChangeSubject( - prConfig.number, - change.revisions[change.current_revision].commit.message, - prConfig.prTitle, - ); - } if (prConfig.prBody) { await client.addMessageIfNotAlreadyExists( prConfig.number, @@ -198,14 +190,6 @@ export async function createPr(prConfig: CreatePRConfig): Promise { `the change should be created automatically from previous push to refs/for/${prConfig.sourceBranch}`, ); } - //Workaround for "Known Problems.1" - if (pr.subject !== prConfig.prTitle) { - await client.updateChangeSubject( - pr._number, - pr.revisions[pr.current_revision].commit.message, - prConfig.prTitle, - ); - } await client.addMessageIfNotAlreadyExists( pr._number, prConfig.prBody, diff --git a/lib/modules/platform/gerrit/readme.md b/lib/modules/platform/gerrit/readme.md index 7246f3f946dc78..cdf994cf29fd4a 100644 --- a/lib/modules/platform/gerrit/readme.md +++ b/lib/modules/platform/gerrit/readme.md @@ -61,15 +61,3 @@ For example, if you want to use the [Merge Confidence](../../../merge-confidence - Creating issues (not a Gerrit concept) - Dependency Dashboard (needs issues first) - -## Known problems - -### PR title is different from first commit message - -Sometimes the PR title passed to the Gerrit platform code is different from the first line of the commit message. -For example: - -- Commit-Message=`Update keycloak.version to v21` -- Pull-Request-Title=`Update keycloak.version to v21 (major)` - -In this case the Gerrit-Platform implementation tries to detect this and change the commit-message in a second patch-set. diff --git a/lib/modules/platform/gerrit/scm.spec.ts b/lib/modules/platform/gerrit/scm.spec.ts index 9be155245ad15e..f2041ef7f365c4 100644 --- a/lib/modules/platform/gerrit/scm.spec.ts +++ b/lib/modules/platform/gerrit/scm.spec.ts @@ -266,6 +266,7 @@ describe('modules/platform/gerrit/scm', () => { baseBranch: 'main', message: 'commit msg', files: [], + prTitle: 'pr title', }), ).resolves.toBeNull(); expect(clientMock.findChanges).toHaveBeenCalledWith( @@ -294,6 +295,7 @@ describe('modules/platform/gerrit/scm', () => { baseBranch: 'main', message: 'commit msg', files: [], + prTitle: 'pr title', }), ).toBe('commitSha'); expect(git.prepareCommit).toHaveBeenCalledWith({ @@ -301,11 +303,12 @@ describe('modules/platform/gerrit/scm', () => { branchName: 'renovate/dependency-1.x', files: [], message: [ - 'commit msg', + 'pr title', expect.stringMatching( /^Renovate-Branch: renovate\/dependency-1\.x\nChange-Id: I[a-z0-9]{40}$/, ), ], + prTitle: 'pr title', force: true, }); expect(git.pushCommit).toHaveBeenCalledWith({ @@ -338,6 +341,7 @@ describe('modules/platform/gerrit/scm', () => { baseBranch: 'main', message: ['commit msg'], files: [], + prTitle: 'pr title', }), ).toBeNull(); expect(git.prepareCommit).toHaveBeenCalledWith({ @@ -345,9 +349,10 @@ describe('modules/platform/gerrit/scm', () => { branchName: 'renovate/dependency-1.x', files: [], message: [ - 'commit msg', + 'pr title', 'Renovate-Branch: renovate/dependency-1.x\nChange-Id: ...', ], + prTitle: 'pr title', force: true, }); expect(git.fetchRevSpec).toHaveBeenCalledWith('refs/changes/1/2'); @@ -379,6 +384,7 @@ describe('modules/platform/gerrit/scm', () => { baseBranch: 'main', message: 'commit msg', files: [], + prTitle: 'pr title', }), ).toBe('commitSha'); expect(git.prepareCommit).toHaveBeenCalledWith({ @@ -386,9 +392,10 @@ describe('modules/platform/gerrit/scm', () => { branchName: 'renovate/dependency-1.x', files: [], message: [ - 'commit msg', + 'pr title', 'Renovate-Branch: renovate/dependency-1.x\nChange-Id: ...', ], + prTitle: 'pr title', force: true, }); expect(git.fetchRevSpec).toHaveBeenCalledWith('refs/changes/1/2'); diff --git a/lib/modules/platform/gerrit/scm.ts b/lib/modules/platform/gerrit/scm.ts index d8500e95205251..6289f1f1f24047 100644 --- a/lib/modules/platform/gerrit/scm.ts +++ b/lib/modules/platform/gerrit/scm.ts @@ -105,10 +105,18 @@ export class GerritScm extends DefaultGitScm { .then((res) => res.pop()); let hasChanges = true; - const origMsg = + const message = typeof commit.message === 'string' ? [commit.message] : commit.message; + + // In Gerrit, the change subject/title is the first line of the commit message + if (commit.prTitle) { + const firstMessageLines = message[0].split('\n'); + firstMessageLines[0] = commit.prTitle; + message[0] = firstMessageLines.join('\n'); + } + commit.message = [ - ...origMsg, + ...message, `Renovate-Branch: ${commit.branchName}\nChange-Id: ${existingChange?.change_id ?? generateChangeId()}`, ]; const commitResult = await git.prepareCommit({ ...commit, force: true }); diff --git a/lib/util/git/types.ts b/lib/util/git/types.ts index 2dd36b64085a12..63544838059dfb 100644 --- a/lib/util/git/types.ts +++ b/lib/util/git/types.ts @@ -83,6 +83,8 @@ export interface CommitFilesConfig { message: string | string[]; force?: boolean; platformCommit?: PlatformCommitOptions; + /** Only needed by Gerrit platform */ + prTitle?: string; } export interface PushFilesConfig { diff --git a/lib/workers/repository/update/branch/commit.ts b/lib/workers/repository/update/branch/commit.ts index aa6c46d9a7be1a..cadafc9ca0e75c 100644 --- a/lib/workers/repository/update/branch/commit.ts +++ b/lib/workers/repository/update/branch/commit.ts @@ -60,5 +60,7 @@ export function commitFilesToBranch( message: config.commitMessage!, force: !!config.forceCommit, platformCommit: config.platformCommit, + // Only needed by Gerrit platform + prTitle: config.prTitle, }); }