From f7e4668eff1086ff53cfbb28a477ab2e592e92c9 Mon Sep 17 00:00:00 2001
From: Felipe Santos <felipecassiors@gmail.com>
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 21c994e7d4..2d5ff5cddb 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<GerritChange>({});
- 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<GerritChange>({});
- 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 b4aeaa9748..89b4ebf3e6 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<void> {
- await this.gerritHttp.putJson(`a/changes/${changeNumber}/message`, {
- body: { message },
- });
- }
-
- async updateChangeSubject(
- number: number,
- currentMessage: string,
- newSubject: string,
- ): Promise<void> {
- // 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<GerritChangeMessageInfo[]> {
const messages = await this.gerritHttp.getJson<GerritChangeMessageInfo[]>(
`a/changes/${changeNumber}/messages`,
diff --git a/lib/modules/platform/gerrit/index.spec.ts b/lib/modules/platform/gerrit/index.spec.ts
index 025f1d1e2e..85e04bd02b 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<GerritChange>({
- subject: oldSubject,
- current_revision: 'some-revision',
- revisions: {
- 'some-revision': partial<GerritRevisionInfo>({
- 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<GerritChange>({
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 20360a761d..d4f5b3c812 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<Pr | null> {
export async function updatePr(prConfig: UpdatePrConfig): Promise<void> {
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<Pr | null> {
`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 7246f3f946..cdf994cf29 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 9be155245a..f2041ef7f3 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 d8500e9520..6289f1f1f2 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 2dd36b6408..6354483805 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 aa6c46d9a7..cadafc9ca0 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,
});
}
--
GitLab