paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 41 Packages Projects Releases Wiki Activity

Merge pull request #28272 from hghtwr/26675

Browse Source 
Feature: Allow adding MR reviewers via publish:gitlab:merge-request action & automatic assignment of reviewers based on approval rules (#26675)
This commit is contained in:
Ben Lambert
2025-01-14 08:25:33 +01:00
committed by GitHub
parent 5af7f78385 5740c8b143
commit 530a31d02a
6 changed files with 474 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/lovely-stingrays-know.md
+5
View File
@@ -0,0 +1,5 @@
---
'@backstage/plugin-scaffolder-backend-module-gitlab': patch
---
Added `reviewerIds` to the Gitlab Merge Request action.
plugins/scaffolder-backend-module-gitlab/report.api.md
+1
View File
@@ -202,6 +202,7 @@ export const createPublishGitlabMergeRequestAction: (options: {
projectid?: string | undefined;
removeSourceBranch?: boolean | undefined;
assignee?: string | undefined;
reviewers?: string[] | undefined;
},
JsonObject
>;
plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.examples.test.ts
+52
View File
@@ -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(),
plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts
+328 -8
View File
@@ -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 = {
plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts
+87 -4
View File
@@ -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)}`,
plugins/scaffolder-backend/report.api.md
+1
View File
@@ -358,6 +358,7 @@ export const createPublishGitlabMergeRequestAction: (options: {
projectid?: string | undefined;
removeSourceBranch?: boolean | undefined;
assignee?: string | undefined;
reviewers?: string[] | undefined;
},
JsonObject
>;