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

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

@@ -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();
     });
   });
 });

lib/modules/platform/gerrit/client.ts

@@ -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,
@@ -72,10 +73,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.getJson<GerritChange>(
-      `a/changes/${changeNumber}?` +
-        this.requestDetails.map((det) => `o=${det}`).join('&'),
+      `a/changes/${changeNumber}?${queryString}`,
     );
     return changes.body;
   }
@@ -196,15 +201,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
     );
   }
 

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

@@ -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(

lib/modules/platform/gerrit/index.ts

@@ -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);
+}

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

@@ -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);
     });
   });
 });

lib/modules/platform/gerrit/scm.ts

@@ -129,19 +129,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;
         }
       }

lib/modules/platform/gerrit/types.ts

@@ -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;
+}

lib/modules/platform/types.ts

@@ -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 {