diff --git a/.changeset/lovely-stingrays-know.md b/.changeset/lovely-stingrays-know.md new file mode 100644 index 0000000000..056b4f391d --- /dev/null +++ b/.changeset/lovely-stingrays-know.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-gitlab': patch +--- + +Added `reviewerIds` to the Gitlab Merge Request action. diff --git a/plugins/scaffolder-backend-module-gitlab/report.api.md b/plugins/scaffolder-backend-module-gitlab/report.api.md index 896c0a5899..b8ed736c03 100644 --- a/plugins/scaffolder-backend-module-gitlab/report.api.md +++ b/plugins/scaffolder-backend-module-gitlab/report.api.md @@ -202,6 +202,7 @@ export const createPublishGitlabMergeRequestAction: (options: { projectid?: string | undefined; removeSourceBranch?: boolean | undefined; assignee?: string | undefined; + reviewers?: string[] | undefined; }, JsonObject >; diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.examples.test.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.examples.test.ts index 087d474807..e6aea81285 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.examples.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.examples.test.ts @@ -42,6 +42,58 @@ const mockGitlabClient = { default_branch: 'main', }; }), + show: jest.fn(async (_: any) => { + return { + default_branch: 'main', + }; + }), + edit: jest.fn(async (_: any) => { + return { + default_branch: 'main', + }; + }), + }, + MergeRequestApprovals: { + allApprovalRules: jest.fn(async (_: any) => { + return [ + { + id: 123, + name: 'rule1', + rule_type: 'regular', + eligible_approvers: [ + { + id: 123, + username: 'John Smith', + }, + { + id: 456, + username: 'Jane Doe', + }, + ], + approvals_required: 1, + users: [], + contains_hidden_groups: false, + report_type: null, + section: null, + source_rule: { approvals_required: 1 }, + overridden: false, + }, + { + id: 456, + name: 'All Members', + rule_type: 'any_approver', + eligible_approvers: [], + approvals_required: 1, + users: [], + groups: [], + contains_hidden_groups: false, + report_type: null, + section: null, + source_rule: { approvals_required: 1 }, + overridden: false, + }, + ]; + }), }, Projects: { create: jest.fn(), 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 b5db1b4811..874764b5ba 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts @@ -51,11 +51,86 @@ const mockGitlabClient = { create: jest.fn(), }, MergeRequests: { - create: jest.fn(async (_: any) => { + create: jest.fn(async (repoId: string) => { + if (repoId === 'owner/repo-without-approvals') { + return { + iid: 5, + }; + } + return { + default_branch: 'main', + iid: 4, + }; + }), + show: jest.fn(async (repoId: string, iid: number) => { + if (repoId === 'owner/repo' && iid === 4) { + return { + iid: 4, + }; + } else if (repoId === 'owner/repo-without-approvals' && iid === 5) { + return { + iid: 5, + }; + } return { default_branch: 'main', }; }), + edit: jest.fn(async (_: any) => { + return { + default_branch: 'main', + }; + }), + }, + MergeRequestApprovals: { + allApprovalRules: jest.fn( + async (repoId: string, options: { mergerequestIId: number }) => { + if ( + repoId === 'owner/repo-without-approvals' && + options.mergerequestIId === 5 + ) { + return []; + } + return [ + { + id: 123, + name: 'rule1', + rule_type: 'regular', + eligible_approvers: [ + { + id: 234, + username: 'Bob Vance', + }, + { + id: 345, + username: 'Dina Fox', + }, + ], + approvals_required: 1, + users: [], + contains_hidden_groups: false, + report_type: null, + section: null, + source_rule: { approvals_required: 1 }, + overridden: false, + }, + { + id: 456, + name: 'All Members', + rule_type: 'any_approver', + eligible_approvers: [], + approvals_required: 1, + users: [], + groups: [], + contains_hidden_groups: false, + report_type: null, + section: null, + source_rule: { approvals_required: 1 }, + overridden: false, + }, + ]; + }, + ), }, Projects: { create: jest.fn(), @@ -67,13 +142,28 @@ const mockGitlabClient = { }, Users: { all: jest.fn(async (userOptions: { username: string }) => { - if (userOptions.username !== 'John Smith') - throw new Error('user does not exist'); - return [ - { - id: 123, - }, - ]; + switch (userOptions.username) { + case 'John Smith': + return [ + { + id: 123, + }, + ]; + case 'Bob Vance': + return [ + { + id: 234, + }, + ]; + case 'Jane Doe': + return [ + { + id: 456, + }, + ]; + default: + throw new Error('user does not exist'); + } }), }, Repositories: { @@ -517,6 +607,236 @@ describe('createGitLabMergeRequest', () => { }); }); + describe('createGitlabMergeRequestWithReviewers', () => { + it('no reviewers are set when a no reviewer are passed in options', 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', + }; + 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, + }, + ); + expect( + mockGitlabClient.MergeRequestApprovals.allApprovalRules, + ).toHaveBeenCalled(); + expect(mockGitlabClient.MergeRequests.edit).toHaveBeenCalledWith( + 'owner/repo', + 4, + { + reviewerIds: [234, 345], // Approval Rule Members + }, + ); + }); + + it('reviewer is set correcly when a valid reviewer username is passed in options in combination with MR 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'], + }; + 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, + ).toHaveBeenCalled(); + expect(mockGitlabClient.MergeRequests.edit).toHaveBeenCalledWith( + 'owner/repo', + 4, + { + reviewerIds: [234, 345, 456], // Approval Rule Members + Jane Doe (individual reviewer) but no duplicates (Bob Vance) + }, + ); + }); + + it('reviewer is set correcly when a valid reviewer username is passed in options and no MR rules exist', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo-without-approvals&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'], + }; + 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-without-approvals', + 'new-mr', + 'main', + ); + expect(mockGitlabClient.Commits.create).not.toHaveBeenCalled(); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo-without-approvals', + '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, + ).toHaveBeenCalledWith('owner/repo-without-approvals', { + mergerequestIId: 5, + }); + expect(mockGitlabClient.MergeRequests.edit).not.toHaveBeenCalled(); + }); + + it('reviewers are set correcly when valid reviewers username are passed in options', 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', 'John Smith'], + }; + 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, 123], + }, + ); + }); + + it('assignee is not set when a valid assignee username is not passed in options', 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', + reviewers: ['John Doe'], + }; + 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: undefined, + reviewerIds: [], + }, + ); + }); + }); + describe('createGitLabMergeRequestWithoutCommitAction', () => { it('default commitAction is auto', async () => { const input = { diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 4e4812fdcf..607a904d0a 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -20,7 +20,12 @@ import { SerializedFile, serializeDirectoryContents, } from '@backstage/plugin-scaffolder-node'; -import { Gitlab, RepositoryTreeSchema, CommitAction } from '@gitbeaker/rest'; +import { + Gitlab, + RepositoryTreeSchema, + CommitAction, + SimpleUserSchema, +} from '@gitbeaker/rest'; import path from 'path'; import { ScmIntegrationRegistry } from '@backstage/integration'; import { InputError } from '@backstage/errors'; @@ -100,6 +105,7 @@ export const createPublishGitlabMergeRequestAction = (options: { projectid?: string; removeSourceBranch?: boolean; assignee?: string; + reviewers?: string[]; }>({ id: 'publish:gitlab:merge-request', examples, @@ -179,6 +185,14 @@ which uses additional API calls in order to detect whether to 'create', 'update' type: 'string', description: 'User this merge request will be assigned to', }, + reviewers: { + title: 'Merge Request Reviewers', + type: 'array', + items: { + type: 'string', + }, + description: 'Users that will be assigned as reviewers', + }, }, }, output: { @@ -207,6 +221,7 @@ which uses additional API calls in order to detect whether to 'create', 'update' async handler(ctx) { const { assignee, + reviewers, branchName, targetBranchName, description, @@ -242,6 +257,27 @@ which uses additional API calls in order to detect whether to 'create', 'update' } } + let reviewerIds: number[] | undefined = undefined; // Explicitly set to undefined. Strangely, passing an empty array to the API will result the other options being undefined also being explicity passed to the Gitlab API call (e.g. assigneeId) + if (reviewers !== undefined) { + reviewerIds = ( + await Promise.all( + reviewers.map(async reviewer => { + try { + const reviewerUser = await api.Users.all({ + username: reviewer, + }); + return reviewerUser[0].id; + } catch (e) { + ctx.logger.warn( + `Failed to find gitlab user id for ${reviewer}: ${e}. Proceeding with MR creation without reviewer.`, + ); + return undefined; + } + }), + ) + ).filter(id => id !== undefined) as number[]; + } + let fileRoot: string; if (sourcePath) { fileRoot = resolveSafeChildPath(ctx.workspacePath, sourcePath); @@ -355,7 +391,7 @@ which uses additional API calls in order to detect whether to 'create', 'update' } } try { - const mergeRequestUrl = await api.MergeRequests.create( + let mergeRequest = await api.MergeRequests.create( repoID, branchName, String(targetBranch), @@ -364,12 +400,59 @@ which uses additional API calls in order to detect whether to 'create', 'update' description, removeSourceBranch: removeSourceBranch ? removeSourceBranch : false, assigneeId, + reviewerIds, }, - ).then(mergeRequest => mergeRequest.web_url ?? mergeRequest.webUrl); + ); + + // Because we don't know the code owners before the MR is created, we can't check the approval rules beforehand. + // Getting the approval rules beforehand is very difficult, especially, because of the inheritance rules for groups. + // Code owners take a moment to be processed and added to the approval rules after the MR is created. + + while ( + mergeRequest.detailed_merge_status === 'preparing' || + mergeRequest.detailed_merge_status === 'approvals_syncing' || + mergeRequest.detailed_merge_status === 'checking' + ) { + mergeRequest = await api.MergeRequests.show(repoID, mergeRequest.iid); + ctx.logger.info(`${mergeRequest.detailed_merge_status}`); + } + + const approvalRules = await api.MergeRequestApprovals.allApprovalRules( + repoID, + { + mergerequestIId: mergeRequest.iid, + }, + ); + + if (approvalRules.length !== 0) { + const eligibleApprovers = approvalRules + .filter(rule => rule.eligible_approvers !== undefined) + .map(rule => { + return rule.eligible_approvers as SimpleUserSchema[]; + }) + .flat(); + + const eligibleUserIds = new Set([ + ...eligibleApprovers.map(user => user.id), + ...(reviewerIds ?? []), + ]); + + mergeRequest = await api.MergeRequests.edit( + repoID, + mergeRequest.iid, + { + reviewerIds: Array.from(eligibleUserIds), + }, + ); + } + ctx.output('projectid', repoID); ctx.output('targetBranchName', targetBranch); ctx.output('projectPath', repoID); - ctx.output('mergeRequestUrl', mergeRequestUrl); + ctx.output( + 'mergeRequestUrl', + mergeRequest.web_url ?? mergeRequest.webUrl, + ); } catch (e) { throw new InputError( `Merge request creation failed. ${getErrorMessage(e)}`, diff --git a/plugins/scaffolder-backend/report.api.md b/plugins/scaffolder-backend/report.api.md index 20273697d3..d2bb6f1bb1 100644 --- a/plugins/scaffolder-backend/report.api.md +++ b/plugins/scaffolder-backend/report.api.md @@ -358,6 +358,7 @@ export const createPublishGitlabMergeRequestAction: (options: { projectid?: string | undefined; removeSourceBranch?: boolean | undefined; assignee?: string | undefined; + reviewers?: string[] | undefined; }, JsonObject >;