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

felipecrs · 2024-11-29T22:48:30Z

Changes

The checkIfApproved function was not working as expected when someone already had published a Code-Review +1 vote. It was wrongly assuming that the change was approved in such case, while the change should only be approved if the change had a Code-Review +2 vote.

Note that when o=DETAILED_LABELS is set, the change response from Gerrit API no longer contains the content from o=LABELS, meaning it should only ever be used in this case (when looking for the approvals), otherwise it would break existing code.

This fix also ensures that the change will be auto-approved again during the auto-merge process, it case the Code-Review +2 was changed to +1 by some CI check using the same account as Renovate, like SonarQube (which posts Code-Review +1).

Finally, this fix will improve the approval logic for when the change is initially created or updated. It will avoid two API calls in each case.

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).

Documentation (please check one with an [x])

I have updated the documentation, or
No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

Code inspection only, or
Newly added/modified unit tests, or
No unit tests but ran on a real repository, or
Both unit tests + ran on a real repository

viceice

conflicted

…ix-auto-merge-if-plus-one

felipecrs · 2024-12-01T15:05:07Z

I just realized this function is also implemented by the GitLab provider, and there it doesn't check for whether it's approved already or not, meaning it will probably issue an error, which is not desired.

I'll think about it.

felipecrs · 2024-12-01T16:43:01Z

@viceice, @rarkins, can you please help me understand this failed check?

❌ Error: Snapshots have been added in this PR. Use toMatch instead.

felipecrs · 2024-12-01T16:46:40Z

I've refactored the other two platforms (Azure and GitLab) that also use autoApprove to account for these improvements as well:

As last resort, the PR will be attempted to be auto-approved before auto-merge, in case the approval got dismissed after it was initially published for whatever reason.
It will no longer attempt to auto-approve if the PR is already approved

Note I do not use Azure or GitLab. I coded these functions based on their documentation. But I believe there is no big risk involved, given it will still attempt to auto-approve in case the is approved condition doesn't match.

felipecrs · 2024-12-01T17:09:52Z

I've retitled the PR to reflect the broader impact of it.

rarkins · 2024-12-02T14:35:53Z

lib/workers/repository/update/pr/automerge.ts

+  if (config.autoApprove && platform.approvePr) {
+    logger.debug('Auto-approving PR if needed before automerge');
+    await platform.approvePr(pr.number);
+  }


Will this cause each PR to be approved on each run? i.e. one extra API call per PR per run?

Will this cause each PR to be approved on each run?

Only if the PR is not yet approved. I implemented all approvePr functions to account for this case.

i.e. one extra API call per PR per run?

Per auto-merge attempt, there will be one extra API call to verify whether the PR is not yet approved, and one more API call to approve it in case it's not yet approved.

Just remember:

this is only applicable for azure, gitlab, and gerrit

and for people who have autoApprove: true

and all other conditions in automerge.ts already passed (except branchStatus, which depends on the approval to be green)

But if you believe it's not a good idea, I can find another way.

Possibly it could be optimized so that it doesn't perform this "is the PR already approved?" check right after we create the PR? i.e. because we know it's impossible that we approved it already.

lib/modules/platform/gitlab/index.ts

rarkins · 2024-12-02T16:32:17Z

lib/workers/repository/update/pr/automerge.ts

+  if (config.autoApprove && platform.approvePr) {
+    logger.debug('Auto-approving PR if needed before automerge');
+    await platform.approvePr(pr.number);
+  }


Possibly it could be optimized so that it doesn't perform this "is the PR already approved?" check right after we create the PR? i.e. because we know it's impossible that we approved it already.

felipecrs · 2024-12-02T17:02:11Z

@rarkins, it's presumptuous from my part that this would be of any help to the existing platforms. I will revert the changes to Azure and GitLab, and focus on Gerrit, which is what I use and know this would make sense for.

This reverts commit c679c28.

…ser" This reverts commit 5eb2846.

