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

Fix automated assignment of reviewers for instances without premium/ultimate license (404). Introduce opt-in flag for automatic reviewer assignment based on approval rules

Browse Source 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
This commit is contained in:
Hghtwr
2025-02-02 12:02:50 +01:00
parent 50ba826817
commit 9d04e91133
3 changed files with 110 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/few-ducks-cross.md
+5
View File
@@ -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
plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts
+90 -8
View File
@@ -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 () => {
plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts
+15 -12
View File
@@ -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,
)}`,
)}.`,
);
}
}