Skip to content

Commit 69d72c2

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

File tree

4 files changed

+152
-166
lines changed

4 files changed

+152
-166
lines changed

‎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: 8 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -277,8 +277,8 @@ describe('modules/platform/gerrit/scm', () => {
277277
});
278278
});
279279

280-
describe('commitFiles()', () => {
281-
it('commitFiles() - empty commit', async () => {
280+
describe('commitAndPush()', () => {
281+
it('commitAndPush() - empty commit', async () => {
282282
clientMock.findChanges.mockResolvedValueOnce([]);
283283
git.prepareCommit.mockResolvedValueOnce(null); //empty commit
284284

@@ -303,14 +303,13 @@ describe('modules/platform/gerrit/scm', () => {
303303
);
304304
});
305305

306-
it('commitFiles() - create first Patch', async () => {
306+
it('commitAndPush() - create first commit', async () => {
307307
clientMock.findChanges.mockResolvedValueOnce([]);
308308
git.prepareCommit.mockResolvedValueOnce({
309309
commitSha: 'commitSha' as LongCommitSha,
310310
parentCommitSha: 'parentSha' as LongCommitSha,
311311
files: [],
312312
});
313-
git.pushCommit.mockResolvedValueOnce(true);
314313

315314
expect(
316315
await gerritScm.commitAndPush({
@@ -334,56 +333,11 @@ 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

345-
it('commitFiles() - create first Patch - auto approve', async () => {
346-
clientMock.findChanges.mockResolvedValueOnce([]);
347-
git.prepareCommit.mockResolvedValueOnce({
348-
commitSha: 'commitSha' as LongCommitSha,
349-
parentCommitSha: 'parentSha' as LongCommitSha,
350-
files: [],
351-
});
352-
git.pushCommit.mockResolvedValueOnce(true);
353-
354-
expect(
355-
await gerritScm.commitAndPush({
356-
branchName: 'renovate/dependency-1.x',
357-
baseBranch: 'main',
358-
message: 'commit msg',
359-
files: [],
360-
prTitle: 'pr title',
361-
autoApprove: true,
362-
}),
363-
).toBe('commitSha');
364-
expect(git.prepareCommit).toHaveBeenCalledExactlyOnceWith({
365-
baseBranch: 'main',
366-
branchName: 'renovate/dependency-1.x',
367-
files: [],
368-
message: [
369-
'pr title',
370-
expect.stringMatching(
371-
/^Renovate-Branch: renovate\/dependency-1\.x\nChange-Id: I[a-z0-9]{40}$/,
372-
),
373-
],
374-
prTitle: 'pr title',
375-
autoApprove: true,
376-
force: true,
377-
});
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-
});
384-
});
385-
386-
it('commitFiles() - existing change-set without new changes', async () => {
340+
it('commitAndPush() - existing change without new changes', async () => {
387341
const existingChange = partial<GerritChange>({
388342
change_id: '...',
389343
current_revision: 'commitSha',
@@ -426,7 +380,7 @@ describe('modules/platform/gerrit/scm', () => {
426380
expect(git.pushCommit).toHaveBeenCalledTimes(0);
427381
});
428382

429-
it('commitFiles() - existing change-set with new changes - auto-approve again', async () => {
383+
it('commitAndPush() - existing change with new changes - auto-approve', async () => {
430384
const existingChange = partial<GerritChange>({
431385
_number: 123456,
432386
change_id: '...',
@@ -477,55 +431,7 @@ describe('modules/platform/gerrit/scm', () => {
477431
});
478432
});
479433

480-
it('commitFiles() - create first patch - with labels', async () => {
481-
clientMock.findChanges.mockResolvedValueOnce([]);
482-
git.prepareCommit.mockResolvedValueOnce({
483-
commitSha: 'commitSha' as LongCommitSha,
484-
parentCommitSha: 'parentSha' as LongCommitSha,
485-
files: [],
486-
});
487-
git.pushCommit.mockResolvedValueOnce(true);
488-
489-
expect(
490-
await gerritScm.commitAndPush({
491-
branchName: 'renovate/dependency-1.x',
492-
baseBranch: 'main',
493-
message: 'commit msg',
494-
files: [],
495-
prTitle: 'pr title',
496-
autoApprove: true,
497-
labels: ['hashtag1', 'hashtag2'],
498-
}),
499-
).toBe('commitSha');
500-
expect(git.prepareCommit).toHaveBeenCalledExactlyOnceWith({
501-
baseBranch: 'main',
502-
branchName: 'renovate/dependency-1.x',
503-
files: [],
504-
message: [
505-
'pr title',
506-
expect.stringMatching(
507-
/^Renovate-Branch: renovate\/dependency-1\.x\nChange-Id: I[a-z0-9]{40}$/,
508-
),
509-
],
510-
prTitle: 'pr title',
511-
autoApprove: true,
512-
force: true,
513-
labels: ['hashtag1', 'hashtag2'],
514-
});
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-
});
526-
});
527-
528-
it('commitFiles() - existing change-set with new changes - ensure labels', async () => {
434+
it('commitAndPush() - existing change with new changes - ensure labels', async () => {
529435
const existingChange = partial<GerritChange>({
530436
_number: 123456,
531437
change_id: '...',

0 commit comments

Comments
 (0)