…ix-auto-merge-if-plus-one

viceice · 2024-12-06T09:16:02Z

lib/modules/platform/gerrit/client.ts

    const changes = await this.gerritHttp.getJson<GerritChange>(
-      `a/changes/${changeNumber}?` +
-        this.requestDetails.map((det) => `o=${det}`).join('&'),
+      `a/changes/${changeNumber}?` + options.map((det) => `o=${det}`).join('&'),


use getQueryString for proper quoting

renovate/lib/util/url.ts

Line 72 in 2f49607

export function getQueryString(params: Record<string, any>): string {

viceice · 2024-12-06T09:17:21Z

lib/modules/platform/gerrit/client.ts

-      change.labels?.['Code-Review'].approved &&
-      change.labels['Code-Review'].approved.username === username
+      reviewLabel === undefined ||
+      Boolean(reviewLabel.all?.some((label) => label.value === 2))


Suggested change

Boolean(reviewLabel.all?.some((label) => label.value === 2))

reviewLabel.all?.some((label) => label.value === 2) === true

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

viceice · 2024-12-06T09:19:22Z

lib/workers/repository/update/pr/automerge.spec.ts

+      config.autoApprove = true;
+      config.automerge = true;
+      config.pruneBranchAfterAutomerge = true;
+      platform.approvePrForAutomerge.mockResolvedValueOnce();


Suggested change

platform.approvePrForAutomerge.mockResolvedValueOnce();

not needed, default for a mock

lib/modules/platform/gerrit/scm.ts

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 5b42180 to 4028843 Compare November 29, 2024 23:14

felipecrs marked this pull request as draft November 29, 2024 23:33

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 4028843 to 06ecde1 Compare November 29, 2024 23:53

felipecrs marked this pull request as ready for review November 29, 2024 23:53

fix(gerrit): not auto-approving if change already had a Code-Review +1

46f8eed

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 06ecde1 to 46f8eed Compare November 29, 2024 23:58

chore(gerrit): improve auto-approve on new patchset workaround

107a298

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 82fe8e1 to 107a298 Compare November 30, 2024 01:52

felipecrs marked this pull request as draft November 30, 2024 06:18

chore(gerrit): try auto-approve right before auto-merge

6ac7b9e

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from dcc53d7 to 6ac7b9e Compare November 30, 2024 07:12

Merge branch 'main' into fix-auto-merge-if-plus-one

6ffa5b6

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 4817e82 to 6ffa5b6 Compare November 30, 2024 07:18

felipecrs marked this pull request as ready for review November 30, 2024 07:24

rarkins previously approved these changes Nov 30, 2024

View reviewed changes

viceice reviewed Dec 1, 2024

View reviewed changes

Merge branch 'main' of https://github.com/renovatebot/renovate into f…

7877331

…ix-auto-merge-if-plus-one

felipecrs dismissed rarkins’s stale review via 7877331 December 1, 2024 14:59

felipecrs marked this pull request as draft December 1, 2024 15:29

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 56bd600 to 761106b Compare December 1, 2024 16:38

test(gerrit): fix approvePr coverage

1d8daf5

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 761106b to 45763e9 Compare December 1, 2024 16:40

felipecrs marked this pull request as ready for review December 1, 2024 16:43

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 45763e9 to 5a0955c Compare December 1, 2024 16:49

felipecrs marked this pull request as draft December 1, 2024 16:51

chore(gerrit): still approve if change was approved another user

5eb2846

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 5a0955c to ab9907a Compare December 1, 2024 17:06

felipecrs marked this pull request as ready for review December 1, 2024 17:06

felipecrs requested review from rarkins and viceice December 1, 2024 17:07

felipecrs changed the title ~~fix(gerrit): not auto-approving if change already had a Code-Review +1~~ feat(autoApprove): approve change before auto-merging if not already approved Dec 1, 2024

felipecrs changed the title ~~feat(autoApprove): approve change before auto-merging if not already approved~~ feat(autoApprove): auto-approve PR before auto-merge attempt if needed Dec 1, 2024

feat(gitlab,azure): try approving before auto-merge

c679c28

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from ab9907a to c679c28 Compare December 1, 2024 17:17

felipecrs added 4 commits December 1, 2024 17:27

chore: add better jsdoc for approvePr

dbdf41a

test(gerrit): fix lint in client.spec.ts

bd05816

test(automerge): add test for auto approve before merging

ac30f2b

Merge branch 'main' into fix-auto-merge-if-plus-one

539c12f

rarkins reviewed Dec 2, 2024

View reviewed changes

rarkins requested changes Dec 2, 2024

View reviewed changes

felipecrs marked this pull request as draft December 2, 2024 17:02

felipecrs added 4 commits December 2, 2024 17:19

Revert "feat(gitlab,azure): try approving before auto-merge"

7923d5b

This reverts commit c679c28.

chore: rename approvePr to approvePrForAutomerge

c4c81e3

Revert "chore(gerrit): still approve if change was approved another u…

4dfc817

…ser" This reverts commit 5eb2846.

chore(gerrit): improve approve method to avoid some API calls

bb317ce

felipecrs changed the title ~~feat(autoApprove): auto-approve PR before auto-merge attempt if needed~~ fix(gerrit): not auto-merging if Code-Review vote was downgraded Dec 2, 2024

felipecrs changed the title ~~fix(gerrit): not auto-merging if Code-Review vote was downgraded~~ fix(gerrit): not auto-merging if Code-Review changed from +2 to +1 Dec 2, 2024

felipecrs added 3 commits December 2, 2024 17:36

Merge branch 'main' of https://github.com/renovatebot/renovate into f…

b1ce02b

…ix-auto-merge-if-plus-one

chore: fix type import

53743b2

chore: remove spurious comment

e0b6f8d

felipecrs requested a review from rarkins December 2, 2024 18:32

felipecrs marked this pull request as ready for review December 2, 2024 18:32

viceice reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

felipecrs commented Nov 29, 2024 •

edited

Loading

viceice left a comment

felipecrs commented Dec 1, 2024

felipecrs commented Dec 1, 2024

felipecrs commented Dec 1, 2024 •

edited

Loading

felipecrs commented Dec 1, 2024

rarkins Dec 2, 2024

felipecrs Dec 2, 2024 •

edited

Loading

felipecrs Dec 2, 2024

rarkins Dec 2, 2024

rarkins Dec 2, 2024

felipecrs commented Dec 2, 2024

viceice Dec 6, 2024

viceice Dec 6, 2024

viceice Dec 6, 2024

	Boolean(reviewLabel.all?.some((label) => label.value === 2))
	reviewLabel.all?.some((label) => label.value === 2) === true

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

Are you sure you want to change the base?

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

Conversation

felipecrs commented Nov 29, 2024 • edited Loading

Changes

Context

Documentation (please check one with an [x])

How I've tested my work (please select one)

viceice left a comment

Choose a reason for hiding this comment

felipecrs commented Dec 1, 2024

felipecrs commented Dec 1, 2024

felipecrs commented Dec 1, 2024 • edited Loading

felipecrs commented Dec 1, 2024

rarkins Dec 2, 2024

Choose a reason for hiding this comment

felipecrs Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

felipecrs Dec 2, 2024

Choose a reason for hiding this comment

rarkins Dec 2, 2024

Choose a reason for hiding this comment

rarkins Dec 2, 2024

Choose a reason for hiding this comment

felipecrs commented Dec 2, 2024

viceice Dec 6, 2024

Choose a reason for hiding this comment

viceice Dec 6, 2024

Choose a reason for hiding this comment

viceice Dec 6, 2024

Choose a reason for hiding this comment

felipecrs commented Nov 29, 2024 •

edited

Loading

felipecrs commented Dec 1, 2024 •

edited

Loading

felipecrs Dec 2, 2024 •

edited

Loading