From 66e392f99e970f0925553d39e248af60b50a9bbc Mon Sep 17 00:00:00 2001 From: Bogdan Nechyporenko Date: Sun, 26 Jan 2025 12:23:32 +0100 Subject: [PATCH 1/6] Making publish:gitlab:merge-request idempotent. Signed-off-by: Bogdan Nechyporenko --- .changeset/soft-readers-move.md | 5 + .../src/actions/gitlabMergeRequest.ts | 168 ++++++++++-------- 2 files changed, 99 insertions(+), 74 deletions(-) create mode 100644 .changeset/soft-readers-move.md diff --git a/.changeset/soft-readers-move.md b/.changeset/soft-readers-move.md new file mode 100644 index 0000000000..6440f0e49f --- /dev/null +++ b/.changeset/soft-readers-move.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-gitlab': patch +--- + +Making publish:gitlab:merge-request idempotent. diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 607a904d0a..c9d6dbbcbe 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -21,9 +21,9 @@ import { serializeDirectoryContents, } from '@backstage/plugin-scaffolder-node'; import { + CommitAction, Gitlab, RepositoryTreeSchema, - CommitAction, SimpleUserSchema, } from '@gitbeaker/rest'; import path from 'path'; @@ -242,7 +242,7 @@ which uses additional API calls in order to detect whether to 'create', 'update' repoUrl, }); - let assigneeId = undefined; + let assigneeId: number | undefined = undefined; if (assignee !== undefined) { try { @@ -379,85 +379,105 @@ which uses additional API calls in order to detect whether to 'create', 'update' ); } } - if (actions.length) { - try { - await api.Commits.create(repoID, branchName, title, actions); - } catch (e) { - throw new InputError( - `Committing the changes to ${branchName} failed. Please check that none of the files created by the template already exists. ${getErrorMessage( - e, - )}`, - ); - } - } - try { - let mergeRequest = await api.MergeRequests.create( - repoID, - branchName, - String(targetBranch), - title, - { - description, - removeSourceBranch: removeSourceBranch ? removeSourceBranch : false, - assigneeId, - reviewerIds, - }, - ); + await ctx.checkpoint({ + key: `commit.to.${repoID}.${branchName}`, + fn: async () => { + if (actions.length) { + try { + const commit = await api.Commits.create( + repoID, + branchName, + title, + actions, + ); + return commit.id; + } catch (e) { + throw new InputError( + `Committing the changes to ${branchName} failed. Please check that none of the files created by the template already exists. ${getErrorMessage( + e, + )}`, + ); + } + } + return null; + }, + }); + await ctx.checkpoint({ + key: `create.mr.${repoID}.${branchName}`, + fn: async () => { + try { + let mergeRequest = await api.MergeRequests.create( + repoID, + branchName, + String(targetBranch), + title, + { + description, + removeSourceBranch: removeSourceBranch + ? removeSourceBranch + : false, + assigneeId, + reviewerIds, + }, + ); - // 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. + // 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}`); - } + 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, - }, - ); + 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(); + 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 ?? []), - ]); + const eligibleUserIds = new Set([ + ...eligibleApprovers.map(user => user.id), + ...(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), + }, + ); + } - ctx.output('projectid', repoID); - ctx.output('targetBranchName', targetBranch); - ctx.output('projectPath', repoID); - ctx.output( - 'mergeRequestUrl', - mergeRequest.web_url ?? mergeRequest.webUrl, - ); - } catch (e) { - throw new InputError( - `Merge request creation failed. ${getErrorMessage(e)}`, - ); - } + ctx.output('projectid', repoID); + ctx.output('targetBranchName', targetBranch); + ctx.output('projectPath', repoID); + ctx.output( + 'mergeRequestUrl', + mergeRequest.web_url ?? mergeRequest.webUrl, + ); + } catch (e) { + throw new InputError( + `Merge request creation failed. ${getErrorMessage(e)}`, + ); + } + }, + }); }, }); }; From 97497d056a3dfd8f8f3ff97293ea8b856f18d8fb Mon Sep 17 00:00:00 2001 From: Bogdan Nechyporenko Date: Thu, 30 Jan 2025 22:22:53 +0100 Subject: [PATCH 2/6] wip Signed-off-by: Bogdan Nechyporenko --- .../src/actions/gitlabMergeRequest.test.ts | 7 +++++-- .../src/actions/mockActionContext.ts | 9 +++++++-- 2 files changed, 12 insertions(+), 4 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 874764b5ba..4e16445544 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts @@ -48,7 +48,7 @@ const mockGitlabClient = { }), }, Commits: { - create: jest.fn(), + create: jest.fn(() => ({ id: 'mockId' })), }, MergeRequests: { create: jest.fn(async (repoId: string) => { @@ -267,7 +267,10 @@ describe('createGitLabMergeRequest', () => { irrelevant: { 'bar.txt': 'Nothing to see here' }, }, }); - const ctx = createMockActionContext({ input, workspacePath }); + const ctx = createMockActionContext({ + input, + workspacePath, + }); await instance.handler(ctx); expect(mockGitlabClient.Projects.show).not.toHaveBeenCalled(); diff --git a/plugins/scaffolder-node-test-utils/src/actions/mockActionContext.ts b/plugins/scaffolder-node-test-utils/src/actions/mockActionContext.ts index 1d45051753..076bb0ce22 100644 --- a/plugins/scaffolder-node-test-utils/src/actions/mockActionContext.ts +++ b/plugins/scaffolder-node-test-utils/src/actions/mockActionContext.ts @@ -21,7 +21,7 @@ import { mockCredentials, mockServices, } from '@backstage/backend-test-utils'; -import { JsonObject } from '@backstage/types'; +import { JsonObject, JsonValue } from '@backstage/types'; import { ActionContext } from '@backstage/plugin-scaffolder-node'; /** @@ -43,7 +43,12 @@ export const createMockActionContext = < output: jest.fn(), createTemporaryDirectory: jest.fn(), input: {} as TActionInput, - checkpoint: jest.fn(), + async checkpoint(opts: { + key: string; + fn: () => Promise | T; + }): Promise { + return opts.fn(); + }, getInitiatorCredentials: () => Promise.resolve(credentials), task: { id: 'mock-task-id', From d086ad050c247eecc4f770bd3e4e5385c07c31f4 Mon Sep 17 00:00:00 2001 From: Bogdan Nechyporenko Date: Thu, 30 Jan 2025 23:41:53 +0100 Subject: [PATCH 3/6] test fix Signed-off-by: Bogdan Nechyporenko --- .../src/actions/gitlabMergeRequest.examples.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 e6aea81285..2f14412b84 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 @@ -34,7 +34,7 @@ const mockGitlabClient = { create: jest.fn(), }, Commits: { - create: jest.fn(), + create: jest.fn(() => ({ id: 'mockId' })), }, MergeRequests: { create: jest.fn(async (_: any) => { From 1198745093e5a7da680ed7b7d0b849e78f8f5104 Mon Sep 17 00:00:00 2001 From: Bogdan Nechyporenko Date: Thu, 6 Feb 2025 18:37:56 +0100 Subject: [PATCH 4/6] The master has been merged into the branch. Signed-off-by: Bogdan Nechyporenko --- .../src/actions/gitlabMergeRequest.ts | 156 ++++++++++-------- 1 file changed, 91 insertions(+), 65 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 26f7eff4ce..89ae1bddf2 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -21,12 +21,12 @@ import { serializeDirectoryContents, } from '@backstage/plugin-scaffolder-node'; import { + Camelize, + CommitAction, + ExpandedMergeRequestSchema, Gitlab, RepositoryTreeSchema, - CommitAction, SimpleUserSchema, - ExpandedMergeRequestSchema, - Camelize, } from '@gitbeaker/rest'; import path from 'path'; import { ScmIntegrationRegistry } from '@backstage/integration'; @@ -302,7 +302,7 @@ which uses additional API calls in order to detect whether to 'create', 'update' repoUrl, }); - let assigneeId = undefined; + let assigneeId: number | undefined = undefined; if (assignee !== undefined) { try { @@ -439,74 +439,100 @@ which uses additional API calls in order to detect whether to 'create', 'update' ); } } - if (actions.length) { - try { - await api.Commits.create(repoID, branchName, title, actions); - } catch (e) { - throw new InputError( - `Committing the changes to ${branchName} failed. Please check that none of the files created by the template already exists. ${getErrorMessage( - e, - )}`, - ); - } - } - try { - let mergeRequest = await api.MergeRequests.create( - repoID, - branchName, - String(targetBranch), - title, - { - description, - removeSourceBranch: removeSourceBranch ? removeSourceBranch : false, - assigneeId, - reviewerIds, - }, - ); - - if (ctx.input.assignReviewersFromApprovalRules) { - try { - const reviewersFromApprovalRules = - await getReviewersFromApprovalRules( - api, - mergeRequest.iid, + await ctx.checkpoint({ + key: `commit.to.${repoID}.${branchName}`, + fn: async () => { + if (actions.length) { + try { + const commit = await api.Commits.create( repoID, - ctx, + branchName, + title, + actions, ); - if (reviewersFromApprovalRules.length > 0) { - const eligibleUserIds = new Set([ - ...reviewersFromApprovalRules, - ...(reviewerIds ?? []), - ]); - - mergeRequest = await api.MergeRequests.edit( - repoID, - mergeRequest.iid, - { - reviewerIds: Array.from(eligibleUserIds), - }, + return commit.id; + } catch (e) { + throw new InputError( + `Committing the changes to ${branchName} failed. Please check that none of the files created by the template already exists. ${getErrorMessage( + e, + )}`, ); } + } + return null; + }, + }); + + const { mrId, mrWebUrl } = await ctx.checkpoint({ + key: `create.mr.${repoID}.${branchName}`, + fn: async () => { + try { + const mergeRequest = await api.MergeRequests.create( + repoID, + branchName, + String(targetBranch), + title, + { + description, + removeSourceBranch: removeSourceBranch + ? removeSourceBranch + : false, + assigneeId, + reviewerIds, + }, + ); + return { + mrId: mergeRequest.iid, + mrWebUrl: mergeRequest.web_url ?? mergeRequest.webUrl, + }; } catch (e) { - ctx.logger.warn( - `Failed to assign reviewers from approval rules: ${getErrorMessage( - e, - )}.`, + throw new InputError( + `Merge request creation failed. ${getErrorMessage(e)}`, ); } - } - ctx.output('projectid', repoID); - ctx.output('targetBranchName', targetBranch); - ctx.output('projectPath', repoID); - ctx.output( - 'mergeRequestUrl', - mergeRequest.web_url ?? mergeRequest.webUrl, - ); - } catch (e) { - throw new InputError( - `Merge request creation failed. ${getErrorMessage(e)}`, - ); - } + }, + }); + + await ctx.checkpoint({ + key: `create.mr.assign.reviewers.${repoID}.${branchName}`, + fn: async () => { + if (ctx.input.assignReviewersFromApprovalRules) { + try { + const reviewersFromApprovalRules = + await getReviewersFromApprovalRules(api, mrId, repoID, ctx); + if (reviewersFromApprovalRules.length > 0) { + const eligibleUserIds = new Set([ + ...reviewersFromApprovalRules, + ...(reviewerIds ?? []), + ]); + + const mergeRequest = await api.MergeRequests.edit( + repoID, + mrId, + { + reviewerIds: Array.from(eligibleUserIds), + }, + ); + return { + mrWebUrl: mergeRequest.web_url ?? mergeRequest.webUrl, + }; + } + } catch (e) { + ctx.logger.warn( + `Failed to assign reviewers from approval rules: ${getErrorMessage( + e, + )}.`, + ); + } + } + return { mrWebUrl }; + }, + }); + + ctx.output('projectid', repoID); + ctx.output('targetBranchName', targetBranch); + ctx.output('projectPath', repoID); + ctx.output('mergeRequestUrl', mrWebUrl); }, }); }; From eafe3cc99eb551c46c9b25a925a5beef422c2dfd Mon Sep 17 00:00:00 2001 From: Bogdan Nechyporenko Date: Thu, 6 Feb 2025 19:21:41 +0100 Subject: [PATCH 5/6] wip Signed-off-by: Bogdan Nechyporenko --- .changeset/soft-readers-move.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.changeset/soft-readers-move.md b/.changeset/soft-readers-move.md index 6440f0e49f..2e35a57e4b 100644 --- a/.changeset/soft-readers-move.md +++ b/.changeset/soft-readers-move.md @@ -1,5 +1,6 @@ --- '@backstage/plugin-scaffolder-backend-module-gitlab': patch +'@backstage/scaffolder-node-test-utils ': patch --- Making publish:gitlab:merge-request idempotent. From 2590c2f8951c2235c19bab9aacf7640a394a9241 Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 18 Feb 2025 09:05:32 +0100 Subject: [PATCH 6/6] chore: remove extra package Signed-off-by: blam --- .changeset/soft-readers-move.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.changeset/soft-readers-move.md b/.changeset/soft-readers-move.md index 2e35a57e4b..6440f0e49f 100644 --- a/.changeset/soft-readers-move.md +++ b/.changeset/soft-readers-move.md @@ -1,6 +1,5 @@ --- '@backstage/plugin-scaffolder-backend-module-gitlab': patch -'@backstage/scaffolder-node-test-utils ': patch --- Making publish:gitlab:merge-request idempotent.