From 27ad9a3446ade7160c9245ad5790a0d2952022ec Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Sat, 21 Dec 2024 21:13:09 +0100 Subject: [PATCH 01/13] update gitbeaker to support reviewerId's in it's interface (#3498), add reviewer id's with async retrieval Signed-off-by: Hghtwr --- packages/backend/src/index.ts | 3 ++ .../src/actions/gitlabMergeRequest.ts | 28 +++++++++++++++++++ yarn.lock | 21 ++++++++++---- 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index 8ae9864a4a..12dfa59302 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -61,4 +61,7 @@ backend.add(import('@backstage/plugin-signals-backend')); backend.add(import('@backstage/plugin-notifications-backend')); backend.add(import('./instanceMetadata')); +// backend.add(eventsModuleGitlabWebhook); +backend.add(import('@backstage/plugin-catalog-backend-module-gitlab-org')); + backend.start(); diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 4e4812fdcf..502b9988aa 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -100,6 +100,7 @@ export const createPublishGitlabMergeRequestAction = (options: { projectid?: string; removeSourceBranch?: boolean; assignee?: string; + reviewers?: string[]; }>({ id: 'publish:gitlab:merge-request', examples, @@ -179,6 +180,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 +216,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 +252,23 @@ which uses additional API calls in order to detect whether to 'create', 'update' } } + let reviewerIds: number[] | undefined; + if (reviewers !== undefined) { + reviewerIds = await Promise.all( + reviewers.map(async reviewer => { + try { + const reviewerUser = await api.Users.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; + } + }), + ); + } + let fileRoot: string; if (sourcePath) { fileRoot = resolveSafeChildPath(ctx.workspacePath, sourcePath); @@ -364,6 +391,7 @@ 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); ctx.output('projectid', repoID); diff --git a/yarn.lock b/yarn.lock index f40ec341af..01138c7cab 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1,6 +1,3 @@ -# This file is generated by running "yarn install" inside your project. -# Manual changes might be lost - proceed with caution! - __metadata: version: 6 cacheKey: 8 @@ -6018,7 +6015,7 @@ __metadata: languageName: unknown linkType: soft -"@backstage/plugin-catalog-backend-module-gitlab-org@workspace:plugins/catalog-backend-module-gitlab-org": +"@backstage/plugin-catalog-backend-module-gitlab-org@workspace:^, @backstage/plugin-catalog-backend-module-gitlab-org@workspace:plugins/catalog-backend-module-gitlab-org": version: 0.0.0-use.local resolution: "@backstage/plugin-catalog-backend-module-gitlab-org@workspace:plugins/catalog-backend-module-gitlab-org" dependencies: @@ -7736,7 +7733,9 @@ __metadata: "@backstage/integration": "workspace:^" "@backstage/plugin-scaffolder-node": "workspace:^" "@backstage/plugin-scaffolder-node-test-utils": "workspace:^" - "@gitbeaker/rest": ^41.2.0 + "@gitbeaker/core": ^39.28.0 + "@gitbeaker/node": ^35.8.0 + "@gitbeaker/rest": ^39.25.0 luxon: ^3.0.0 winston: ^3.2.1 yaml: ^2.0.0 @@ -10176,6 +10175,17 @@ __metadata: languageName: node linkType: hard +"@gitbeaker/core@npm:^39.28.0, @gitbeaker/core@npm:^39.34.3": + version: 39.34.3 + resolution: "@gitbeaker/core@npm:39.34.3" + dependencies: + "@gitbeaker/requester-utils": ^39.34.3 + qs: ^6.11.2 + xcase: ^2.0.1 + checksum: 2b0ccd6137ee717b6f51c51c8f7063b84beeebfdb044574f2257f282864411c4f0932f2619893385f2a86bb4fa342b75be972fd1868640ab4e260f2280cc52b3 + languageName: node + linkType: hard + "@gitbeaker/core@npm:^40.6.0": version: 40.6.0 resolution: "@gitbeaker/core@npm:40.6.0" @@ -28878,6 +28888,7 @@ __metadata: "@backstage/plugin-auth-node": "workspace:^" "@backstage/plugin-catalog-backend": "workspace:^" "@backstage/plugin-catalog-backend-module-backstage-openapi": "workspace:^" + "@backstage/plugin-catalog-backend-module-gitlab-org": "workspace:^" "@backstage/plugin-catalog-backend-module-openapi": "workspace:^" "@backstage/plugin-catalog-backend-module-scaffolder-entity-model": "workspace:^" "@backstage/plugin-catalog-backend-module-unprocessed": "workspace:^" From f9c8dc79449d9c5ef093bb2f04a104c9a5baceb4 Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Sat, 21 Dec 2024 23:00:52 +0100 Subject: [PATCH 02/13] revert test changes Signed-off-by: Hghtwr --- packages/backend/package.json | 1 + packages/backend/src/index.ts | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/backend/package.json b/packages/backend/package.json index 6409dfae7a..6483a77397 100644 --- a/packages/backend/package.json +++ b/packages/backend/package.json @@ -40,6 +40,7 @@ "@backstage/plugin-auth-node": "workspace:^", "@backstage/plugin-catalog-backend": "workspace:^", "@backstage/plugin-catalog-backend-module-backstage-openapi": "workspace:^", + "@backstage/plugin-catalog-backend-module-gitlab-org": "workspace:^", "@backstage/plugin-catalog-backend-module-openapi": "workspace:^", "@backstage/plugin-catalog-backend-module-scaffolder-entity-model": "workspace:^", "@backstage/plugin-catalog-backend-module-unprocessed": "workspace:^", diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index 12dfa59302..8ae9864a4a 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -61,7 +61,4 @@ backend.add(import('@backstage/plugin-signals-backend')); backend.add(import('@backstage/plugin-notifications-backend')); backend.add(import('./instanceMetadata')); -// backend.add(eventsModuleGitlabWebhook); -backend.add(import('@backstage/plugin-catalog-backend-module-gitlab-org')); - backend.start(); From 0bafea1a81e4155f9305e03fc30154af85b94046 Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Sat, 21 Dec 2024 23:02:25 +0100 Subject: [PATCH 03/13] revert test changes Signed-off-by: Hghtwr --- packages/backend/package.json | 1 - yarn.lock | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/backend/package.json b/packages/backend/package.json index 6483a77397..6409dfae7a 100644 --- a/packages/backend/package.json +++ b/packages/backend/package.json @@ -40,7 +40,6 @@ "@backstage/plugin-auth-node": "workspace:^", "@backstage/plugin-catalog-backend": "workspace:^", "@backstage/plugin-catalog-backend-module-backstage-openapi": "workspace:^", - "@backstage/plugin-catalog-backend-module-gitlab-org": "workspace:^", "@backstage/plugin-catalog-backend-module-openapi": "workspace:^", "@backstage/plugin-catalog-backend-module-scaffolder-entity-model": "workspace:^", "@backstage/plugin-catalog-backend-module-unprocessed": "workspace:^", diff --git a/yarn.lock b/yarn.lock index 01138c7cab..6087c9cf30 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6015,7 +6015,7 @@ __metadata: languageName: unknown linkType: soft -"@backstage/plugin-catalog-backend-module-gitlab-org@workspace:^, @backstage/plugin-catalog-backend-module-gitlab-org@workspace:plugins/catalog-backend-module-gitlab-org": +"@backstage/plugin-catalog-backend-module-gitlab-org@workspace:plugins/catalog-backend-module-gitlab-org": version: 0.0.0-use.local resolution: "@backstage/plugin-catalog-backend-module-gitlab-org@workspace:plugins/catalog-backend-module-gitlab-org" dependencies: @@ -28888,7 +28888,6 @@ __metadata: "@backstage/plugin-auth-node": "workspace:^" "@backstage/plugin-catalog-backend": "workspace:^" "@backstage/plugin-catalog-backend-module-backstage-openapi": "workspace:^" - "@backstage/plugin-catalog-backend-module-gitlab-org": "workspace:^" "@backstage/plugin-catalog-backend-module-openapi": "workspace:^" "@backstage/plugin-catalog-backend-module-scaffolder-entity-model": "workspace:^" "@backstage/plugin-catalog-backend-module-unprocessed": "workspace:^" From 8fb529a2a04973b3c1f5ca345102df2a6c8aae2e Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Sat, 21 Dec 2024 23:06:08 +0100 Subject: [PATCH 04/13] add changeset Signed-off-by: Hghtwr --- .changeset/lovely-stingrays-know.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/lovely-stingrays-know.md diff --git a/.changeset/lovely-stingrays-know.md b/.changeset/lovely-stingrays-know.md new file mode 100644 index 0000000000..f672a31e24 --- /dev/null +++ b/.changeset/lovely-stingrays-know.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-gitlab': minor +--- + +Added reviewerIds to the Gitlab Merge Request action. From 04f8fb830d19c7476fe7c033fb5106c4543098a9 Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Mon, 23 Dec 2024 12:01:18 +0100 Subject: [PATCH 05/13] return undefined if reviewer not found, add basic testing Signed-off-by: Hghtwr --- .../src/actions/gitlabMergeRequest.test.ts | 193 +++++++++++++++++- .../src/actions/gitlabMergeRequest.ts | 32 +-- 2 files changed, 204 insertions(+), 21 deletions(-) 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..e95856658e 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts @@ -67,13 +67,29 @@ 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 'Jane Doe': + return [ + { + id: 456, + }, + ]; + default: + throw new Error('user does not exist'); + } + // if (userOptions.username !== 'John Smith') + // throw new Error('user does not exist'); + // return [ + // { + // id: 123, + // }, + // ]; }), }, Repositories: { @@ -517,6 +533,169 @@ 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, + }, + ); + }); + + it('reviewer is set correcly when a valid reviewer username is 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'], + }; + 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], + }, + ); + }); + + 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 502b9988aa..e72f4831a9 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -252,21 +252,25 @@ which uses additional API calls in order to detect whether to 'create', 'update' } } - let reviewerIds: number[] | undefined; + 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.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; - } - }), - ); + 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; From ee55a6cdefaf818796544a56c04fb945155d2b84 Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Mon, 23 Dec 2024 15:57:28 +0100 Subject: [PATCH 06/13] added automatic assignment of reviewers when they are eligible approvers Signed-off-by: Hghtwr --- .../src/actions/gitlabMergeRequest.ts | 60 +++++++++++++++++-- 1 file changed, 56 insertions(+), 4 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index e72f4831a9..9493b899b5 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'; @@ -386,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), @@ -397,11 +402,58 @@ which uses additional API calls in order to detect whether to 'create', 'update' 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, + }, + ); + + // This works right away for users/groups set in the rules. + 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)}`, From d68530719f98d93c2625e6db9219398886e8b74c Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Mon, 23 Dec 2024 16:07:54 +0100 Subject: [PATCH 07/13] fixed changeset Signed-off-by: Hghtwr --- .changeset/lovely-stingrays-know.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.changeset/lovely-stingrays-know.md b/.changeset/lovely-stingrays-know.md index f672a31e24..056b4f391d 100644 --- a/.changeset/lovely-stingrays-know.md +++ b/.changeset/lovely-stingrays-know.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-scaffolder-backend-module-gitlab': minor +'@backstage/plugin-scaffolder-backend-module-gitlab': patch --- -Added reviewerIds to the Gitlab Merge Request action. +Added `reviewerIds` to the Gitlab Merge Request action. From 9bcd3f582e44752b38cbc8b54f510188e99dbdf0 Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Sun, 29 Dec 2024 10:12:01 +0100 Subject: [PATCH 08/13] add tests for automatic reviewer assignment Signed-off-by: Hghtwr --- .../gitlabMergeRequest.examples.test.ts | 52 ++++++ .../src/actions/gitlabMergeRequest.test.ts | 156 +++++++++++++++++- .../src/actions/gitlabMergeRequest.ts | 1 - 3 files changed, 204 insertions(+), 5 deletions(-) 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 e95856658e..a61a26ebb5 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(), @@ -74,6 +149,12 @@ const mockGitlabClient = { id: 123, }, ]; + case 'Bob Vance': + return [ + { + id: 234, + }, + ]; case 'Jane Doe': return [ { @@ -571,9 +652,19 @@ describe('createGitLabMergeRequest', () => { 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', async () => { + 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', @@ -582,7 +673,7 @@ describe('createGitLabMergeRequest', () => { removeSourceBranch: false, targetPath: 'Subdirectory', assignee: 'John Smith', - reviewers: ['Jane Doe'], + reviewers: ['Jane Doe', 'Bob Vance'], }; mockDir.setContent({ [workspacePath]: { @@ -609,9 +700,66 @@ describe('createGitLabMergeRequest', () => { description: 'This is an important change', removeSourceBranch: false, assigneeId: 123, - reviewerIds: [456], + 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 () => { diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 9493b899b5..607a904d0a 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -424,7 +424,6 @@ which uses additional API calls in order to detect whether to 'create', 'update' }, ); - // This works right away for users/groups set in the rules. if (approvalRules.length !== 0) { const eligibleApprovers = approvalRules .filter(rule => rule.eligible_approvers !== undefined) From 2cc4b5944acd5bb8d5f8cd0535963e5563550d6d Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Sun, 29 Dec 2024 10:37:07 +0100 Subject: [PATCH 09/13] updating api report Signed-off-by: Hghtwr --- .../report.api.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/report.api.md b/plugins/scaffolder-backend-module-gitlab/report.api.md index 896c0a5899..e67ee05146 100644 --- a/plugins/scaffolder-backend-module-gitlab/report.api.md +++ b/plugins/scaffolder-backend-module-gitlab/report.api.md @@ -33,11 +33,11 @@ export const createGitlabIssueAction: (options: { projectId: number; labels?: string | undefined; description?: string | undefined; - weight?: number | undefined; token?: string | undefined; + weight?: number | undefined; assignees?: number[] | undefined; - createdAt?: string | undefined; confidential?: boolean | undefined; + createdAt?: string | undefined; milestoneId?: number | undefined; epicId?: number | undefined; dueDate?: string | undefined; @@ -61,8 +61,8 @@ export const createGitlabProjectAccessTokenAction: (options: { projectId: string | number; name?: string | undefined; token?: string | undefined; - scopes?: string[] | undefined; expiresAt?: string | undefined; + scopes?: string[] | undefined; accessLevel?: number | undefined; }, { @@ -76,8 +76,8 @@ export const createGitlabProjectDeployTokenAction: (options: { }) => TemplateAction< { name: string; - scopes: string[]; repoUrl: string; + scopes: string[]; projectId: string | number; username?: string | undefined; token?: string | undefined; @@ -118,7 +118,7 @@ export const createGitlabRepoPushAction: (options: { sourcePath?: string | undefined; targetPath?: string | undefined; token?: string | undefined; - commitAction?: 'update' | 'delete' | 'create' | undefined; + commitAction?: 'update' | 'create' | 'delete' | undefined; }, JsonObject >; @@ -198,10 +198,11 @@ export const createPublishGitlabMergeRequestAction: (options: { sourcePath?: string | undefined; targetPath?: string | undefined; token?: string | undefined; - commitAction?: 'auto' | 'update' | 'delete' | 'create' | 'skip' | undefined; + commitAction?: 'auto' | 'update' | 'skip' | 'create' | 'delete' | undefined; projectid?: string | undefined; removeSourceBranch?: boolean | undefined; assignee?: string | undefined; + reviewers?: string[] | undefined; }, JsonObject >; @@ -211,8 +212,8 @@ export const createTriggerGitlabPipelineAction: (options: { integrations: ScmIntegrationRegistry; }) => TemplateAction< { - branch: string; repoUrl: string; + branch: string; projectId: number; tokenDescription: string; token?: string | undefined; @@ -234,8 +235,8 @@ export const editGitlabIssueAction: (options: { title?: string | undefined; labels?: string | undefined; description?: string | undefined; - weight?: number | undefined; token?: string | undefined; + weight?: number | undefined; assignees?: number[] | undefined; addLabels?: string | undefined; confidential?: boolean | undefined; From b11da743d23e4577e90defa71bb5d83780a2d9bc Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Mon, 30 Dec 2024 09:45:23 +0100 Subject: [PATCH 10/13] update api signature Signed-off-by: Hghtwr --- .../report.api.md | 16 ++++++++-------- plugins/scaffolder-backend/report.api.md | 1 + 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/report.api.md b/plugins/scaffolder-backend-module-gitlab/report.api.md index e67ee05146..b8ed736c03 100644 --- a/plugins/scaffolder-backend-module-gitlab/report.api.md +++ b/plugins/scaffolder-backend-module-gitlab/report.api.md @@ -33,11 +33,11 @@ export const createGitlabIssueAction: (options: { projectId: number; labels?: string | undefined; description?: string | undefined; - token?: string | undefined; weight?: number | undefined; + token?: string | undefined; assignees?: number[] | undefined; - confidential?: boolean | undefined; createdAt?: string | undefined; + confidential?: boolean | undefined; milestoneId?: number | undefined; epicId?: number | undefined; dueDate?: string | undefined; @@ -61,8 +61,8 @@ export const createGitlabProjectAccessTokenAction: (options: { projectId: string | number; name?: string | undefined; token?: string | undefined; - expiresAt?: string | undefined; scopes?: string[] | undefined; + expiresAt?: string | undefined; accessLevel?: number | undefined; }, { @@ -76,8 +76,8 @@ export const createGitlabProjectDeployTokenAction: (options: { }) => TemplateAction< { name: string; - repoUrl: string; scopes: string[]; + repoUrl: string; projectId: string | number; username?: string | undefined; token?: string | undefined; @@ -118,7 +118,7 @@ export const createGitlabRepoPushAction: (options: { sourcePath?: string | undefined; targetPath?: string | undefined; token?: string | undefined; - commitAction?: 'update' | 'create' | 'delete' | undefined; + commitAction?: 'update' | 'delete' | 'create' | undefined; }, JsonObject >; @@ -198,7 +198,7 @@ export const createPublishGitlabMergeRequestAction: (options: { sourcePath?: string | undefined; targetPath?: string | undefined; token?: string | undefined; - commitAction?: 'auto' | 'update' | 'skip' | 'create' | 'delete' | undefined; + commitAction?: 'auto' | 'update' | 'delete' | 'create' | 'skip' | undefined; projectid?: string | undefined; removeSourceBranch?: boolean | undefined; assignee?: string | undefined; @@ -212,8 +212,8 @@ export const createTriggerGitlabPipelineAction: (options: { integrations: ScmIntegrationRegistry; }) => TemplateAction< { - repoUrl: string; branch: string; + repoUrl: string; projectId: number; tokenDescription: string; token?: string | undefined; @@ -235,8 +235,8 @@ export const editGitlabIssueAction: (options: { title?: string | undefined; labels?: string | undefined; description?: string | undefined; - token?: string | undefined; weight?: number | undefined; + token?: string | undefined; assignees?: number[] | undefined; addLabels?: string | undefined; confidential?: boolean | undefined; 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 >; From c0a16aee2e87c10584bdf677a731b055f272481e Mon Sep 17 00:00:00 2001 From: Hghtwr Date: Thu, 2 Jan 2025 10:18:08 +0100 Subject: [PATCH 11/13] remove commented parts Signed-off-by: Hghtwr --- .../src/actions/gitlabMergeRequest.test.ts | 7 ------- 1 file changed, 7 deletions(-) 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 a61a26ebb5..874764b5ba 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts @@ -164,13 +164,6 @@ const mockGitlabClient = { default: throw new Error('user does not exist'); } - // if (userOptions.username !== 'John Smith') - // throw new Error('user does not exist'); - // return [ - // { - // id: 123, - // }, - // ]; }), }, Repositories: { From 64554d16adb317282b659f5241c04b92b6ad85c1 Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 14 Jan 2025 08:05:13 +0100 Subject: [PATCH 12/13] chore: reset Signed-off-by: blam Signed-off-by: blam --- package.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 41533ef516..89c00b9d9d 100644 --- a/package.json +++ b/package.json @@ -84,9 +84,6 @@ ] }, "prettier": "@backstage/cli/config/prettier", - "jest": { - "rejectFrontendNetworkRequests": true - }, "resolutions": { "@changesets/assemble-release-plan@^6.0.0": "patch:@changesets/assemble-release-plan@npm%3A6.0.0#./.yarn/patches/@changesets-assemble-release-plan-npm-6.0.0-f7b3005037.patch", "@material-ui/pickers@^3.2.10": "patch:@material-ui/pickers@npm%3A3.3.11#./.yarn/patches/@material-ui-pickers-npm-3.3.11-1c8f68ea20.patch", @@ -137,6 +134,9 @@ "sort-package-json": "^2.8.0", "typescript": "~5.2.0" }, + "jest": { + "rejectFrontendNetworkRequests": true + }, "packageManager": "yarn@3.8.1", "engines": { "node": "20 || 22" From 5740c8b1437303dbd2eee01954a60b47bc231e90 Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 14 Jan 2025 08:11:32 +0100 Subject: [PATCH 13/13] cghore: reset Signed-off-by: blam --- package.json | 6 +++--- yarn.lock | 18 ++++-------------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/package.json b/package.json index 89c00b9d9d..41533ef516 100644 --- a/package.json +++ b/package.json @@ -84,6 +84,9 @@ ] }, "prettier": "@backstage/cli/config/prettier", + "jest": { + "rejectFrontendNetworkRequests": true + }, "resolutions": { "@changesets/assemble-release-plan@^6.0.0": "patch:@changesets/assemble-release-plan@npm%3A6.0.0#./.yarn/patches/@changesets-assemble-release-plan-npm-6.0.0-f7b3005037.patch", "@material-ui/pickers@^3.2.10": "patch:@material-ui/pickers@npm%3A3.3.11#./.yarn/patches/@material-ui-pickers-npm-3.3.11-1c8f68ea20.patch", @@ -134,9 +137,6 @@ "sort-package-json": "^2.8.0", "typescript": "~5.2.0" }, - "jest": { - "rejectFrontendNetworkRequests": true - }, "packageManager": "yarn@3.8.1", "engines": { "node": "20 || 22" diff --git a/yarn.lock b/yarn.lock index 6087c9cf30..f40ec341af 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1,3 +1,6 @@ +# This file is generated by running "yarn install" inside your project. +# Manual changes might be lost - proceed with caution! + __metadata: version: 6 cacheKey: 8 @@ -7733,9 +7736,7 @@ __metadata: "@backstage/integration": "workspace:^" "@backstage/plugin-scaffolder-node": "workspace:^" "@backstage/plugin-scaffolder-node-test-utils": "workspace:^" - "@gitbeaker/core": ^39.28.0 - "@gitbeaker/node": ^35.8.0 - "@gitbeaker/rest": ^39.25.0 + "@gitbeaker/rest": ^41.2.0 luxon: ^3.0.0 winston: ^3.2.1 yaml: ^2.0.0 @@ -10175,17 +10176,6 @@ __metadata: languageName: node linkType: hard -"@gitbeaker/core@npm:^39.28.0, @gitbeaker/core@npm:^39.34.3": - version: 39.34.3 - resolution: "@gitbeaker/core@npm:39.34.3" - dependencies: - "@gitbeaker/requester-utils": ^39.34.3 - qs: ^6.11.2 - xcase: ^2.0.1 - checksum: 2b0ccd6137ee717b6f51c51c8f7063b84beeebfdb044574f2257f282864411c4f0932f2619893385f2a86bb4fa342b75be972fd1868640ab4e260f2280cc52b3 - languageName: node - linkType: hard - "@gitbeaker/core@npm:^40.6.0": version: 40.6.0 resolution: "@gitbeaker/core@npm:40.6.0"