From e55a5dea094b75318b6d5222924c29166900bb9f Mon Sep 17 00:00:00 2001 From: Kenneth Feng Date: Tue, 19 Oct 2021 21:49:41 -0400 Subject: [PATCH 1/5] plugin/scaffolder: set the file mode in publish:github:pull-request. The ability to set file mode was introduced in octokit-plugin-create-pull-request:v3.10. This PR updates the underlying octokit-plugin-create-pull-request version to 3.10, and sets the file mode when creating the pull request. Signed-off-by: Kenneth Feng --- .changeset/two-cougars-breathe.md | 5 ++ plugins/scaffolder-backend/package.json | 2 +- .../builtin/publish/githubPullRequest.test.ts | 86 ++++++++++++++++++- .../builtin/publish/githubPullRequest.ts | 19 +++- 4 files changed, 105 insertions(+), 7 deletions(-) create mode 100644 .changeset/two-cougars-breathe.md diff --git a/.changeset/two-cougars-breathe.md b/.changeset/two-cougars-breathe.md new file mode 100644 index 0000000000..0b61078e85 --- /dev/null +++ b/.changeset/two-cougars-breathe.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Fixed bug where the mode of an executable file was ignored diff --git a/plugins/scaffolder-backend/package.json b/plugins/scaffolder-backend/package.json index ea29866485..643ab6e30c 100644 --- a/plugins/scaffolder-backend/package.json +++ b/plugins/scaffolder-backend/package.json @@ -64,7 +64,7 @@ "luxon": "^2.0.2", "morgan": "^1.10.0", "nunjucks": "^3.2.3", - "octokit-plugin-create-pull-request": "^3.9.3", + "octokit-plugin-create-pull-request": "^3.10.0", "uuid": "^8.2.0", "winston": "^3.2.1", "yaml": "^1.10.0" diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts index ed72b14a65..dbb5f3caff 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts @@ -98,7 +98,11 @@ describe('createPublishGithubPullRequestAction', () => { { commit: 'Create my new app', files: { - 'file.txt': 'Hello there!', + 'file.txt': { + encoding: 'utf-8', + content: 'Hello there!', + mode: '100644', + }, }, }, ], @@ -167,7 +171,11 @@ describe('createPublishGithubPullRequestAction', () => { { commit: 'Create my new app', files: { - 'foo.txt': 'Hello there!', + 'foo.txt': { + content: 'Hello there!', + encoding: 'utf-8', + mode: '100644', + }, }, }, ], @@ -221,7 +229,79 @@ describe('createPublishGithubPullRequestAction', () => { { commit: 'Create my new app', files: { - 'file.txt': 'Hello there!', + 'file.txt': { + content: 'Hello there!', + encoding: 'utf-8', + mode: '100644', + }, + }, + }, + ], + }); + }); + + it('creates outputs for the url', async () => { + await instance.handler(ctx); + + expect(ctx.output).toHaveBeenCalledWith( + 'remoteUrl', + 'https://github.com/myorg/myrepo/pull/123', + ); + }); + afterEach(() => { + mockFs.restore(); + jest.resetAllMocks(); + }); + }); + + describe('with executable file', () => { + let input: GithubPullRequestActionInput; + let ctx: ActionContext; + + beforeEach(() => { + input = { + repoUrl: 'github.com?owner=myorg&repo=myrepo', + title: 'Create my new app', + branchName: 'new-app', + description: 'This PR is really good', + }; + + mockFs({ + [workspacePath]: { + 'file.txt': mockFs.file({ + content: 'Hello there!', + mode: 33277, // File mode: 100755 + }), + }, + }); + + ctx = { + createTemporaryDirectory: jest.fn(), + output: jest.fn(), + logger: getRootLogger(), + logStream: new Writable(), + input, + workspacePath, + }; + }); + it('creates a pull request', async () => { + await instance.handler(ctx); + + expect(fakeClient.createPullRequest).toHaveBeenCalledWith({ + owner: 'myorg', + repo: 'myrepo', + title: 'Create my new app', + head: 'new-app', + body: 'This PR is really good', + changes: [ + { + commit: 'Create my new app', + files: { + 'file.txt': { + content: 'Hello there!', + encoding: 'utf-8', + mode: '100755', + }, }, }, ], diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index 253fd72b38..733fd673ff 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { readFile } from 'fs-extra'; +import fs from 'fs-extra'; import path from 'path'; import { parseRepoUrl } from './util'; @@ -194,7 +194,20 @@ export const createPublishGithubPullRequestAction = ({ }); const fileContents = await Promise.all( - localFilePaths.map(p => readFile(path.resolve(fileRoot, p))), + localFilePaths.map(filePath => { + const absPath = path.resolve(fileRoot, filePath); + const content = fs.readFileSync(absPath).toString(); + const fileStat = fs.statSync(absPath); + const isExecutable = fileStat.mode === 33277 // aka. 100755; + // See the properties of tree items + // in https://docs.github.com/en/rest/reference/git#trees + const githubTreeItemMode = isExecutable ? '100755' : '100644'; + return { + encoding: 'utf-8', + content: content, + mode: githubTreeItemMode, + }; + }), ); const repoFilePaths = localFilePaths.map(repoFilePath => { @@ -205,7 +218,7 @@ export const createPublishGithubPullRequestAction = ({ { files: zipObject( repoFilePaths, - fileContents.map(buf => buf.toString()), + fileContents, ), commit: title, }, From b9602099765c3063cbcfef6374c5f6735033db98 Mon Sep 17 00:00:00 2001 From: Kenneth Feng Date: Fri, 22 Oct 2021 10:01:55 -0400 Subject: [PATCH 2/5] fix style issues Signed-off-by: Kenneth Feng --- .../actions/builtin/publish/githubPullRequest.test.ts | 6 +++--- .../actions/builtin/publish/githubPullRequest.ts | 7 ++----- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts index dbb5f3caff..1de9a7e268 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts @@ -172,9 +172,9 @@ describe('createPublishGithubPullRequestAction', () => { commit: 'Create my new app', files: { 'foo.txt': { - content: 'Hello there!', - encoding: 'utf-8', - mode: '100644', + content: 'Hello there!', + encoding: 'utf-8', + mode: '100644', }, }, }, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index 733fd673ff..aec59c86fd 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -198,7 +198,7 @@ export const createPublishGithubPullRequestAction = ({ const absPath = path.resolve(fileRoot, filePath); const content = fs.readFileSync(absPath).toString(); const fileStat = fs.statSync(absPath); - const isExecutable = fileStat.mode === 33277 // aka. 100755; + const isExecutable = fileStat.mode === 33277; // aka. 100755 // See the properties of tree items // in https://docs.github.com/en/rest/reference/git#trees const githubTreeItemMode = isExecutable ? '100755' : '100644'; @@ -216,10 +216,7 @@ export const createPublishGithubPullRequestAction = ({ const changes = [ { - files: zipObject( - repoFilePaths, - fileContents, - ), + files: zipObject(repoFilePaths, fileContents), commit: title, }, ]; From 7a5cb137b599094a019904ad6053031829dfb18c Mon Sep 17 00:00:00 2001 From: Kenneth Feng Date: Fri, 22 Oct 2021 21:45:04 -0400 Subject: [PATCH 3/5] Always use base64 encoding to prevent interpreting binary files as utf-8 text files Signed-off-by: Kenneth Feng --- .../builtin/publish/githubPullRequest.test.ts | 16 ++++++++-------- .../actions/builtin/publish/githubPullRequest.ts | 14 +++++++++++--- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts index 1de9a7e268..27c6f1e59d 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts @@ -99,8 +99,8 @@ describe('createPublishGithubPullRequestAction', () => { commit: 'Create my new app', files: { 'file.txt': { - encoding: 'utf-8', - content: 'Hello there!', + content: Buffer.from('Hello there!').toString('base64'), + encoding: 'base64', mode: '100644', }, }, @@ -172,8 +172,8 @@ describe('createPublishGithubPullRequestAction', () => { commit: 'Create my new app', files: { 'foo.txt': { - content: 'Hello there!', - encoding: 'utf-8', + content: Buffer.from('Hello there!').toString('base64'), + encoding: 'base64', mode: '100644', }, }, @@ -230,8 +230,8 @@ describe('createPublishGithubPullRequestAction', () => { commit: 'Create my new app', files: { 'file.txt': { - content: 'Hello there!', - encoding: 'utf-8', + content: Buffer.from('Hello there!').toString('base64'), + encoding: 'base64', mode: '100644', }, }, @@ -298,8 +298,8 @@ describe('createPublishGithubPullRequestAction', () => { commit: 'Create my new app', files: { 'file.txt': { - content: 'Hello there!', - encoding: 'utf-8', + content: Buffer.from('Hello there!').toString('base64'), + encoding: 'base64', mode: '100755', }, }, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index aec59c86fd..21361072ef 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -196,15 +196,23 @@ export const createPublishGithubPullRequestAction = ({ const fileContents = await Promise.all( localFilePaths.map(filePath => { const absPath = path.resolve(fileRoot, filePath); - const content = fs.readFileSync(absPath).toString(); + const base64EncodedContent = fs + .readFileSync(absPath) + .toString('base64'); const fileStat = fs.statSync(absPath); const isExecutable = fileStat.mode === 33277; // aka. 100755 // See the properties of tree items // in https://docs.github.com/en/rest/reference/git#trees const githubTreeItemMode = isExecutable ? '100755' : '100644'; + // Always use base64 encoding 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. + // + // 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) return { - encoding: 'utf-8', - content: content, + encoding: 'base64', + content: base64EncodedContent, mode: githubTreeItemMode, }; }), From ce024e93ce042faedd4ff238c8bb111fa4107166 Mon Sep 17 00:00:00 2001 From: Kenneth Feng Date: Mon, 25 Oct 2021 10:16:44 -0400 Subject: [PATCH 4/5] Cast string 'base64' to the compatible Encoding type. Signed-off-by: Kenneth Feng --- .../scaffolder/actions/builtin/publish/githubPullRequest.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index 21361072ef..e147f49b23 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -30,6 +30,8 @@ import { createPullRequest } from 'octokit-plugin-create-pull-request'; import globby from 'globby'; import { resolveSafeChildPath } from '@backstage/backend-common'; +export type Encoding = 'utf-8' | 'base64'; + class GithubResponseError extends CustomErrorBase {} type CreatePullRequestResponse = { @@ -210,8 +212,9 @@ export const createPublishGithubPullRequestAction = ({ // // 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) + const encoding: Encoding = 'base64'; return { - encoding: 'base64', + encoding: encoding, content: base64EncodedContent, mode: githubTreeItemMode, }; From 004a23b5fcb88a645a7ad0c06cbba53b9cd96c4b Mon Sep 17 00:00:00 2001 From: Kenneth Feng Date: Mon, 25 Oct 2021 10:18:56 -0400 Subject: [PATCH 5/5] update octokit-plugin-create-pull-request in yarn.lock Signed-off-by: Kenneth Feng --- yarn.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/yarn.lock b/yarn.lock index c905fbea6f..2b0af3e7fc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -21340,10 +21340,10 @@ obuf@^1.0.0, obuf@^1.1.2: resolved "https://registry.npmjs.org/obuf/-/obuf-1.1.2.tgz#09bea3343d41859ebd446292d11c9d4db619084e" integrity sha512-PX1wu0AmAdPqOL1mWhqmlOd8kOIZQwGZw6rh7uby9fTc5lhaOWFLX3I6R1hrF9k3zUY40e6igsLGkDXK92LJNg== -octokit-plugin-create-pull-request@^3.9.3: - version "3.9.3" - resolved "https://registry.npmjs.org/octokit-plugin-create-pull-request/-/octokit-plugin-create-pull-request-3.9.3.tgz#f99f53907ac322a3494cc970514a023d7b659e2b" - integrity sha512-lTyNnCRoT4IvCQx2Cb4eFMqg8aIpsaDd59MNwf4OPnWAJM7hT6g7RW/icImvAzZLR4t5ENSLNzWarv2XqLL+Lg== +octokit-plugin-create-pull-request@^3.10.0: + version "3.10.0" + resolved "https://registry.npmjs.org/octokit-plugin-create-pull-request/-/octokit-plugin-create-pull-request-3.10.0.tgz#c9a589e0014e949eadd24a03ace10565007784d5" + integrity sha512-QU3nk62+OimV7ki+pV90cXoqqbUAQLdbqccS7/cNajdjQ2KYmaakz21FL1y78a5N0mA2P4WOs0o2+aunvbWI0w== dependencies: "@octokit/types" "^6.8.2"