From 9adfe4675958cdc91d9337204213e2537c27363c Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Tue, 24 Sep 2024 22:35:36 -0500 Subject: [PATCH 1/7] GitLab MR: introduce skip commit action. Signed-off-by: Matt Benson --- .changeset/few-hairs-compare.md | 5 + .../package.json | 1 + .../report.api.md | 2 +- .../src/actions/gitlabMergeRequest.test.ts | 295 +++++++++++++++++- .../src/actions/gitlabMergeRequest.ts | 142 ++++++--- plugins/scaffolder-backend/report.api.md | 2 +- yarn.lock | 1 + 7 files changed, 408 insertions(+), 40 deletions(-) create mode 100644 .changeset/few-hairs-compare.md diff --git a/.changeset/few-hairs-compare.md b/.changeset/few-hairs-compare.md new file mode 100644 index 0000000000..d717e07351 --- /dev/null +++ b/.changeset/few-hairs-compare.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-gitlab': minor +--- + +GitLab MR: introduce 'skip' commit action. diff --git a/plugins/scaffolder-backend-module-gitlab/package.json b/plugins/scaffolder-backend-module-gitlab/package.json index eba3426349..69d92ac5d9 100644 --- a/plugins/scaffolder-backend-module-gitlab/package.json +++ b/plugins/scaffolder-backend-module-gitlab/package.json @@ -55,6 +55,7 @@ "@gitbeaker/node": "^35.8.0", "@gitbeaker/rest": "^39.25.0", "luxon": "^3.0.0", + "winston": "^3.2.1", "yaml": "^2.0.0", "zod": "^3.22.4" }, diff --git a/plugins/scaffolder-backend-module-gitlab/report.api.md b/plugins/scaffolder-backend-module-gitlab/report.api.md index f00055a876..4ed062d21a 100644 --- a/plugins/scaffolder-backend-module-gitlab/report.api.md +++ b/plugins/scaffolder-backend-module-gitlab/report.api.md @@ -193,7 +193,7 @@ export const createPublishGitlabMergeRequestAction: (options: { sourcePath?: string | undefined; targetPath?: string | undefined; token?: string | undefined; - commitAction?: 'auto' | 'update' | 'delete' | 'create' | undefined; + commitAction?: 'auto' | 'update' | 'delete' | 'create' | 'skip' | undefined; projectid?: string | undefined; removeSourceBranch?: boolean | undefined; assignee?: string | undefined; 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 46e60584c9..cffa5fd666 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.test.ts @@ -30,6 +30,22 @@ const mockGitlabClient = { }, Branches: { create: jest.fn(), + show: jest.fn(async (_repoID: string | number, name: string) => { + if (['main', 'existing-branch'].includes(name)) { + return { + name, + merged: name === 'main', + protected: name === 'main', + default: name === 'main', + developers_can_push: name !== 'main', + developers_can_merge: name !== 'main', + can_push: name !== 'main', + web_url: `https://foo.bar.baz/owner/repo/-/tree/${name}`, + commit: { message: 'last change' }, + }; + } + throw new Error(`Unknown branch ${name}`); + }), }, Commits: { create: jest.fn(), @@ -83,6 +99,28 @@ const mockGitlabClient = { }, ), }, + RepositoryFiles: { + show: jest.fn( + async (repoID: string | number, filePath: string, ref: string) => { + if (repoID !== 'owner/repo') throw new Error('repo does not exist'); + if (filePath !== 'source/auto.txt') + throw new Error('filePath does not exist'); + return { + file_name: 'auto.txt', + file_path: 'source/auto.txt', + size: 11, + encoding: 'base64', + content: 'Zm9vLWJhci1iYXo=', + content_sha256: + '269dce1a5bb90188b2d9cf542a7c30e410c7d8251e34a97bfea56062df51ae23', + ref, + blob_id: 'a1e8f8d745cc87e3a9248358d9352bb7f9a0aeba', + commit_id: 'd5a3ff139356ce33e37e73add446f16869741b50', + last_commit_id: '570e7b2abdd848b95f2f578043fc23bd6f6fd24d', + }; + }, + ), + }, }; jest.mock('@gitbeaker/node', () => ({ @@ -149,6 +187,7 @@ describe('createGitLabMergeRequest', () => { 'new-mr', 'test', ); + expect(mockGitlabClient.Commits.create).not.toHaveBeenCalled(); expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -184,6 +223,7 @@ describe('createGitLabMergeRequest', () => { 'new-mr', 'main', ); + expect(mockGitlabClient.Commits.create).not.toHaveBeenCalled(); expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -191,7 +231,6 @@ describe('createGitLabMergeRequest', () => { 'Create my new MR', { description: 'This MR is really good', removeSourceBranch: false }, ); - expect(ctx.output).toHaveBeenCalledWith('targetBranchName', 'main'); }); }); @@ -216,6 +255,12 @@ describe('createGitLabMergeRequest', () => { 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', @@ -244,6 +289,12 @@ describe('createGitLabMergeRequest', () => { 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', @@ -278,6 +329,12 @@ describe('createGitLabMergeRequest', () => { 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', @@ -311,6 +368,12 @@ describe('createGitLabMergeRequest', () => { 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', @@ -344,6 +407,12 @@ describe('createGitLabMergeRequest', () => { 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', @@ -376,6 +445,12 @@ describe('createGitLabMergeRequest', () => { 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', @@ -404,6 +479,11 @@ describe('createGitLabMergeRequest', () => { const ctx = createMockActionContext({ input, workspacePath }); await instance.handler(ctx); + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -425,6 +505,16 @@ describe('createGitLabMergeRequest', () => { }, ]), ); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'This MR is really good', + removeSourceBranch: false, + }, + ); }); }); @@ -446,6 +536,11 @@ describe('createGitLabMergeRequest', () => { const ctx = createMockActionContext({ input, workspacePath }); await instance.handler(ctx); + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -467,6 +562,16 @@ describe('createGitLabMergeRequest', () => { }, ]), ); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'This MR is really good', + removeSourceBranch: false, + }, + ); }); }); @@ -490,6 +595,11 @@ describe('createGitLabMergeRequest', () => { const ctx = createMockActionContext({ input, workspacePath }); await instance.handler(ctx); + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -504,6 +614,16 @@ describe('createGitLabMergeRequest', () => { }, ], ); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'MR description', + removeSourceBranch: false, + }, + ); }); it('commitAction is update when update is passed in options', async () => { @@ -525,6 +645,11 @@ describe('createGitLabMergeRequest', () => { const ctx = createMockActionContext({ input, workspacePath }); await instance.handler(ctx); + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -539,6 +664,16 @@ describe('createGitLabMergeRequest', () => { }, ], ); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'MR description', + removeSourceBranch: false, + }, + ); }); it('commitAction is auto when auto is passed in options', async () => { @@ -558,6 +693,11 @@ describe('createGitLabMergeRequest', () => { const ctx = createMockActionContext({ input, workspacePath }); await instance.handler(ctx); + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -579,6 +719,16 @@ describe('createGitLabMergeRequest', () => { }, ]), ); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'MR description', + removeSourceBranch: false, + }, + ); }); it('commitAction is auto when auto is passed in options with targetPath', async () => { @@ -600,6 +750,11 @@ describe('createGitLabMergeRequest', () => { const ctx = createMockActionContext({ input, workspacePath }); await instance.handler(ctx); + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -621,6 +776,16 @@ describe('createGitLabMergeRequest', () => { }, ]), ); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'MR description', + removeSourceBranch: false, + }, + ); }); it('commitAction is delete when delete is passed in options', async () => { @@ -642,6 +807,11 @@ describe('createGitLabMergeRequest', () => { const ctx = createMockActionContext({ input, workspacePath }); await instance.handler(ctx); + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -656,6 +826,119 @@ describe('createGitLabMergeRequest', () => { }, ], ); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'other MR description', + removeSourceBranch: false, + }, + ); + }); + it('commitAction skip skips commit', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo&owner=owner', + title: 'Create my new MR', + branchName: 'new-mr', + description: 'MR description', + commitAction: 'skip', + }; + 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: 'MR description', + removeSourceBranch: false, + }, + ); + }); + it('commitAction skip reuses existing branch', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo&owner=owner', + title: 'Create my new MR', + branchName: 'existing-branch', + description: 'MR description', + commitAction: 'skip', + }; + const ctx = createMockActionContext({ input, workspacePath }); + await instance.handler(ctx); + + expect(mockGitlabClient.Branches.show).toHaveBeenCalledWith( + 'owner/repo', + 'existing-branch', + ); + expect(mockGitlabClient.Branches.create).not.toHaveBeenCalled(); + expect(mockGitlabClient.Commits.create).not.toHaveBeenCalled(); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'existing-branch', + 'main', + 'Create my new MR', + { + description: 'MR description', + removeSourceBranch: false, + }, + ); + }); + it('commitAction auto skips unmodified files', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo&owner=owner', + title: 'Create my new MR', + branchName: 'new-mr', + description: 'MR description', + commitAction: 'auto', + }; + mockDir.setContent({ + [workspacePath]: { + source: { 'foo.txt': 'Hello there!', 'auto.txt': 'foo-bar-baz' }, + }, + }); + + const ctx = createMockActionContext({ input, workspacePath }); + await instance.handler(ctx); + + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); + expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'Create my new MR', + expect.arrayContaining([ + { + action: 'create', + filePath: 'source/foo.txt', + content: 'SGVsbG8gdGhlcmUh', + encoding: 'base64', + execute_filemode: false, + }, + ]), + ); + expect(mockGitlabClient.MergeRequests.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + 'Create my new MR', + { + description: 'MR description', + removeSourceBranch: false, + }, + ); }); }); @@ -681,6 +964,11 @@ describe('createGitLabMergeRequest', () => { await instance.handler(ctx); + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', @@ -719,6 +1007,11 @@ describe('createGitLabMergeRequest', () => { await instance.handler(ctx); + expect(mockGitlabClient.Branches.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'main', + ); expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( 'owner/repo', 'new-mr', diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 733365c6df..a719176bd7 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -20,25 +20,57 @@ import { SerializedFile, serializeDirectoryContents, } from '@backstage/plugin-scaffolder-node'; -import { Types } from '@gitbeaker/core'; +import { Gitlab, Types } from '@gitbeaker/core'; import path from 'path'; import { ScmIntegrationRegistry } from '@backstage/integration'; import { InputError } from '@backstage/errors'; import { resolveSafeChildPath } from '@backstage/backend-plugin-api'; import { createGitlabApi } from './helpers'; import { examples } from './gitlabMergeRequest.examples'; +import { createHash } from 'crypto'; +import { Logger } from 'winston'; -function getFileAction( - fileInfo: { file: SerializedFile; targetPath: string | undefined }, +function computeSha256(file: SerializedFile): string { + const hash = createHash('sha256'); + hash.update(file.content); + return hash.digest('hex'); +} + +async function getFileAction( + fileInfo: { file: SerializedFile; targetPath?: string }, + target: { repoID: string; branch: string }, + api: Gitlab, + ctx: { logger: Logger }, remoteFiles: Types.RepositoryTreeSchema[], - defaultCommitAction: 'create' | 'delete' | 'update' | 'auto' | undefined, -): 'create' | 'delete' | 'update' { - if (!defaultCommitAction || defaultCommitAction === 'auto') { + defaultCommitAction: + | 'create' + | 'delete' + | 'update' + | 'skip' + | 'auto' = 'auto', +): Promise<'create' | 'delete' | 'update' | 'skip'> { + if (defaultCommitAction === 'auto') { const filePath = path.join(fileInfo.targetPath ?? '', fileInfo.file.path); - return remoteFiles && - remoteFiles.some(remoteFile => remoteFile.path === filePath) - ? 'update' - : 'create'; + if (remoteFiles) { + if (remoteFiles.some(remoteFile => remoteFile.path === filePath)) { + try { + const targetFile = await api.RepositoryFiles.show( + target.repoID, + filePath, + target.branch, + ); + if (computeSha256(fileInfo.file) === targetFile.content_sha256) { + return 'skip'; + } + } catch (error) { + ctx.logger.warn( + `Unable to retrieve detailed information for remote file ${filePath}`, + ); + } + return 'update'; + } + } + return 'create'; } return defaultCommitAction; } @@ -62,7 +94,7 @@ export const createPublishGitlabMergeRequestAction = (options: { sourcePath?: string; targetPath?: string; token?: string; - commitAction?: 'create' | 'delete' | 'update' | 'auto'; + commitAction?: 'create' | 'delete' | 'update' | 'skip' | 'auto'; /** @deprecated projectID passed as query parameters in the repoUrl */ projectid?: string; removeSourceBranch?: boolean; @@ -78,7 +110,9 @@ export const createPublishGitlabMergeRequestAction = (options: { repoUrl: { type: 'string', title: 'Repository Location', - description: `Accepts the format 'gitlab.com?repo=project_name&owner=group_name' where 'project_name' is the repository name and 'group_name' is a group or username`, + description: `\ +Accepts the format 'gitlab.com?repo=project_name&owner=group_name' where \ +'project_name' is the repository name and 'group_name' is a group or username`, }, /** @deprecated projectID is passed as query parameters in the repoUrl */ projectid: { @@ -109,8 +143,11 @@ export const createPublishGitlabMergeRequestAction = (options: { sourcePath: { type: 'string', title: 'Working Subdirectory', - description: - 'Subdirectory of working directory to copy changes from', + description: `\ +Subdirectory of working directory to copy changes from. \ +For reasons of backward compatibility, any specified 'targetPath' input will \ +be applied in place of an absent/falsy value for this input. \ +Circumvent this behavior using '.'`, }, targetPath: { type: 'string', @@ -126,8 +163,9 @@ export const createPublishGitlabMergeRequestAction = (options: { title: 'Commit action', type: 'string', enum: ['create', 'update', 'delete', 'auto'], - description: - 'The action to be used for git commit. Defaults to auto. "auto" is custom action provide by backstage, (automatic assign create or update action) /!\\ Use more api calls /!\\ *', + description: `\ +The action to be used for git commit. Defaults to the custom 'auto' action provided by backstage, +which uses additional API calls in order to detect whether to 'create', 'update' or 'skip' each source file.`, }, removeSourceBranch: { title: 'Delete source branch', @@ -224,7 +262,7 @@ export const createPublishGitlabMergeRequestAction = (options: { } let remoteFiles: Types.RepositoryTreeSchema[] = []; - if (!ctx.input.commitAction || ctx.input.commitAction === 'auto') { + if ((ctx.input.commitAction ?? 'auto') === 'auto') { try { remoteFiles = await api.Repositories.tree(repoID, { ref: targetBranch, @@ -238,12 +276,26 @@ export const createPublishGitlabMergeRequestAction = (options: { } } - const actions: Types.CommitAction[] = fileContents.map(file => ({ - action: getFileAction( - { file, targetPath }, - remoteFiles, - ctx.input.commitAction, - ), + const actions: Types.CommitAction[] = ( + ( + await Promise.all( + fileContents.map(async file => + getFileAction( + { file, targetPath }, + { repoID, branch: targetBranch! }, + api, + ctx, + remoteFiles, + ctx.input.commitAction, + ).then(action => ({ file, action })), + ), + ) + ).filter(o => o.action !== 'skip') as { + file: SerializedFile; + action: Types.CommitAction['action']; + }[] + ).map(({ file, action }) => ({ + action, filePath: targetPath ? path.posix.join(targetPath, file.path) : file.path, @@ -252,22 +304,38 @@ export const createPublishGitlabMergeRequestAction = (options: { execute_filemode: file.executable, })); - try { - await api.Branches.create(repoID, branchName, String(targetBranch)); - } catch (e) { - throw new InputError( - `The branch creation failed. Please check that your repo does not already contain a branch named '${branchName}'. ${e}`, - ); + let createBranch: boolean; + if (actions.length) { + createBranch = true; + } else { + try { + await api.Branches.show(repoID, branchName); + createBranch = false; + ctx.logger.info( + `Using existing branch ${branchName} without modification.`, + ); + } catch (e) { + createBranch = true; + } } - - try { - await api.Commits.create(repoID, branchName, ctx.input.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. ${e}`, - ); + if (createBranch) { + try { + await api.Branches.create(repoID, branchName, String(targetBranch)); + } catch (e) { + throw new InputError( + `The branch creation failed. Please check that your repo does not already contain a branch named '${branchName}'. ${e}`, + ); + } + } + 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. ${e}`, + ); + } } - try { const mergeRequestUrl = await api.MergeRequests.create( repoID, diff --git a/plugins/scaffolder-backend/report.api.md b/plugins/scaffolder-backend/report.api.md index d3dddc21e9..5c513d75cd 100644 --- a/plugins/scaffolder-backend/report.api.md +++ b/plugins/scaffolder-backend/report.api.md @@ -334,7 +334,7 @@ export const createPublishGitlabMergeRequestAction: (options: { sourcePath?: string | undefined; targetPath?: string | undefined; token?: string | undefined; - commitAction?: 'auto' | 'update' | 'delete' | 'create' | undefined; + commitAction?: 'auto' | 'update' | 'delete' | 'create' | 'skip' | undefined; projectid?: string | undefined; removeSourceBranch?: boolean | undefined; assignee?: string | undefined; diff --git a/yarn.lock b/yarn.lock index 7361d0ef51..957d4006e3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7456,6 +7456,7 @@ __metadata: "@gitbeaker/node": ^35.8.0 "@gitbeaker/rest": ^39.25.0 luxon: ^3.0.0 + winston: ^3.2.1 yaml: ^2.0.0 zod: ^3.22.4 languageName: unknown From 7cdbb244a1959630fee003e7c42e668cd460da5a Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Wed, 25 Sep 2024 09:31:27 -0500 Subject: [PATCH 2/7] improve efficiency when commit is explicitly skipped Signed-off-by: Matt Benson --- .../src/actions/gitlabMergeRequest.ts | 57 ++++++++++--------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index a719176bd7..616fdc1fd2 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -276,33 +276,36 @@ which uses additional API calls in order to detect whether to 'create', 'update' } } - const actions: Types.CommitAction[] = ( - ( - await Promise.all( - fileContents.map(async file => - getFileAction( - { file, targetPath }, - { repoID, branch: targetBranch! }, - api, - ctx, - remoteFiles, - ctx.input.commitAction, - ).then(action => ({ file, action })), - ), - ) - ).filter(o => o.action !== 'skip') as { - file: SerializedFile; - action: Types.CommitAction['action']; - }[] - ).map(({ file, action }) => ({ - action, - filePath: targetPath - ? path.posix.join(targetPath, file.path) - : file.path, - encoding: 'base64', - content: file.content.toString('base64'), - execute_filemode: file.executable, - })); + const actions: Types.CommitAction[] = + ctx.input.commitAction === 'skip' + ? [] + : ( + ( + await Promise.all( + fileContents.map(async file => + getFileAction( + { file, targetPath }, + { repoID, branch: targetBranch! }, + api, + ctx, + remoteFiles, + ctx.input.commitAction, + ).then(action => ({ file, action })), + ), + ) + ).filter(o => o.action !== 'skip') as { + file: SerializedFile; + action: Types.CommitAction['action']; + }[] + ).map(({ file, action }) => ({ + action, + filePath: targetPath + ? path.posix.join(targetPath, file.path) + : file.path, + encoding: 'base64', + content: file.content.toString('base64'), + execute_filemode: file.executable, + })); let createBranch: boolean; if (actions.length) { From 5b6feba582189f02b7d33dafa2d0c708d7234782 Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Tue, 1 Oct 2024 16:01:33 -0500 Subject: [PATCH 3/7] incorporate code review feedback: nesting depth Signed-off-by: Matt Benson --- .../src/actions/gitlabMergeRequest.ts | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 616fdc1fd2..58c461c5fa 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -51,24 +51,23 @@ async function getFileAction( ): Promise<'create' | 'delete' | 'update' | 'skip'> { if (defaultCommitAction === 'auto') { const filePath = path.join(fileInfo.targetPath ?? '', fileInfo.file.path); - if (remoteFiles) { - if (remoteFiles.some(remoteFile => remoteFile.path === filePath)) { - try { - const targetFile = await api.RepositoryFiles.show( - target.repoID, - filePath, - target.branch, - ); - if (computeSha256(fileInfo.file) === targetFile.content_sha256) { - return 'skip'; - } - } catch (error) { - ctx.logger.warn( - `Unable to retrieve detailed information for remote file ${filePath}`, - ); + + if (remoteFiles?.some(remoteFile => remoteFile.path === filePath)) { + try { + const targetFile = await api.RepositoryFiles.show( + target.repoID, + filePath, + target.branch, + ); + if (computeSha256(fileInfo.file) === targetFile.content_sha256) { + return 'skip'; } - return 'update'; + } catch (error) { + ctx.logger.warn( + `Unable to retrieve detailed information for remote file ${filePath}`, + ); } + return 'update'; } return 'create'; } From 368d8db74ac38395d055711ed3f561c86717a6cf Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Tue, 1 Oct 2024 16:21:56 -0500 Subject: [PATCH 4/7] code review: don't mix promise with await Signed-off-by: Matt Benson --- .../src/actions/gitlabMergeRequest.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 58c461c5fa..3ef66f5f6f 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -274,23 +274,23 @@ which uses additional API calls in order to detect whether to 'create', 'update' ); } } - const actions: Types.CommitAction[] = ctx.input.commitAction === 'skip' ? [] : ( ( await Promise.all( - fileContents.map(async file => - getFileAction( + fileContents.map(async file => { + const action = await getFileAction( { file, targetPath }, { repoID, branch: targetBranch! }, api, ctx, remoteFiles, ctx.input.commitAction, - ).then(action => ({ file, action })), - ), + ); + return { file, action }; + }), ) ).filter(o => o.action !== 'skip') as { file: SerializedFile; From fcc073ffd7220e9ef22ba08d53332deba21aba52 Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Tue, 1 Oct 2024 16:25:02 -0500 Subject: [PATCH 5/7] code review: don't pass entire context to file action computation Signed-off-by: Matt Benson --- .../src/actions/gitlabMergeRequest.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 3ef66f5f6f..7374aa2648 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -40,7 +40,7 @@ async function getFileAction( fileInfo: { file: SerializedFile; targetPath?: string }, target: { repoID: string; branch: string }, api: Gitlab, - ctx: { logger: Logger }, + logger: Logger, remoteFiles: Types.RepositoryTreeSchema[], defaultCommitAction: | 'create' @@ -63,7 +63,7 @@ async function getFileAction( return 'skip'; } } catch (error) { - ctx.logger.warn( + logger.warn( `Unable to retrieve detailed information for remote file ${filePath}`, ); } @@ -285,7 +285,7 @@ which uses additional API calls in order to detect whether to 'create', 'update' { file, targetPath }, { repoID, branch: targetBranch! }, api, - ctx, + ctx.logger, remoteFiles, ctx.input.commitAction, ); From be1ec2fd1f076e7989b9594ebcd2d7605fc51717 Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Mon, 7 Oct 2024 09:41:35 -0500 Subject: [PATCH 6/7] Update .changeset/few-hairs-compare.md Co-authored-by: Ben Lambert Signed-off-by: Matt Benson --- .changeset/few-hairs-compare.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/few-hairs-compare.md b/.changeset/few-hairs-compare.md index d717e07351..48cd7b7f25 100644 --- a/.changeset/few-hairs-compare.md +++ b/.changeset/few-hairs-compare.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-scaffolder-backend-module-gitlab': minor +'@backstage/plugin-scaffolder-backend-module-gitlab': patch --- GitLab MR: introduce 'skip' commit action. From 29788ea4e399871d8ecdb13f828aa7babae00bbc Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 15 Oct 2024 11:04:18 +0200 Subject: [PATCH 7/7] chore: move to loggerService instead Signed-off-by: blam --- .../src/actions/gitlabMergeRequest.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts index 7374aa2648..ec9ae7317d 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabMergeRequest.ts @@ -24,11 +24,13 @@ import { Gitlab, Types } from '@gitbeaker/core'; import path from 'path'; import { ScmIntegrationRegistry } from '@backstage/integration'; import { InputError } from '@backstage/errors'; -import { resolveSafeChildPath } from '@backstage/backend-plugin-api'; +import { + LoggerService, + resolveSafeChildPath, +} from '@backstage/backend-plugin-api'; import { createGitlabApi } from './helpers'; import { examples } from './gitlabMergeRequest.examples'; import { createHash } from 'crypto'; -import { Logger } from 'winston'; function computeSha256(file: SerializedFile): string { const hash = createHash('sha256'); @@ -40,7 +42,7 @@ async function getFileAction( fileInfo: { file: SerializedFile; targetPath?: string }, target: { repoID: string; branch: string }, api: Gitlab, - logger: Logger, + logger: LoggerService, remoteFiles: Types.RepositoryTreeSchema[], defaultCommitAction: | 'create'