Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gerrit): not auto-merging if Code-Review changed from +2 to +1 #32818

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
46f8eed
fix(gerrit): not auto-approving if change already had a Code-Review +1
felipecrs Nov 29, 2024
107a298
chore(gerrit): improve auto-approve on new patchset workaround
felipecrs Nov 30, 2024
6ac7b9e
chore(gerrit): try auto-approve right before auto-merge
felipecrs Nov 30, 2024
6ffa5b6
Merge branch 'main' into fix-auto-merge-if-plus-one
felipecrs Nov 30, 2024
7877331
Merge branch 'main' of https://github.com/renovatebot/renovate into f…
felipecrs Dec 1, 2024
1d8daf5
test(gerrit): fix approvePr coverage
felipecrs Dec 1, 2024
5eb2846
chore(gerrit): still approve if change was approved another user
felipecrs Dec 1, 2024
c679c28
feat(gitlab,azure): try approving before auto-merge
felipecrs Dec 1, 2024
dbdf41a
chore: add better jsdoc for approvePr
felipecrs Dec 1, 2024
bd05816
test(gerrit): fix lint in client.spec.ts
felipecrs Dec 1, 2024
ac30f2b
test(automerge): add test for auto approve before merging
felipecrs Dec 1, 2024
539c12f
Merge branch 'main' into fix-auto-merge-if-plus-one
felipecrs Dec 2, 2024
7923d5b
Revert "feat(gitlab,azure): try approving before auto-merge"
felipecrs Dec 2, 2024
c4c81e3
chore: rename approvePr to approvePrForAutomerge
felipecrs Dec 2, 2024
4dfc817
Revert "chore(gerrit): still approve if change was approved another u…
felipecrs Dec 2, 2024
bb317ce
chore(gerrit): improve approve method to avoid some API calls
felipecrs Dec 2, 2024
b1ce02b
Merge branch 'main' of https://github.com/renovatebot/renovate into f…
felipecrs Dec 2, 2024
53743b2
chore: fix type import
felipecrs Dec 2, 2024
e0b6f8d
chore: remove spurious comment
felipecrs Dec 2, 2024
74137a8
Merge branch 'main' of https://github.com/renovatebot/renovate into f…
felipecrs Dec 29, 2024
b93ee7a
Address review comments
felipecrs Dec 29, 2024
add6ca5
Add tests for approvePrForAutomerge
felipecrs Dec 29, 2024
933f13d
Merge
felipecrs Jan 17, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 50 additions & 60 deletions lib/modules/platform/gerrit/client.spec.ts
Original file line number Diff line number Diff line change
@@ -399,10 +399,20 @@ describe('modules/platform/gerrit/client', () => {

describe('approveChange()', () => {
it('already approved - do nothing', async () => {
const change = partial<GerritChange>({});
const change = partial<GerritChange>({
labels: {
'Code-Review': {
all: [{ value: 2 }],
},
},
});
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);
await expect(client.approveChange(123456)).toResolve();
});
@@ -411,17 +421,27 @@ describe('modules/platform/gerrit/client', () => {
const change = partial<GerritChange>({ labels: {} });
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);

await expect(client.approveChange(123456)).toResolve();
});

it('not already approved - approve now', async () => {
const change = partial<GerritChange>({ labels: { 'Code-Review': {} } });
const change = partial<GerritChange>({
labels: { 'Code-Review': { all: [] } },
});
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);
const approveMock = httpMock
.scope(gerritEndpointUrl)
@@ -433,62 +453,32 @@ describe('modules/platform/gerrit/client', () => {
await expect(client.approveChange(123456)).toResolve();
expect(approveMock.isDone()).toBeTrue();
});
});

describe('wasApprovedBy()', () => {
it('label not exists', () => {
expect(
client.wasApprovedBy(partial<GerritChange>({}), 'user'),
).toBeUndefined();
});

it('not approved by anyone', () => {
expect(
client.wasApprovedBy(
partial<GerritChange>({
labels: {
'Code-Review': {},
},
}),
'user',
),
).toBeUndefined();
});

it('approved by given user', () => {
expect(
client.wasApprovedBy(
partial<GerritChange>({
labels: {
'Code-Review': {
approved: {
_account_id: 1,
username: 'user',
},
},
},
}),
'user',
),
).toBeTrue();
});

it('approved by given other', () => {
expect(
client.wasApprovedBy(
partial<GerritChange>({
labels: {
'Code-Review': {
approved: {
_account_id: 1,
username: 'other',
},
},
},
}),
'user',
),
).toBeFalse();
it('not already approved because of +1 - approve now', async () => {
const change = partial<GerritChange>({
labels: {
'Code-Review': {
all: [{ value: 1 }],
},
},
});
httpMock
.scope(gerritEndpointUrl)
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);
const approveMock = httpMock
.scope(gerritEndpointUrl)
.post('/a/changes/123456/revisions/current/review', {
labels: { 'Code-Review': +2 },
notify: 'NONE',
})
.reply(200, gerritRestResponse(''), jsonResultHeader);
await expect(client.approveChange(123456)).toResolve();
expect(approveMock.isDone()).toBeTrue();
});
});
});
23 changes: 12 additions & 11 deletions lib/modules/platform/gerrit/client.ts
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@ import { REPOSITORY_ARCHIVED } from '../../../constants/error-messages';
import { logger } from '../../../logger';
import { GerritHttp } from '../../../util/http/gerrit';
import { regEx } from '../../../util/regex';
import { getQueryString } from '../../../util/url';
import type {
GerritAccountInfo,
GerritBranchInfo,
@@ -73,10 +74,14 @@ class GerritClient {
return changes.body;
}

async getChange(changeNumber: number): Promise<GerritChange> {
async getChange(
changeNumber: number,
extraOptions?: string[],
): Promise<GerritChange> {
const options = [...this.requestDetails, ...(extraOptions ?? [])];
const queryString = getQueryString({ o: options });
const changes = await this.gerritHttp.getJsonUnchecked<GerritChange>(
`a/changes/${changeNumber}?` +
this.requestDetails.map((det) => `o=${det}`).join('&'),
`a/changes/${changeNumber}?${queryString}`,
);
return changes.body;
}
@@ -197,15 +202,11 @@ class GerritClient {
}

async checkIfApproved(changeId: number): Promise<boolean> {
const change = await client.getChange(changeId);
const reviewLabels = change?.labels?.['Code-Review'];
return reviewLabels === undefined || reviewLabels.approved !== undefined;
}

wasApprovedBy(change: GerritChange, username: string): boolean | undefined {
const change = await client.getChange(changeId, ['DETAILED_LABELS']);
const reviewLabel = change?.labels?.['Code-Review'];
return (
change.labels?.['Code-Review'].approved &&
change.labels['Code-Review'].approved.username === username
reviewLabel === undefined ||
reviewLabel.all?.some((label) => label.value === 2) === true
);
}

51 changes: 8 additions & 43 deletions lib/modules/platform/gerrit/index.spec.ts
Original file line number Diff line number Diff line change
@@ -187,28 +187,6 @@ describe('modules/platform/gerrit/index', () => {
gerrit.writeToConfig({ labels: {} });
});

it('updatePr() - auto approve enabled', async () => {
const change = partial<GerritChange>({
current_revision: 'some-revision',
revisions: {
'some-revision': partial<GerritRevisionInfo>({
commit: {
message: 'some message',
},
}),
},
});
clientMock.getChange.mockResolvedValueOnce(change);
await gerrit.updatePr({
number: 123456,
prTitle: 'subject',
platformPrOptions: {
autoApprove: true,
},
});
expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
});

it('updatePr() - closed => abandon the change', async () => {
const change = partial<GerritChange>({});
clientMock.getChange.mockResolvedValueOnce(change);
@@ -309,7 +287,7 @@ describe('modules/platform/gerrit/index', () => {
]);
});

it('createPr() - update body WITHOUT approve', async () => {
it('createPr() - update body', async () => {
const pr = await gerrit.createPr({
sourceBranch: 'source',
targetBranch: 'target',
@@ -325,26 +303,6 @@ describe('modules/platform/gerrit/index', () => {
'body',
TAG_PULL_REQUEST_BODY,
);
expect(clientMock.approveChange).not.toHaveBeenCalled();
});

it('createPr() - update body and approve', async () => {
const pr = await gerrit.createPr({
sourceBranch: 'source',
targetBranch: 'target',
prTitle: change.subject,
prBody: 'body',
platformPrOptions: {
autoApprove: true,
},
});
expect(pr).toHaveProperty('number', 123456);
expect(clientMock.addMessageIfNotAlreadyExists).toHaveBeenCalledWith(
123456,
'body',
TAG_PULL_REQUEST_BODY,
);
expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
});
});

@@ -750,6 +708,13 @@ describe('modules/platform/gerrit/index', () => {
//TODO: add some tests for Gerrit-specific replacements..
});

describe('approvePrForAutomerge()', () => {
it('approvePrForAutomerge() - calls approveChange', async () => {
await expect(gerrit.approvePrForAutomerge(123456)).toResolve();
expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
});
});

describe('currently unused/not-implemented functions', () => {
it('deleteLabel()', async () => {
await expect(
17 changes: 11 additions & 6 deletions lib/modules/platform/gerrit/index.ts
Original file line number Diff line number Diff line change
@@ -160,9 +160,6 @@ export async function updatePr(prConfig: UpdatePrConfig): Promise<void> {
TAG_PULL_REQUEST_BODY,
);
}
if (prConfig.platformPrOptions?.autoApprove) {
await client.approveChange(prConfig.number);
}
if (prConfig.state && prConfig.state === 'closed') {
await client.abandonChange(prConfig.number);
}
@@ -195,9 +192,6 @@ export async function createPr(prConfig: CreatePRConfig): Promise<Pr | null> {
prConfig.prBody,
TAG_PULL_REQUEST_BODY,
);
if (prConfig.platformPrOptions?.autoApprove) {
await client.approveChange(pr._number);
}
return getPr(pr._number);
}

@@ -442,3 +436,14 @@ export function findIssue(title: string): Promise<Issue | null> {
export function getIssueList(): Promise<Issue[]> {
return Promise.resolve([]);
}

/**
* The Code-Review +2 vote of when the change was created or updated in Gerrit
* may have been downgraded by a CI check utilizing the same account as
* Renovate (e.g. SonarQube which posts Code-Review +1). This function will
* vote with +2 again on the change, if needed, before Renovate attempt to
* automerge it.
*/
export async function approvePrForAutomerge(number: number): Promise<void> {
await client.approveChange(number);
}
10 changes: 3 additions & 7 deletions lib/modules/platform/gerrit/scm.spec.ts
Original file line number Diff line number Diff line change
@@ -369,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,
@@ -385,6 +384,7 @@ describe('modules/platform/gerrit/scm', () => {
message: 'commit msg',
files: [],
prTitle: 'pr title',
autoApprove: true,
}),
).toBe('commitSha');
expect(git.prepareCommit).toHaveBeenCalledWith({
@@ -396,19 +396,15 @@ 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');
expect(git.pushCommit).toHaveBeenCalledWith({
files: [],
sourceRef: 'renovate/dependency-1.x',
targetRef: 'refs/for/main%notify=NONE',
targetRef: 'refs/for/main%notify=NONE,l=Code-Review+2',
});
expect(clientMock.wasApprovedBy).toHaveBeenCalledWith(
existingChange,
'user',
);
expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
});
});
});
13 changes: 5 additions & 8 deletions lib/modules/platform/gerrit/scm.ts
Original file line number Diff line number Diff line change
@@ -130,19 +130,16 @@ export class GerritScm extends DefaultGitScm {
hasChanges = await git.hasDiff('HEAD', 'FETCH_HEAD'); //avoid empty patchsets
}
if (hasChanges || commit.force) {
const pushOptions = ['notify=NONE'];
if (commit.autoApprove) {
pushOptions.push('l=Code-Review+2');
}
const pushResult = await git.pushCommit({
sourceRef: commit.branchName,
targetRef: `refs/for/${commit.baseBranch!}%notify=NONE`,
targetRef: `refs/for/${commit.baseBranch!}%${pushOptions.join(',')}`,
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)
) {
await client.approveChange(existingChange._number);
}
return commitSha;
}
}
7 changes: 7 additions & 0 deletions lib/modules/platform/gerrit/types.ts
Original file line number Diff line number Diff line change
@@ -79,6 +79,8 @@ export interface GerritLabelInfo {
rejected?: GerritAccountInfo;
/** If true, the label blocks submit operation. If not set, the default is false. */
blocking?: boolean;
/** List of votes. Only set when o=DETAILED_LABELS. */
all?: GerritApprovalInfo[];
}

export interface GerritActionInfo {
@@ -101,3 +103,8 @@ export interface GerritMergeableInfo {
| 'CHERRY_PICK';
mergeable: boolean;
}

export interface GerritApprovalInfo {
value?: number;
username?: string;
}
5 changes: 5 additions & 0 deletions lib/modules/platform/types.ts
Original file line number Diff line number Diff line change
@@ -283,6 +283,11 @@ export interface Platform {

maxBodyLength(): number;
labelCharLimit?(): number;
/**
* Platforms that support `autoApprove` can implement this function. It will be
* invoked during automerge before the PR branch status is checked.
*/
approvePrForAutomerge?(number: number): Promise<void>;
}

export interface PlatformScm {
2 changes: 2 additions & 0 deletions lib/util/git/types.ts
Original file line number Diff line number Diff line change
@@ -86,6 +86,8 @@ export interface CommitFilesConfig {
platformCommit?: PlatformCommitOptions;
/** Only needed by Gerrit platform */
prTitle?: string;
/** Only needed by Gerrit platform */
autoApprove?: boolean;
}

export interface PushFilesConfig {
2 changes: 2 additions & 0 deletions lib/workers/repository/update/branch/commit.ts
Original file line number Diff line number Diff line change
@@ -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,
});
}
12 changes: 12 additions & 0 deletions lib/workers/repository/update/pr/automerge.spec.ts
Original file line number Diff line number Diff line change
@@ -124,6 +124,18 @@ describe('workers/repository/update/pr/automerge', () => {
expect(platform.mergePr).toHaveBeenCalledTimes(0);
});

it('should attempt to auto-approve if supported', async () => {
config.autoApprove = true;
config.automerge = true;
config.pruneBranchAfterAutomerge = true;
platform.getBranchStatus.mockResolvedValueOnce('green');
platform.mergePr.mockResolvedValueOnce(true);
const res = await prAutomerge.checkAutoMerge(pr, config);
expect(res).toEqual({ automerged: true, branchRemoved: true });
expect(platform.approvePrForAutomerge).toHaveBeenCalledTimes(1);
expect(platform.mergePr).toHaveBeenCalledTimes(1);
});

it('should not automerge if enabled and pr is unmergeable', async () => {
config.automerge = true;
scm.isBranchConflicted.mockResolvedValueOnce(true);
4 changes: 4 additions & 0 deletions lib/workers/repository/update/pr/automerge.ts
Original file line number Diff line number Diff line change
@@ -69,6 +69,10 @@ export async function checkAutoMerge(
prAutomergeBlockReason: 'PlatformNotReady',
};
}
if (config.autoApprove && platform.approvePrForAutomerge) {
logger.debug('Auto-approving PR for automerge');
await platform.approvePrForAutomerge(pr.number);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it this a noop if it's already approved? see other open conversation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the PR could have been un-approved after initial approve. See the PR body's Context:

I self-host Renovate and I noticed that auto-merge was not working as expected. I realized it was caused by this issue, because for a given project we have here, the same user/robot also votes the change with a Code-Review +1 vote when SonarQube analysis is successful (which is a very known pattern).

SonarQube analysis in such case happens during CI, after Renovate has initially approved the PR upon creation or update.

This extra-check will account for cases where the PR had its vote downgraded from +2 to +1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a special case that can only happen with Gerrit. On GitHub and Gitlab, there's no such concept of approval level. It's either approved or not, by a given user.

However, in Gerrit, "approvals" can either be +2 (highest, needed to merge), or +1 (not enough to merge). And a user, can have its vote decreased from +2 to +1.

}
const branchStatus = await resolveBranchStatus(
branchName,
!!config.internalChecksAsSuccess,