From 265dd73dcf2c9ca4323ce000bcd5eaf8b3e0f0a7 Mon Sep 17 00:00:00 2001 From: Phred Date: Thu, 19 Jun 2025 03:18:51 -0500 Subject: [PATCH] refactored based on reviewer feedback to use `filesToDelete` array Signed-off-by: Phred --- .../report.api.md | 2 +- .../src/actions/githubPullRequest.test.ts | 53 +++++++++------ .../src/actions/githubPullRequest.ts | 65 ++++++++++--------- 3 files changed, 67 insertions(+), 53 deletions(-) diff --git a/plugins/scaffolder-backend-module-github/report.api.md b/plugins/scaffolder-backend-module-github/report.api.md index 2a896836de..2941be7702 100644 --- a/plugins/scaffolder-backend-module-github/report.api.md +++ b/plugins/scaffolder-backend-module-github/report.api.md @@ -446,7 +446,7 @@ export const createPublishGithubPullRequestAction: ( branchName: string; title: string; description: string; - deletionMarker?: string | undefined; + filesToDelete?: string[] | undefined; targetBranchName?: string | undefined; draft?: boolean | undefined; sourcePath?: string | undefined; diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts index 26553f68ca..5a8019579b 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts @@ -24,6 +24,7 @@ import { TemplateAction, } from '@backstage/plugin-scaffolder-node'; import fs from 'fs-extra'; +import path from 'node:path'; import { createPublishGithubPullRequestAction } from './githubPullRequest'; import { createMockDirectory } from '@backstage/backend-test-utils'; import { createMockActionContext } from '@backstage/plugin-scaffolder-node-test-utils'; @@ -304,16 +305,13 @@ describe('createPublishGithubPullRequestAction', () => { }); }); - describe('with deletionMarker', () => { - const deletionMarker = - 'if-you-find-a-file-whose-contents-matches-this-delete-it'; - + describe('with filesToDelete', () => { let input: GithubPullRequestActionInput; let ctx: ActionContext; beforeEach(() => { input = { - deletionMarker, + filesToDelete: ['changed-file-to-delete.txt', 'delete-me-too.md'], repoUrl: 'github.com?owner=myorg&repo=myrepo', title: 'Create my new app', branchName: 'new-app', @@ -323,6 +321,7 @@ describe('createPublishGithubPullRequestAction', () => { mockDir.setContent({ [workspacePath]: { 'catpants.md': 'cat + pants', + 'changed-file-to-delete.txt': 'file is changed and deleted', 'foobar.txt': 'Hello there!', }, }); @@ -330,7 +329,7 @@ describe('createPublishGithubPullRequestAction', () => { ctx = createMockActionContext({ input, workspacePath }); }); - it('should create a pull request when no files match the marker', async () => { + it('should delete named files', async () => { await instance.handler(ctx); expect(fakeClient.createPullRequest).toHaveBeenCalledWith({ @@ -353,25 +352,37 @@ describe('createPublishGithubPullRequestAction', () => { encoding: 'base64', mode: '100644', }, + 'changed-file-to-delete.txt': DELETE_FILE, + 'delete-me-too.md': DELETE_FILE, }, }, ], }); }); - describe('when files are marked for deletion', () => { + describe('with targetPath', () => { + const targetPath = `target-path-${Date.now()}`; + beforeEach(() => { + Object.assign(input, { + filesToDelete: [ + path.posix.join('nested', 'catpants.md'), + path.posix.join('nested', 'delete-me.too'), + ], + targetPath, + }); + mockDir.setContent({ [workspacePath]: { - 'foo.txt': 'Hello there!', - 'im-here-to-be-deleted': deletionMarker, - 'baz.txt': 'baz text', - 'delete-me-too': deletionMarker, + 'catpants.md': 'cat + pants', + 'foobar.txt': 'Hello there!', + [path.posix.join('nested', 'catpants.md')]: 'delete me', + [path.posix.join('nested', 'delete-me.too')]: 'delete me too', }, }); }); - it('should delete marked files', async () => { + it('should delete named files', async () => { await instance.handler(ctx); expect(fakeClient.createPullRequest).toHaveBeenCalledWith({ @@ -384,18 +395,20 @@ describe('createPublishGithubPullRequestAction', () => { { commit: input.title, files: { - 'foo.txt': { + [path.posix.join(targetPath, 'catpants.md')]: { + content: Buffer.from('cat + pants').toString('base64'), + encoding: 'base64', + mode: '100644', + }, + [path.posix.join(targetPath, 'foobar.txt')]: { content: Buffer.from('Hello there!').toString('base64'), encoding: 'base64', mode: '100644', }, - 'im-here-to-be-deleted': DELETE_FILE, - 'baz.txt': { - content: Buffer.from('baz text').toString('base64'), - encoding: 'base64', - mode: '100644', - }, - 'delete-me-too': DELETE_FILE, + [path.posix.join(targetPath, 'nested', 'catpants.md')]: + DELETE_FILE, + [path.posix.join(targetPath, 'nested', 'delete-me.too')]: + DELETE_FILE, }, }, ], diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts index c334d27781..3b4ba5c83f 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts @@ -146,13 +146,10 @@ export const createPublishGithubPullRequestAction = ( z.string({ description: 'The name for the branch', }), - deletionMarker: z => + filesToDelete: z => z - .string({ - description: 'Contents of files that will be deleted', - }) - .min(33, { - message: 'deletion marker must be at least 33 characters long', + .array(z.string(), { + description: 'List of files that will be deleted', }) .optional(), targetBranchName: z => @@ -281,7 +278,7 @@ export const createPublishGithubPullRequestAction = ( const { repoUrl, branchName, - deletionMarker, + filesToDelete, targetBranchName, title, description, @@ -336,34 +333,38 @@ export const createPublishGithubPullRequestAction = ( file: SerializedFile, ): 'utf-8' | 'base64' => (file.symlink ? 'utf-8' : 'base64'); - const encodedDeletionMarker = - deletionMarker && Buffer.from(deletionMarker).toString('base64'); const files = Object.fromEntries( - directoryContents.map(file => { - const content = file.content.toString(determineFileEncoding(file)); - - return [ - targetPath ? path.posix.join(targetPath, file.path) : file.path, - content === encodedDeletionMarker - ? DELETE_FILE - : { - // See the properties of tree items - // in https://docs.github.com/en/rest/reference/git#trees - mode: determineFileMode(file), - // Always use base64 encoding where possible to avoid doubling a binary file in size - // due to interpreting a binary file as utf-8 and sending github - // the utf-8 encoded content. Symlinks are kept as utf-8 to avoid them - // being formatted as a series of scrambled characters - // - // For example, the original gradle-wrapper.jar is 57.8k in https://github.com/kennethzfeng/pull-request-test/pull/5/files. - // Its size could be doubled to 98.3K (See https://github.com/kennethzfeng/pull-request-test/pull/4/files) - encoding: determineFileEncoding(file), - content, - }, - ]; - }), + directoryContents.map(file => [ + targetPath ? path.posix.join(targetPath, file.path) : file.path, + { + // See the properties of tree items + // in https://docs.github.com/en/rest/reference/git#trees + mode: determineFileMode(file), + // Always use base64 encoding where possible to avoid doubling a binary file in size + // due to interpreting a binary file as utf-8 and sending github + // the utf-8 encoded content. Symlinks are kept as utf-8 to avoid them + // being formatted as a series of scrambled characters + // + // For example, the original gradle-wrapper.jar is 57.8k in https://github.com/kennethzfeng/pull-request-test/pull/5/files. + // Its size could be doubled to 98.3K (See https://github.com/kennethzfeng/pull-request-test/pull/4/files) + encoding: determineFileEncoding(file), + content: file.content.toString(determineFileEncoding(file)), + }, + ]), ); + if (filesToDelete) { + Object.assign( + files, + Object.fromEntries( + filesToDelete.map(filePath => [ + targetPath ? path.posix.join(targetPath, filePath) : filePath, + DELETE_FILE, + ]), + ), + ); + } + // If this is a dry run, log and return if (ctx.isDryRun) { ctx.logger.info(`Performing dry run of creating pull request`);