From a8e98484792edd0266bdb6125f0a719c0170a7c9 Mon Sep 17 00:00:00 2001 From: Arthur Gavlyukovskiy Date: Wed, 5 Oct 2022 21:12:53 +0200 Subject: [PATCH] Added sourcePath to 'publish:gitlab:merge-request' action. Made both `sourcePath` and `targetPath` optional with fallback to `workspacePath`. Because currently `targetPath` serves as both source and target, falling back to `targetPath` if `sourcePath` is not specified. Also removed `draft` option from tests, because it's not supported in gitlab (probably copied from github tests). Signed-off-by: Arthur Gavlyukovskiy --- .changeset/unlucky-seas-sip.md | 5 + plugins/scaffolder-backend/api-report.md | 1 + .../publish/gitlabMergeRequest.test.ts | 172 ++++++++++++++++-- .../builtin/publish/gitlabMergeRequest.ts | 74 +++++--- 4 files changed, 211 insertions(+), 41 deletions(-) create mode 100644 .changeset/unlucky-seas-sip.md diff --git a/.changeset/unlucky-seas-sip.md b/.changeset/unlucky-seas-sip.md new file mode 100644 index 0000000000..682a34f3d7 --- /dev/null +++ b/.changeset/unlucky-seas-sip.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Added optional `sourcePath` parameter to `publish:gitlab:merge-request` action, `targetPath` is now optional and falls back to current workspace path. diff --git a/plugins/scaffolder-backend/api-report.md b/plugins/scaffolder-backend/api-report.md index ca5f68294b..3b2e174329 100644 --- a/plugins/scaffolder-backend/api-report.md +++ b/plugins/scaffolder-backend/api-report.md @@ -429,6 +429,7 @@ export const createPublishGitlabMergeRequestAction: (options: { title: string; description: string; branchName: string; + sourcePath: string; targetPath: string; token?: string | undefined; commitAction?: 'update' | 'create' | 'delete' | undefined; diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.test.ts index 76fe60a2b0..d1888fd9d2 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.test.ts @@ -108,7 +108,6 @@ describe('createGitLabMergeRequest', () => { title: 'Create my new MR', branchName: 'new-mr', description: 'This MR is really good', - draft: true, targetPath: 'Subdirectory', }; mockFs({ @@ -145,7 +144,6 @@ describe('createGitLabMergeRequest', () => { branchName: 'new-mr', description: 'MR description', removeSourceBranch: true, - draft: true, targetPath: 'Subdirectory', }; mockFs({ @@ -181,7 +179,6 @@ describe('createGitLabMergeRequest', () => { branchName: 'new-mr', description: 'other MR description', removeSourceBranch: false, - draft: true, targetPath: 'Subdirectory', }; mockFs({ @@ -222,7 +219,6 @@ describe('createGitLabMergeRequest', () => { branchName: 'new-mr', description: 'This is an important change', removeSourceBranch: false, - draft: true, targetPath: 'Subdirectory', assignee: 'John Smith', }; @@ -263,7 +259,6 @@ describe('createGitLabMergeRequest', () => { branchName: 'new-mr', description: 'This is an important change', removeSourceBranch: false, - draft: true, targetPath: 'Subdirectory', assingnee: 'John Doe', }; @@ -305,7 +300,6 @@ describe('createGitLabMergeRequest', () => { branchName: 'new-mr', description: 'This is an important change', removeSourceBranch: false, - draft: true, targetPath: 'Subdirectory', }; mockFs({ @@ -344,7 +338,6 @@ describe('createGitLabMergeRequest', () => { branchName: 'new-mr', description: 'This is an important change', removeSourceBranch: false, - draft: true, targetPath: 'Subdirectory', assignee: 'Unknown', }; @@ -376,6 +369,52 @@ describe('createGitLabMergeRequest', () => { }, ); }); + + it('use workspacePath as default when no sourcePath or targetPath is specified', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo&owner=owner', + title: 'Create my new MR', + branchName: 'new-mr', + description: 'This MR is really good', + }; + mockFs({ + [workspacePath]: { + source: { 'foo.txt': 'Hello there!' }, + irrelevant: { 'bar.txt': 'Nothing to see here' }, + }, + }); + const ctx = { + createTemporaryDirectory: jest.fn(), + output: jest.fn(), + logger: getRootLogger(), + logStream: new Writable(), + input, + workspacePath, + }; + await instance.handler(ctx); + + expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'Create my new MR', + [ + { + action: 'create', + filePath: 'irrelevant/bar.txt', + encoding: 'base64', + content: 'Tm90aGluZyB0byBzZWUgaGVyZQ==', + execute_filemode: false, + }, + { + action: 'create', + filePath: 'source/foo.txt', + encoding: 'base64', + content: 'SGVsbG8gdGhlcmUh', + execute_filemode: false, + }, + ], + ); + }); }); describe('createGitLabMergeRequestWithoutCommitAction', () => { @@ -385,7 +424,6 @@ describe('createGitLabMergeRequest', () => { title: 'Create my new MR', branchName: 'new-mr', description: 'This MR is really good', - draft: true, targetPath: 'source', }; mockFs({ @@ -429,7 +467,6 @@ describe('createGitLabMergeRequest', () => { branchName: 'new-mr', description: 'MR description', commitAction: 'create', - draft: true, targetPath: 'source', }; mockFs({ @@ -472,7 +509,6 @@ describe('createGitLabMergeRequest', () => { branchName: 'new-mr', description: 'MR description', commitAction: 'update', - draft: true, targetPath: 'source', }; mockFs({ @@ -515,7 +551,6 @@ describe('createGitLabMergeRequest', () => { branchName: 'new-mr', description: 'other MR description', commitAction: 'delete', - draft: true, targetPath: 'source', }; mockFs({ @@ -551,4 +586,119 @@ describe('createGitLabMergeRequest', () => { ); }); }); + + describe('with sourcePath', () => { + it('creates a Merge Request with only relevant files', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo&owner=owner', + title: 'Create my new MR', + branchName: 'new-mr', + description: 'MR description', + sourcePath: 'source', + commitAction: 'create', + }; + + mockFs({ + [workspacePath]: { + source: { 'foo.txt': 'Hello there!' }, + irrelevant: { 'bar.txt': 'Nothing to see here' }, + }, + }); + + const ctx = { + createTemporaryDirectory: jest.fn(), + output: jest.fn(), + logger: getRootLogger(), + logStream: new Writable(), + input, + workspacePath, + }; + + await instance.handler(ctx); + + expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'Create my new MR', + [ + { + action: 'create', + filePath: 'foo.txt', + content: 'SGVsbG8gdGhlcmUh', + encoding: 'base64', + execute_filemode: false, + }, + ], + ); + }); + + it('creates a Merge Request with only relevant files placed under different targetPath', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo&owner=owner', + title: 'Create my new MR', + branchName: 'new-mr', + description: 'MR description', + sourcePath: 'source', + targetPath: 'target', + commitAction: 'create', + }; + + mockFs({ + [workspacePath]: { + source: { 'foo.txt': 'Hello there!' }, + irrelevant: { 'bar.txt': 'Nothing to see here' }, + }, + }); + + const ctx = { + createTemporaryDirectory: jest.fn(), + output: jest.fn(), + logger: getRootLogger(), + logStream: new Writable(), + input, + workspacePath, + }; + + await instance.handler(ctx); + + expect(mockGitlabClient.Commits.create).toHaveBeenCalledWith( + 'owner/repo', + 'new-mr', + 'Create my new MR', + [ + { + action: 'create', + filePath: 'target/foo.txt', + content: 'SGVsbG8gdGhlcmUh', + encoding: 'base64', + execute_filemode: false, + }, + ], + ); + }); + + it('should not allow to use files outside of the workspace', async () => { + const input = { + repoUrl: 'gitlab.com?repo=repo&owner=owner', + title: 'Create my new MR', + branchName: 'new-mr', + description: 'MR description', + sourcePath: '../../test', + commitAction: 'create', + }; + + const ctx = { + createTemporaryDirectory: jest.fn(), + output: jest.fn(), + logger: getRootLogger(), + logStream: new Writable(), + input, + workspacePath, + }; + + await expect(instance.handler(ctx)).rejects.toThrow( + 'Relative path is not allowed to refer to a directory outside its parent', + ); + }); + }); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.ts index 1410096a73..5a5dbf26a1 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.ts @@ -38,6 +38,7 @@ export const createPublishGitlabMergeRequestAction = (options: { title: string; description: string; branchName: string; + sourcePath: string; targetPath: string; token?: string; commitAction?: 'create' | 'delete' | 'update'; @@ -49,7 +50,7 @@ export const createPublishGitlabMergeRequestAction = (options: { id: 'publish:gitlab:merge-request', schema: { input: { - required: ['repoUrl', 'targetPath', 'branchName'], + required: ['repoUrl', 'branchName'], type: 'object', properties: { repoUrl: { @@ -78,6 +79,12 @@ export const createPublishGitlabMergeRequestAction = (options: { title: 'Destination Branch name', description: 'The description of the merge request', }, + sourcePath: { + type: 'string', + title: 'Working Subdirectory', + description: + 'Subdirectory of working directory to copy changes from', + }, targetPath: { type: 'string', title: 'Repository Subdirectory', @@ -128,7 +135,17 @@ export const createPublishGitlabMergeRequestAction = (options: { }, }, async handler(ctx) { - const repoUrl = ctx.input.repoUrl; + const { + assignee, + branchName, + description, + repoUrl, + removeSourceBranch, + targetPath, + sourcePath, + title, + token: providedToken, + } = ctx.input; const { host, owner, repo } = parseRepoUrl(repoUrl, integrations); const projectPath = `${owner}/${repo}`; @@ -140,28 +157,24 @@ export const createPublishGitlabMergeRequestAction = (options: { const integrationConfig = integrations.gitlab.byHost(host); - const destinationBranch = ctx.input.branchName; - if (!integrationConfig) { throw new InputError( `No matching integration configuration for host ${host}, please check your integrations config`, ); } - if (!integrationConfig.config.token && !ctx.input.token) { + if (!integrationConfig.config.token && !providedToken) { throw new InputError(`No token available for host ${host}`); } - const token = ctx.input.token ?? integrationConfig.config.token!; - const tokenType = ctx.input.token ? 'oauthToken' : 'token'; + const token = providedToken ?? integrationConfig.config.token!; + const tokenType = providedToken ? 'oauthToken' : 'token'; const api = new Gitlab({ host: integrationConfig.config.baseUrl, [tokenType]: token, }); - const assignee = ctx.input.assignee; - let assigneeId = undefined; if (assignee !== undefined) { @@ -175,17 +188,25 @@ export const createPublishGitlabMergeRequestAction = (options: { } } - const targetPath = resolveSafeChildPath( - ctx.workspacePath, - ctx.input.targetPath, - ); - const fileContents = await serializeDirectoryContents(targetPath, { + let fileRoot: string; + if (sourcePath) { + fileRoot = resolveSafeChildPath(ctx.workspacePath, sourcePath); + } else if (targetPath) { + // for backward compatibility + fileRoot = resolveSafeChildPath(ctx.workspacePath, targetPath); + } else { + fileRoot = ctx.workspacePath; + } + + const fileContents = await serializeDirectoryContents(fileRoot, { gitignore: true, }); const actions: Types.CommitAction[] = fileContents.map(file => ({ action: ctx.input.commitAction ?? 'create', - filePath: path.posix.join(ctx.input.targetPath, file.path), + filePath: targetPath + ? path.posix.join(targetPath, file.path) + : file.path, encoding: 'base64', content: file.content.toString('base64'), execute_filemode: file.executable, @@ -197,7 +218,7 @@ export const createPublishGitlabMergeRequestAction = (options: { try { await api.Branches.create( projectPath, - destinationBranch, + branchName, String(defaultBranch), ); } catch (e) { @@ -205,30 +226,23 @@ export const createPublishGitlabMergeRequestAction = (options: { } try { - await api.Commits.create( - projectPath, - destinationBranch, - ctx.input.title, - actions, - ); + await api.Commits.create(projectPath, branchName, title, actions); } catch (e) { throw new InputError( - `Committing the changes to ${destinationBranch} failed ${e}`, + `Committing the changes to ${branchName} failed ${e}`, ); } try { const mergeRequestUrl = await api.MergeRequests.create( projectPath, - destinationBranch, + branchName, String(defaultBranch), - ctx.input.title, + title, { - description: ctx.input.description, - removeSourceBranch: ctx.input.removeSourceBranch - ? ctx.input.removeSourceBranch - : false, - assigneeId: assigneeId, + description, + removeSourceBranch: removeSourceBranch ? removeSourceBranch : false, + assigneeId, }, ).then((mergeRequest: { web_url: string }) => { return mergeRequest.web_url;