Skip to content

Commit 38b16bd

Browse files
felipecrsfelipecrs
committed
fix(gerrit): defer change creation to createPr()
Signed-off-by: Felipe Santos <felipe.santos@ericsson.com>
1 parent 808801d commit 38b16bd

4 files changed

Lines changed: 150 additions & 90 deletions

File tree

‎lib/modules/platform/gerrit/index.spec.ts‎

Lines changed: 90 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import { codeBlock } from 'common-tags';
2-
import { DateTime } from 'luxon';
32
import { REPOSITORY_ARCHIVED } from '../../../constants/error-messages';
43
import type { BranchStatus } from '../../../types';
54
import { repoFingerprint } from '../util';
@@ -39,15 +38,6 @@ vi.mock('./client');
3938
const clientMock = vi.mocked(_client);
4039

4140
describe('modules/platform/gerrit/index', () => {
42-
const t0 = DateTime.fromISO('2025-04-14T16:33:37.000000000', {
43-
zone: 'utc',
44-
}) as DateTime<true>;
45-
46-
beforeAll(() => {
47-
vi.useFakeTimers();
48-
vi.setSystemTime(t0.toMillis());
49-
});
50-
5141
beforeEach(async () => {
5242
hostRules.find.mockReturnValue({
5343
username: 'user',
@@ -257,24 +247,42 @@ describe('modules/platform/gerrit/index', () => {
257247
});
258248

259249
describe('createPr()', () => {
260-
it('createPr() - no existing found => rejects', async () => {
261-
clientMock.findChanges.mockResolvedValueOnce([]);
262-
await expect(
263-
gerrit.createPr({
264-
sourceBranch: 'source',
265-
targetBranch: 'target',
266-
prTitle: 'title',
267-
prBody: 'body',
268-
}),
269-
).rejects.toThrow(
270-
`the change should be created automatically from previous push to refs/for/source`,
250+
it('createPr() - creates change by pushing to refs/for/', async () => {
251+
git.pushCommit.mockResolvedValueOnce(true);
252+
const change = partial<GerritChange>({
253+
_number: 123456,
254+
current_revision: 'some-revision',
255+
revisions: {
256+
'some-revision': partial<GerritRevisionInfo>({
257+
commit_with_footers: 'Renovate-Branch: source',
258+
}),
259+
},
260+
});
261+
clientMock.findChanges.mockResolvedValueOnce([change]);
262+
const pr = await gerrit.createPr({
263+
sourceBranch: 'source',
264+
targetBranch: 'target',
265+
prTitle: 'title',
266+
prBody: 'body',
267+
});
268+
expect(pr).toHaveProperty('number', 123456);
269+
expect(git.pushCommit).toHaveBeenCalledExactlyOnceWith({
270+
sourceRef: 'source',
271+
targetRef: 'refs/for/target',
272+
files: [],
273+
pushOptions: ['notify=NONE'],
274+
});
275+
expect(clientMock.addMessage).toHaveBeenCalledExactlyOnceWith(
276+
123456,
277+
'body',
278+
TAG_PULL_REQUEST_BODY,
271279
);
272280
});
273281

274-
it('createPr() - found existing but not created in the last 5 minutes => rejects', async () => {
282+
it('createPr() - with autoApprove', async () => {
283+
git.pushCommit.mockResolvedValueOnce(true);
275284
const change = partial<GerritChange>({
276285
_number: 123456,
277-
created: t0.minus({ minutes: 6 }).toISO().replace('T', ' '),
278286
current_revision: 'some-revision',
279287
revisions: {
280288
'some-revision': partial<GerritRevisionInfo>({
@@ -283,45 +291,90 @@ describe('modules/platform/gerrit/index', () => {
283291
},
284292
});
285293
clientMock.findChanges.mockResolvedValueOnce([change]);
286-
await expect(
287-
gerrit.createPr({
288-
sourceBranch: 'source',
289-
targetBranch: 'target',
290-
prTitle: 'title',
291-
prBody: 'body',
292-
}),
293-
).rejects.toThrow(/it was not created in the last 5 minutes/);
294+
const pr = await gerrit.createPr({
295+
sourceBranch: 'source',
296+
targetBranch: 'target',
297+
prTitle: 'title',
298+
prBody: 'body',
299+
platformPrOptions: {
300+
autoApprove: true,
301+
},
302+
});
303+
expect(pr).toHaveProperty('number', 123456);
304+
expect(git.pushCommit).toHaveBeenCalledExactlyOnceWith({
305+
sourceRef: 'source',
306+
targetRef: 'refs/for/target',
307+
files: [],
308+
pushOptions: ['notify=NONE', 'label=Code-Review+2'],
309+
});
310+
expect(clientMock.addMessage).toHaveBeenCalledExactlyOnceWith(
311+
123456,
312+
'body',
313+
TAG_PULL_REQUEST_BODY,
314+
);
294315
});
295316

296-
it('createPr() - add body as message', async () => {
317+
it('createPr() - with labels', async () => {
318+
git.pushCommit.mockResolvedValueOnce(true);
297319
const change = partial<GerritChange>({
298320
_number: 123456,
299321
current_revision: 'some-revision',
300-
created: t0.minus({ seconds: 30 }).toISO().replace('T', ' '),
301322
revisions: {
302323
'some-revision': partial<GerritRevisionInfo>({
303324
commit_with_footers: 'Renovate-Branch: source',
304325
}),
305326
},
306-
messages: [],
307327
});
308328
clientMock.findChanges.mockResolvedValueOnce([change]);
309329
const pr = await gerrit.createPr({
310330
sourceBranch: 'source',
311331
targetBranch: 'target',
312332
prTitle: 'title',
313333
prBody: 'body',
314-
platformPrOptions: {
315-
autoApprove: false,
316-
},
334+
labels: ['label1', 'label2'],
317335
});
318336
expect(pr).toHaveProperty('number', 123456);
337+
expect(git.pushCommit).toHaveBeenCalledExactlyOnceWith({
338+
sourceRef: 'source',
339+
targetRef: 'refs/for/target',
340+
files: [],
341+
pushOptions: ['notify=NONE', 'hashtag=label1', 'hashtag=label2'],
342+
});
319343
expect(clientMock.addMessage).toHaveBeenCalledExactlyOnceWith(
320344
123456,
321345
'body',
322346
TAG_PULL_REQUEST_BODY,
323347
);
324348
});
349+
350+
it('createPr() - no change found after push => rejects', async () => {
351+
git.pushCommit.mockResolvedValueOnce(true);
352+
clientMock.findChanges.mockResolvedValueOnce([]);
353+
await expect(
354+
gerrit.createPr({
355+
sourceBranch: 'source',
356+
targetBranch: 'target',
357+
prTitle: 'title',
358+
prBody: 'body',
359+
}),
360+
).rejects.toThrow(
361+
`Could not find the Gerrit change after pushing to refs/for/target`,
362+
);
363+
});
364+
365+
it('createPr() - push fails => rejects', async () => {
366+
git.pushCommit.mockResolvedValueOnce(false);
367+
await expect(
368+
gerrit.createPr({
369+
sourceBranch: 'source',
370+
targetBranch: 'target',
371+
prTitle: 'title',
372+
prBody: 'body',
373+
}),
374+
).rejects.toThrow(
375+
`Failed to push commit to refs/for/target to create Gerrit change`,
376+
);
377+
});
325378
});
326379

327380
describe('getBranchPr()', () => {

‎lib/modules/platform/gerrit/index.ts‎

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import { isTruthy } from '@sindresorhus/is';
2-
import { DateTime } from 'luxon';
32
import { logger } from '../../../logger';
43
import type { BranchStatus } from '../../../types';
54
import { parseJson } from '../../../util/common';
@@ -186,6 +185,34 @@ export async function createPr(prConfig: CreatePRConfig): Promise<Pr | null> {
186185
prConfig.labels?.toString() ?? ''
187186
})`,
188187
);
188+
189+
logger.debug(
190+
`Pushing commit to refs/for/${prConfig.targetBranch} to create Gerrit change`,
191+
);
192+
const pushOptions = ['notify=NONE'];
193+
if (prConfig.platformPrOptions?.autoApprove) {
194+
pushOptions.push('label=Code-Review+2');
195+
}
196+
if (prConfig.labels) {
197+
for (const label of prConfig.labels) {
198+
pushOptions.push(`hashtag=${label}`);
199+
}
200+
}
201+
202+
const pushResult = await git.pushCommit({
203+
sourceRef: prConfig.sourceBranch,
204+
targetRef: `refs/for/${prConfig.targetBranch}`,
205+
files: [],
206+
pushOptions,
207+
});
208+
209+
if (!pushResult) {
210+
throw new Error(
211+
`Failed to push commit to refs/for/${prConfig.targetBranch} to create Gerrit change`,
212+
);
213+
}
214+
215+
// Now find the newly created change
189216
const change = (
190217
await client.findChanges(config.repository!, {
191218
branchName: prConfig.sourceBranch,
@@ -197,14 +224,7 @@ export async function createPr(prConfig: CreatePRConfig): Promise<Pr | null> {
197224
).pop();
198225
if (change === undefined) {
199226
throw new Error(
200-
`the change should be created automatically from previous push to refs/for/${prConfig.sourceBranch}`,
201-
);
202-
}
203-
const created = DateTime.fromISO(change.created.replace(' ', 'T'), {});
204-
const fiveMinutesAgo = DateTime.utc().minus({ minutes: 5 });
205-
if (created < fiveMinutesAgo) {
206-
throw new Error(
207-
`the change should have been created automatically from previous push to refs/for/${prConfig.sourceBranch}, but it was not created in the last 5 minutes (${change.created})`,
227+
`Could not find the Gerrit change after pushing to refs/for/${prConfig.targetBranch}`,
208228
);
209229
}
210230
await client.addMessage(

‎lib/modules/platform/gerrit/scm.spec.ts‎

Lines changed: 6 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,6 @@ describe('modules/platform/gerrit/scm', () => {
310310
parentCommitSha: 'parentSha' as LongCommitSha,
311311
files: [],
312312
});
313-
git.pushCommit.mockResolvedValueOnce(true);
314313

315314
expect(
316315
await gerritScm.commitAndPush({
@@ -334,12 +333,8 @@ describe('modules/platform/gerrit/scm', () => {
334333
prTitle: 'pr title',
335334
force: true,
336335
});
337-
expect(git.pushCommit).toHaveBeenCalledExactlyOnceWith({
338-
files: [],
339-
sourceRef: 'renovate/dependency-1.x',
340-
targetRef: 'refs/for/main',
341-
pushOptions: ['notify=NONE'],
342-
});
336+
// For new changes, push should NOT be called - it will be done by createPr()
337+
expect(git.pushCommit).not.toHaveBeenCalled();
343338
});
344339

345340
it('commitFiles() - create first Patch - auto approve', async () => {
@@ -349,7 +344,6 @@ describe('modules/platform/gerrit/scm', () => {
349344
parentCommitSha: 'parentSha' as LongCommitSha,
350345
files: [],
351346
});
352-
git.pushCommit.mockResolvedValueOnce(true);
353347

354348
expect(
355349
await gerritScm.commitAndPush({
@@ -375,12 +369,8 @@ describe('modules/platform/gerrit/scm', () => {
375369
autoApprove: true,
376370
force: true,
377371
});
378-
expect(git.pushCommit).toHaveBeenCalledExactlyOnceWith({
379-
files: [],
380-
sourceRef: 'renovate/dependency-1.x',
381-
targetRef: 'refs/for/main',
382-
pushOptions: ['notify=NONE', 'label=Code-Review+2'],
383-
});
372+
// For new changes, push should NOT be called - it will be done by createPr()
373+
expect(git.pushCommit).not.toHaveBeenCalled();
384374
});
385375

386376
it('commitFiles() - existing change-set without new changes', async () => {
@@ -484,7 +474,6 @@ describe('modules/platform/gerrit/scm', () => {
484474
parentCommitSha: 'parentSha' as LongCommitSha,
485475
files: [],
486476
});
487-
git.pushCommit.mockResolvedValueOnce(true);
488477

489478
expect(
490479
await gerritScm.commitAndPush({
@@ -512,17 +501,8 @@ describe('modules/platform/gerrit/scm', () => {
512501
force: true,
513502
labels: ['hashtag1', 'hashtag2'],
514503
});
515-
expect(git.pushCommit).toHaveBeenCalledExactlyOnceWith({
516-
files: [],
517-
sourceRef: 'renovate/dependency-1.x',
518-
targetRef: 'refs/for/main',
519-
pushOptions: [
520-
'notify=NONE',
521-
'label=Code-Review+2',
522-
'hashtag=hashtag1',
523-
'hashtag=hashtag2',
524-
],
525-
});
504+
// For new changes, push should NOT be called - it will be done by createPr()
505+
expect(git.pushCommit).not.toHaveBeenCalled();
526506
});
527507

528508
it('commitFiles() - existing change-set with new changes - ensure labels', async () => {

‎lib/modules/platform/gerrit/scm.ts‎

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -145,26 +145,33 @@ export class GerritScm extends DefaultGitScm {
145145
const fetchRefSpec = currentRevision.ref;
146146
await git.fetchRevSpec(fetchRefSpec); // fetch current ChangeSet for git diff
147147
hasChanges = await git.hasDiff('HEAD', 'FETCH_HEAD'); // avoid pushing empty patch sets
148-
}
149-
if (hasChanges || commit.force) {
150-
const pushOptions = ['notify=NONE'];
151-
if (commit.autoApprove) {
152-
pushOptions.push('label=Code-Review+2');
153-
}
154-
if (commit.labels) {
155-
for (const label of commit.labels) {
156-
pushOptions.push(`hashtag=${label}`);
148+
// Only push to refs/for/ when updating an existing change
149+
if (hasChanges || commit.force) {
150+
const pushOptions = ['notify=NONE'];
151+
if (commit.autoApprove) {
152+
pushOptions.push('label=Code-Review+2');
153+
}
154+
if (commit.labels) {
155+
for (const label of commit.labels) {
156+
pushOptions.push(`hashtag=${label}`);
157+
}
158+
}
159+
const pushResult = await git.pushCommit({
160+
sourceRef: commit.branchName,
161+
targetRef: `refs/for/${commit.baseBranch!}`,
162+
files: commit.files,
163+
pushOptions,
164+
});
165+
if (pushResult) {
166+
return commitSha;
157167
}
158168
}
159-
const pushResult = await git.pushCommit({
160-
sourceRef: commit.branchName,
161-
targetRef: `refs/for/${commit.baseBranch!}`,
162-
files: commit.files,
163-
pushOptions,
164-
});
165-
if (pushResult) {
166-
return commitSha;
167-
}
169+
} else {
170+
// The push will be done by createPr() to actually create the Gerrit change
171+
logger.debug(
172+
`Commit prepared for new change on branch ${commit.branchName}, but not pushed to refs/for/ yet`,
173+
);
174+
return commitSha;
168175
}
169176
}
170177
return null; // empty commit, no changes in this Gerrit Change

0 commit comments

Comments
 (0)