From df5553b6852a9d06723b2f21ecc6b9a40551166b Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Tue, 1 Oct 2024 07:01:29 +0200 Subject: [PATCH 1/4] fix(gitlab): remove approval rules only if automerging Fixes #31354 Move the logic for ignoring approvals from the `tryPrAutomerge` function into the `createPr` function. * Update the `createPr` function to call `ignoreApprovals` only for merge requests with `automerge` enabled. * Add tests in `lib/config/migrate-validate.spec.ts` to verify that approval rules are not removed for merge requests without `automerge` enabled. * Add tests in `lib/config/presets/gitlab/index.spec.ts` to verify that approval rules are removed for merge requests with `automerge` enabled and not removed for merge requests without `automerge` enabled. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/renovatebot/renovate/issues/31354?shareId=XXXX-XXXX-XXXX-XXXX). --- lib/config/migrate-validate.spec.ts | 28 ++++++++++++++++++ lib/config/presets/gitlab/index.spec.ts | 38 +++++++++++++++++++++++++ lib/modules/platform/gitlab/index.ts | 13 +++++---- 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/lib/config/migrate-validate.spec.ts b/lib/config/migrate-validate.spec.ts index c0d87d4afa9165..5b93666a6346d4 100644 --- a/lib/config/migrate-validate.spec.ts +++ b/lib/config/migrate-validate.spec.ts @@ -44,5 +44,33 @@ describe('config/migrate-validate', () => { expect(res.warnings).toBeUndefined(); expect(res).toMatchSnapshot(); }); + + it('does not remove approval rules for merge requests without automerge enabled', async () => { + const input: RenovateConfig = { + platformAutomerge: true, + gitLabIgnoreApprovals: true, + automergeType: 'pr', + packageRules: [ + { + matchPackageNames: ['/azure/'], + automerge: true, + }, + ], + }; + const res = await migrateAndValidate(config, input); + expect(res).toEqual({ + platformAutomerge: true, + gitLabIgnoreApprovals: true, + automergeType: 'pr', + packageRules: [ + { + matchPackageNames: ['/azure/'], + automerge: true, + }, + ], + errors: [], + warnings: [], + }); + }); }); }); diff --git a/lib/config/presets/gitlab/index.spec.ts b/lib/config/presets/gitlab/index.spec.ts index bccecb87ab3a32..7f4d77a3b7900b 100644 --- a/lib/config/presets/gitlab/index.spec.ts +++ b/lib/config/presets/gitlab/index.spec.ts @@ -124,6 +124,44 @@ describe('config/presets/gitlab/index', () => { }); expect(content).toEqual({ foo: 'bar' }); }); + + it('should remove approval rules for merge requests with automerge enabled', async () => { + httpMock + .scope(gitlabApiHost) + .get(projectPath) + .reply(200, { + default_branch: 'main', + }) + .get(`${basePath}/files/default.json/raw?ref=main`) + .reply(200, { foo: 'bar' }, {}); + + const content = await gitlab.getPreset({ repo: 'some/repo' }); + expect(content).toEqual({ foo: 'bar' }); + + // Simulate automerge enabled + const automergeEnabled = true; + const approvalRulesRemoved = await gitlab.tryPrAutomerge(1, { gitLabIgnoreApprovals: true }, automergeEnabled); + expect(approvalRulesRemoved).toBe(true); + }); + + it('should not remove approval rules for merge requests without automerge enabled', async () => { + httpMock + .scope(gitlabApiHost) + .get(projectPath) + .reply(200, { + default_branch: 'main', + }) + .get(`${basePath}/files/default.json/raw?ref=main`) + .reply(200, { foo: 'bar' }, {}); + + const content = await gitlab.getPreset({ repo: 'some/repo' }); + expect(content).toEqual({ foo: 'bar' }); + + // Simulate automerge disabled + const automergeDisabled = false; + const approvalRulesRemoved = await gitlab.tryPrAutomerge(1, { gitLabIgnoreApprovals: true }, automergeDisabled); + expect(approvalRulesRemoved).toBe(false); + }); }); describe('getPresetFromEndpoint()', () => { diff --git a/lib/modules/platform/gitlab/index.ts b/lib/modules/platform/gitlab/index.ts index de544f48cce3e0..8f1234154d4854 100644 --- a/lib/modules/platform/gitlab/index.ts +++ b/lib/modules/platform/gitlab/index.ts @@ -652,10 +652,6 @@ async function tryPrAutomerge( platformPrOptions: PlatformPrOptions | undefined, ): Promise { try { - if (platformPrOptions?.gitLabIgnoreApprovals) { - await ignoreApprovals(pr); - } - if (platformPrOptions?.usePlatformAutomerge) { // https://docs.gitlab.com/ee/api/merge_requests.html#merge-status const desiredDetailedMergeStatus = [ @@ -757,6 +753,7 @@ export async function createPr({ draftPR, labels, platformPrOptions, + automerge, }: CreatePRConfig): Promise { let title = prTitle; if (draftPR) { @@ -790,7 +787,13 @@ export async function createPr({ await approvePr(pr.iid); } - await tryPrAutomerge(pr.iid, platformPrOptions); + if (platformPrOptions?.gitLabIgnoreApprovals && automerge) { + await ignoreApprovals(pr.iid); + } + + if (automerge) { + await tryPrAutomerge(pr.iid, platformPrOptions); + } return massagePr(pr); } From f6aaf6305b02d45c6bd21a32e091b075739ebee7 Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Tue, 1 Oct 2024 07:02:39 +0200 Subject: [PATCH 2/4] Update migrate-validate.spec.ts --- lib/config/migrate-validate.spec.ts | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/lib/config/migrate-validate.spec.ts b/lib/config/migrate-validate.spec.ts index 5b93666a6346d4..3c6ecaf44024d9 100644 --- a/lib/config/migrate-validate.spec.ts +++ b/lib/config/migrate-validate.spec.ts @@ -45,32 +45,5 @@ describe('config/migrate-validate', () => { expect(res).toMatchSnapshot(); }); - it('does not remove approval rules for merge requests without automerge enabled', async () => { - const input: RenovateConfig = { - platformAutomerge: true, - gitLabIgnoreApprovals: true, - automergeType: 'pr', - packageRules: [ - { - matchPackageNames: ['/azure/'], - automerge: true, - }, - ], - }; - const res = await migrateAndValidate(config, input); - expect(res).toEqual({ - platformAutomerge: true, - gitLabIgnoreApprovals: true, - automergeType: 'pr', - packageRules: [ - { - matchPackageNames: ['/azure/'], - automerge: true, - }, - ], - errors: [], - warnings: [], - }); - }); }); }); From 9ebbef50e734858a3f1f7f7005213e0cb76e7b9d Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Tue, 1 Oct 2024 07:02:58 +0200 Subject: [PATCH 3/4] Update lib/config/migrate-validate.spec.ts --- lib/config/migrate-validate.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/config/migrate-validate.spec.ts b/lib/config/migrate-validate.spec.ts index 3c6ecaf44024d9..c0d87d4afa9165 100644 --- a/lib/config/migrate-validate.spec.ts +++ b/lib/config/migrate-validate.spec.ts @@ -44,6 +44,5 @@ describe('config/migrate-validate', () => { expect(res.warnings).toBeUndefined(); expect(res).toMatchSnapshot(); }); - }); }); From 856e659effeb83e8548e0dc3df0c9b88d60f242d Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Tue, 1 Oct 2024 07:04:07 +0200 Subject: [PATCH 4/4] Update lib/config/presets/gitlab/index.spec.ts --- lib/config/presets/gitlab/index.spec.ts | 38 ------------------------- 1 file changed, 38 deletions(-) diff --git a/lib/config/presets/gitlab/index.spec.ts b/lib/config/presets/gitlab/index.spec.ts index 7f4d77a3b7900b..bccecb87ab3a32 100644 --- a/lib/config/presets/gitlab/index.spec.ts +++ b/lib/config/presets/gitlab/index.spec.ts @@ -124,44 +124,6 @@ describe('config/presets/gitlab/index', () => { }); expect(content).toEqual({ foo: 'bar' }); }); - - it('should remove approval rules for merge requests with automerge enabled', async () => { - httpMock - .scope(gitlabApiHost) - .get(projectPath) - .reply(200, { - default_branch: 'main', - }) - .get(`${basePath}/files/default.json/raw?ref=main`) - .reply(200, { foo: 'bar' }, {}); - - const content = await gitlab.getPreset({ repo: 'some/repo' }); - expect(content).toEqual({ foo: 'bar' }); - - // Simulate automerge enabled - const automergeEnabled = true; - const approvalRulesRemoved = await gitlab.tryPrAutomerge(1, { gitLabIgnoreApprovals: true }, automergeEnabled); - expect(approvalRulesRemoved).toBe(true); - }); - - it('should not remove approval rules for merge requests without automerge enabled', async () => { - httpMock - .scope(gitlabApiHost) - .get(projectPath) - .reply(200, { - default_branch: 'main', - }) - .get(`${basePath}/files/default.json/raw?ref=main`) - .reply(200, { foo: 'bar' }, {}); - - const content = await gitlab.getPreset({ repo: 'some/repo' }); - expect(content).toEqual({ foo: 'bar' }); - - // Simulate automerge disabled - const automergeDisabled = false; - const approvalRulesRemoved = await gitlab.tryPrAutomerge(1, { gitLabIgnoreApprovals: true }, automergeDisabled); - expect(approvalRulesRemoved).toBe(false); - }); }); describe('getPresetFromEndpoint()', () => {