From 9d04e91133cc108e10a4b645d3e3948b280abec5 Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Sun, 2 Feb 2025 12:02:50 +0100 Subject: [PATCH] Fix automated assignment of reviewers for instances without premium/ultimate license (404). Introduce opt-in flag for automatic reviewer assignment based on approval rules Signed-off-by: Hghtwr --- .changeset/few-ducks-cross.md | 5 + .../src/actions/gitlabMergeRequest.test.ts | 98 +++++++++++++++++-- .../src/actions/gitlabMergeRequest.ts | 27 ++--- 3 files changed, 110 insertions(+), 20 deletions(-) create mode 100644 .changeset/few-ducks-cross.md diff --git a/.changeset/few-ducks-cross.md b/.changeset/few-ducks-cross.md new file mode 100644 index 0000000000..8f1bc15e15 --- /dev/null +++ b/.changeset/few-ducks-cross.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-gitlab': patch +--- + +Fix automated assignment of reviewers for instances without premium/ultimate license (404). Introduce opt-in flag for automatic reviewer assignment based on approval rules diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts index 874764b5ba..08194b52cc 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts @@ -52,6 +52,11 @@ const mockGitlabClient = { }, MergeRequests: { create: jest.fn(async (repoId: string) => { + if (repoId === 'owner/repo-without-approval-rule-license') { + return { + iid: 6, + }; + } if (repoId === 'owner/repo-without-approvals') { return { iid: 5, @@ -63,6 +68,11 @@ const mockGitlabClient = { }; }), show: jest.fn(async (repoId: string, iid: number) => { + if (repoId === 'owner/repo-without-approval-rule-license' && iid === 6) { + return { + iid: 6, + }; + } if (repoId === 'owner/repo' && iid === 4) { return { iid: 4, @@ -91,6 +101,16 @@ const mockGitlabClient = { ) { return []; } + if ( + repoId === 'owner/repo-without-approval-rule-license' && + options.mergerequestIId === 6 + ) { + throw new Error('Not Found', { + cause: { + description: '404 Not found', + }, + }); + } return [ { id: 123, @@ -608,7 +628,7 @@ describe('createGitLabMergeRequest', () => { }); describe('createGitlabMergeRequestWithReviewers', () => { - it('no reviewers are set when a no reviewer are passed in options', async () => { + it('no dedicated reviewers are set when a no reviewer are passed in options but mr approval reviewers are set when flag activated', async () => { const input = { repoUrl: 'gitlab.com?repo=repo&owner=owner', title: 'Create my new MR', @@ -617,6 +637,7 @@ describe('createGitLabMergeRequest', () => { removeSourceBranch: false, targetPath: 'Subdirectory', assignee: 'John Smith', + assignReviewersFromApprovalRules: true, }; mockDir.setContent({ [workspacePath]: { @@ -667,6 +688,7 @@ describe('createGitLabMergeRequest', () => { targetPath: 'Subdirectory', assignee: 'John Smith', reviewers: ['Jane Doe', 'Bob Vance'], + assignReviewersFromApprovalRules: true, }; mockDir.setContent({ [workspacePath]: { @@ -708,7 +730,53 @@ describe('createGitLabMergeRequest', () => { ); }); - it('reviewer is set correcly when a valid reviewer username is passed in options and no MR rules exist', async () => { + it('reviewer is set correcly when a valid reviewer username is passed in options in combination with deactivated approval rules', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo&owner=owner', + title: 'Create my new MR', + branchName: 'new-mr', + description: 'This is an important change', + removeSourceBranch: false, + targetPath: 'Subdirectory', + assignee: 'John Smith', + reviewers: ['Jane Doe', 'Bob Vance'], + assignReviewersFromApprovalRules: false, + }; + mockDir.setContent({ + [workspacePath]: { + source: { 'foo.txt': 'Hello there!' }, + irrelevant: { 'bar.txt': 'Nothing to see here' }, + }, + }); + + const ctx = createMockActionContext({ input, workspacePath }); + await instance.handler(ctx); + + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); + expect(mockGitlabClient.Commits.create).not.toHaveBeenCalled(); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'This is an important change', + removeSourceBranch: false, + assigneeId: 123, + reviewerIds: [456, 234], // Jane Doe and Bob Vance + }, + ); + expect( + mockGitlabClient.MergeRequestApprovals.allApprovalRules, + ).not.toHaveBeenCalled(); + expect(mockGitlabClient.MergeRequests.edit).not.toHaveBeenCalled(); + }); + + it('reviewer is set correctly when a valid reviewer username is passed in options and no MR rules exist and approval rules are activated', async () => { const input = { repoUrl: 'gitlab.com?repo=repo-without-approvals&owner=owner', title: 'Create my new MR', @@ -718,6 +786,7 @@ describe('createGitLabMergeRequest', () => { targetPath: 'Subdirectory', assignee: 'John Smith', reviewers: ['Jane Doe', 'Bob Vance'], + assignReviewersFromApprovalRules: true, }; mockDir.setContent({ [workspacePath]: { @@ -755,16 +824,18 @@ describe('createGitLabMergeRequest', () => { expect(mockGitlabClient.MergeRequests.edit).not.toHaveBeenCalled(); }); - it('reviewers are set correcly when valid reviewers username are passed in options', async () => { + it('reviewer is set correcly when a valid reviewer username is passed in options and MR rules are not included in the Gitlab license (404)', async () => { const input = { - repoUrl: 'gitlab.com?repo=repo&owner=owner', + repoUrl: + 'gitlab.com?repo=repo-without-approval-rule-license&owner=owner', title: 'Create my new MR', branchName: 'new-mr', description: 'This is an important change', removeSourceBranch: false, targetPath: 'Subdirectory', assignee: 'John Smith', - reviewers: ['Jane Doe', 'John Smith'], + reviewers: ['Jane Doe', 'Bob Vance'], + assignReviewersFromApprovalRules: true, }; mockDir.setContent({ [workspacePath]: { @@ -774,16 +845,17 @@ describe('createGitLabMergeRequest', () => { }); const ctx = createMockActionContext({ input, workspacePath }); + ctx.logger.warn = jest.fn(); await instance.handler(ctx); expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( - 'owner/repo', + 'owner/repo-without-approval-rule-license', 'new-mr', 'main', ); expect(mockGitlabClient.Commits.create).not.toHaveBeenCalled(); expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( - 'owner/repo', + 'owner/repo-without-approval-rule-license', 'new-mr', 'main', 'Create my new MR', @@ -791,9 +863,19 @@ describe('createGitLabMergeRequest', () => { description: 'This is an important change', removeSourceBranch: false, assigneeId: 123, - reviewerIds: [456, 123], + reviewerIds: [456, 234], // Jane Doe and Bob Vance }, ); + expect( + mockGitlabClient.MergeRequestApprovals.allApprovalRules, + ).toHaveBeenCalledWith('owner/repo-without-approval-rule-license', { + mergerequestIId: 6, + }); + expect(mockGitlabClient.MergeRequests.edit).not.toHaveBeenCalled(); + expect(ctx.logger.warn).toHaveBeenCalledWith( + 'Failed to retrieve approval rules for MR 6: Error: Not Found. Proceeding with MR creation without reviewers from approval rules.', + ); + expect(ctx.output).toHaveBeenCalledWith('targetBranchName', 'main'); // This ensures that the MR scaffolder step finishes successfully and all errors are catched. }); it('assignee is not set when a valid assignee username is not passed in options', async () => { diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index c09659194d..26f7eff4ce 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -116,6 +116,7 @@ async function getReviewersFromApprovalRules( mergerequestIId: mergeRequest.iid, }, ); + return approvalRules .filter(rule => rule.eligible_approvers !== undefined) .map(rule => { @@ -472,23 +473,25 @@ which uses additional API calls in order to detect whether to 'create', 'update' repoID, ctx, ); - const eligibleUserIds = new Set([ - ...reviewersFromApprovalRules, - ...(reviewerIds ?? []), - ]); + if (reviewersFromApprovalRules.length > 0) { + const eligibleUserIds = new Set([ + ...reviewersFromApprovalRules, + ...(reviewerIds ?? []), + ]); - mergeRequest = await api.MergeRequests.edit( - repoID, - mergeRequest.iid, - { - reviewerIds: Array.from(eligibleUserIds), - }, - ); + mergeRequest = await api.MergeRequests.edit( + repoID, + mergeRequest.iid, + { + reviewerIds: Array.from(eligibleUserIds), + }, + ); + } } catch (e) { ctx.logger.warn( `Failed to assign reviewers from approval rules: ${getErrorMessage( e, - )}`, + )}.`, ); } }