From f36bcf908627a2bcb52184e0251864245f8b308f Mon Sep 17 00:00:00 2001 From: Phred Date: Sun, 15 Jun 2025 19:43:28 -0500 Subject: [PATCH 1/7] allow file deletion in pull requests Signed-off-by: Phred --- .changeset/social-insects-cheat.md | 5 + .../src/actions/githubPullRequest.test.ts | 103 +++++++++++++++++- .../src/actions/githubPullRequest.ts | 57 +++++++--- 3 files changed, 145 insertions(+), 20 deletions(-) create mode 100644 .changeset/social-insects-cheat.md diff --git a/.changeset/social-insects-cheat.md b/.changeset/social-insects-cheat.md new file mode 100644 index 0000000000..13593865c2 --- /dev/null +++ b/.changeset/social-insects-cheat.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-github': patch +--- + +Added support for deleting files 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 670af15595..a8a0c8d8b1 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts @@ -27,6 +27,7 @@ import fs from 'fs-extra'; import { createPublishGithubPullRequestAction } from './githubPullRequest'; import { createMockDirectory } from '@backstage/backend-test-utils'; import { createMockActionContext } from '@backstage/plugin-scaffolder-node-test-utils'; +import { DELETE_FILE } from 'octokit-plugin-create-pull-request'; type GithubPullRequestActionInput = ReturnType< typeof createPublishGithubPullRequestAction @@ -46,12 +47,12 @@ describe('createPublishGithubPullRequestAction', () => { let config: Config; let integrations: ScmIntegrations; + const deletionMarker = + 'if-you-find-a-file-whose-contents-matches-this-delete-it'; const mockDir = createMockDirectory(); const workspacePath = mockDir.resolve('workspace'); beforeEach(() => { - mockDir.clear(); - config = new ConfigReader({}); integrations = ScmIntegrations.fromConfig(config); fakeClient = { @@ -92,6 +93,7 @@ describe('createPublishGithubPullRequestAction', () => { }); afterEach(() => { + mockDir.clear(); jest.resetAllMocks(); }); @@ -304,6 +306,103 @@ describe('createPublishGithubPullRequestAction', () => { }); }); + describe('with deletionMarker', () => { + let input: GithubPullRequestActionInput; + let ctx: ActionContext; + + beforeEach(() => { + input = { + deletionMarker, + repoUrl: 'github.com?owner=myorg&repo=myrepo', + title: 'Create my new app', + branchName: 'new-app', + description: 'This PR is really good', + }; + + mockDir.setContent({ + [workspacePath]: { + 'catpants.md': 'cat + pants', + 'foobar.txt': 'Hello there!', + }, + }); + + ctx = createMockActionContext({ input, workspacePath }); + }); + + it('should create a pull request when no files match the marker', async () => { + await instance.handler(ctx); + + expect(fakeClient.createPullRequest).toHaveBeenCalledWith({ + owner: 'myorg', + repo: 'myrepo', + title: input.title, + head: input.branchName, + body: input.description, + changes: [ + { + commit: input.title, + files: { + 'catpants.md': { + content: Buffer.from('cat + pants').toString('base64'), + encoding: 'base64', + mode: '100644', + }, + 'foobar.txt': { + content: Buffer.from('Hello there!').toString('base64'), + encoding: 'base64', + mode: '100644', + }, + }, + }, + ], + }); + }); + + describe('when files are marked for deletion', () => { + beforeEach(() => { + mockDir.setContent({ + [workspacePath]: { + 'foo.txt': 'Hello there!', + 'im-here-to-be-deleted': deletionMarker, + 'baz.txt': 'baz text', + 'delete-me-too': deletionMarker, + }, + }); + }); + + it('should delete marked files', async () => { + await instance.handler(ctx); + + expect(fakeClient.createPullRequest).toHaveBeenCalledWith({ + owner: 'myorg', + repo: 'myrepo', + title: input.title, + head: input.branchName, + body: input.description, + changes: [ + { + commit: input.title, + files: { + 'foo.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, + }, + }, + ], + }); + }); + }); + }); + describe('with repoUrl', () => { let input: GithubPullRequestActionInput; let ctx: ActionContext; diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts index 54b90e3544..c334d27781 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts @@ -27,7 +27,10 @@ import { } from '@backstage/plugin-scaffolder-node'; import { Octokit } from 'octokit'; import { CustomErrorBase, InputError } from '@backstage/errors'; -import { createPullRequest } from 'octokit-plugin-create-pull-request'; +import { + createPullRequest, + DELETE_FILE, +} from 'octokit-plugin-create-pull-request'; import { getOctokitOptions } from '../util'; import { examples } from './githubPullRequest.examples'; import { @@ -143,6 +146,15 @@ export const createPublishGithubPullRequestAction = ( z.string({ description: 'The name for the branch', }), + deletionMarker: z => + z + .string({ + description: 'Contents of files that will be deleted', + }) + .min(33, { + message: 'deletion marker must be at least 33 characters long', + }) + .optional(), targetBranchName: z => z .string({ @@ -269,6 +281,7 @@ export const createPublishGithubPullRequestAction = ( const { repoUrl, branchName, + deletionMarker, targetBranchName, title, description, @@ -323,24 +336,32 @@ 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 => [ - 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)), - }, - ]), + 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, + }, + ]; + }), ); // If this is a dry run, log and return From 3312901cf292632fac347b963d178882d0934e4c Mon Sep 17 00:00:00 2001 From: Phred Date: Sun, 15 Jun 2025 21:01:49 -0500 Subject: [PATCH 2/7] added API report Signed-off-by: Phred --- plugins/scaffolder-backend-module-github/report.api.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/scaffolder-backend-module-github/report.api.md b/plugins/scaffolder-backend-module-github/report.api.md index 28f710557a..5c9d490fdc 100644 --- a/plugins/scaffolder-backend-module-github/report.api.md +++ b/plugins/scaffolder-backend-module-github/report.api.md @@ -247,8 +247,8 @@ export function createGithubRepoCreateAction(options: { access: string; } | { - team: string; access: string; + team: string; } )[] | undefined; @@ -406,8 +406,8 @@ export function createPublishGithubAction(options: { access: string; } | { - team: string; access: string; + team: string; } )[] | undefined; @@ -446,6 +446,7 @@ export const createPublishGithubPullRequestAction: ( branchName: string; title: string; description: string; + deletionMarker?: string | undefined; targetBranchName?: string | undefined; draft?: boolean | undefined; sourcePath?: string | undefined; From db7a11502efa30fcae7231ded843de0b0fc56010 Mon Sep 17 00:00:00 2001 From: Phred Date: Sun, 15 Jun 2025 21:33:17 -0500 Subject: [PATCH 3/7] added API report (again) Signed-off-by: Phred After `yarn install` and `yarn build:api-reports` Signed-off-by: benjdlambert --- plugins/scaffolder-backend-module-github/report.api.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/scaffolder-backend-module-github/report.api.md b/plugins/scaffolder-backend-module-github/report.api.md index 5c9d490fdc..2a896836de 100644 --- a/plugins/scaffolder-backend-module-github/report.api.md +++ b/plugins/scaffolder-backend-module-github/report.api.md @@ -247,8 +247,8 @@ export function createGithubRepoCreateAction(options: { access: string; } | { - access: string; team: string; + access: string; } )[] | undefined; @@ -406,8 +406,8 @@ export function createPublishGithubAction(options: { access: string; } | { - access: string; team: string; + access: string; } )[] | undefined; From 116468461f0e14056c8f1f73e889ad197e56ce72 Mon Sep 17 00:00:00 2001 From: Phred Date: Mon, 16 Jun 2025 20:08:23 -0500 Subject: [PATCH 4/7] moved variable within test scope Signed-off-by: Phred --- .../src/actions/githubPullRequest.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 a8a0c8d8b1..26553f68ca 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts @@ -47,8 +47,6 @@ describe('createPublishGithubPullRequestAction', () => { let config: Config; let integrations: ScmIntegrations; - const deletionMarker = - 'if-you-find-a-file-whose-contents-matches-this-delete-it'; const mockDir = createMockDirectory(); const workspacePath = mockDir.resolve('workspace'); @@ -307,6 +305,9 @@ describe('createPublishGithubPullRequestAction', () => { }); describe('with deletionMarker', () => { + const deletionMarker = + 'if-you-find-a-file-whose-contents-matches-this-delete-it'; + let input: GithubPullRequestActionInput; let ctx: ActionContext; From 265dd73dcf2c9ca4323ce000bcd5eaf8b3e0f0a7 Mon Sep 17 00:00:00 2001 From: Phred Date: Thu, 19 Jun 2025 03:18:51 -0500 Subject: [PATCH 5/7] 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`); From 411ed753b2011a7e2a221c302fb98194dee4c2d8 Mon Sep 17 00:00:00 2001 From: Phred Date: Mon, 23 Jun 2025 07:02:31 -0500 Subject: [PATCH 6/7] removed `Object.assign` usage based on code review Signed-off-by: Phred --- .../src/actions/githubPullRequest.ts | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts index 3b4ba5c83f..7d1e88d0c7 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts @@ -333,8 +333,8 @@ export const createPublishGithubPullRequestAction = ( file: SerializedFile, ): 'utf-8' | 'base64' => (file.symlink ? 'utf-8' : 'base64'); - const files = Object.fromEntries( - directoryContents.map(file => [ + const files = Object.fromEntries([ + ...directoryContents.map(file => [ targetPath ? path.posix.join(targetPath, file.path) : file.path, { // See the properties of tree items @@ -351,19 +351,13 @@ export const createPublishGithubPullRequestAction = ( 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, - ]), - ), - ); - } + // order of arrays is important so filesToDelete will overwrite + // changes from files above + ...(filesToDelete || []).map(filePath => [ + targetPath ? path.posix.join(targetPath, filePath) : filePath, + DELETE_FILE, + ]), + ]); // If this is a dry run, log and return if (ctx.isDryRun) { From c2b9797d266ffb83bb197e10165c2b6882013eb5 Mon Sep 17 00:00:00 2001 From: Phred Date: Mon, 23 Jun 2025 07:32:08 -0500 Subject: [PATCH 7/7] added sample usage to changelog Signed-off-by: Phred --- .changeset/social-insects-cheat.md | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/.changeset/social-insects-cheat.md b/.changeset/social-insects-cheat.md index 13593865c2..494cba15e7 100644 --- a/.changeset/social-insects-cheat.md +++ b/.changeset/social-insects-cheat.md @@ -2,4 +2,20 @@ '@backstage/plugin-scaffolder-backend-module-github': patch --- -Added support for deleting files +Added support for file deletion to `publish:github:pull-request` action. + +Example usage: + +```diff + - action: publish:github:pull-request + id: clean-up-pr + input: + description: This is the description ++ filesToDelete: ++ - outdated/changelog.md ++ - sample-file.txt + owner: owner + repo: repo + title: Title Goes Here + +```