From 514eb0baf4ffa328d2a3a9ae55987809c200ca93 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 24 Aug 2022 11:14:09 -0700 Subject: [PATCH 1/3] feat: group commit files into batches --- src/bin/code-suggester.ts | 5 +++ src/bin/workflow.ts | 1 + src/github/commit-and-push.ts | 52 +++++++++++++++++-------- src/index.ts | 3 +- src/types.ts | 4 ++ test/cli.ts | 1 + test/commit-and-push.ts | 71 ++++++++++++++++++++++++++++++++++- 7 files changed, 119 insertions(+), 18 deletions(-) diff --git a/src/bin/code-suggester.ts b/src/bin/code-suggester.ts index c8b5d11d..510349cb 100644 --- a/src/bin/code-suggester.ts +++ b/src/bin/code-suggester.ts @@ -97,6 +97,11 @@ yargs default: [], type: 'array', }, + 'files-per-commit': { + describe: 'Number of files per commit. Defaults to 100', + default: 100, + type: 'number', + }, }) .command(REVIEW_PR_COMMAND, 'Review an open pull request', { 'upstream-repo': { diff --git a/src/bin/workflow.ts b/src/bin/workflow.ts index bc0e0be4..97b7064e 100644 --- a/src/bin/workflow.ts +++ b/src/bin/workflow.ts @@ -42,6 +42,7 @@ export function coerceUserCreatePullRequestOptions(): CreatePullRequestUserOptio fork: yargs.argv.fork as boolean, labels: yargs.argv.labels as string[], logger, + filesPerCommit: yargs.argv.filesPerCommit as number, }; } diff --git a/src/github/commit-and-push.ts b/src/github/commit-and-push.ts index 48b81c2c..a126861b 100644 --- a/src/github/commit-and-push.ts +++ b/src/github/commit-and-push.ts @@ -22,6 +22,8 @@ import { import {Octokit} from '@octokit/rest'; import {logger} from '../logger'; +const DEFAULT_FILES_PER_COMMIT = 100; + /** * Generate and return a GitHub tree object structure * containing the target change data @@ -53,6 +55,15 @@ export function generateTreeObjects(changes: Changes): TreeObject[] { return tree; } +function* inGroupsOf( + all: T[], + groupSize: number +): Generator { + for (let i = 0; i < all.length; i += groupSize) { + yield all.slice(i, i + groupSize); + } +} + /** * Upload and create a remote GitHub tree * and resolves with the new tree SHA. @@ -67,28 +78,32 @@ export async function createTree( octokit: Octokit, origin: RepoDomain, refHead: string, - tree: TreeObject[] + tree: TreeObject[], + filesPerCommit: number = DEFAULT_FILES_PER_COMMIT ): Promise { - const oldTreeSha = ( + let headTreeSha = ( await octokit.git.getCommit({ owner: origin.owner, repo: origin.repo, commit_sha: refHead, }) ).data.tree.sha; - logger.info('Got the latest commit tree'); - const treeSha = ( - await octokit.git.createTree({ - owner: origin.owner, - repo: origin.repo, - tree, - base_tree: oldTreeSha, - }) - ).data.sha; + logger.info(`Got the latest commit tree: ${headTreeSha}`); + for (const treeGroup of inGroupsOf(tree, filesPerCommit)) { + logger.debug(`Tree sha: ${headTreeSha}`); + headTreeSha = ( + await octokit.git.createTree({ + owner: origin.owner, + repo: origin.repo, + tree: treeGroup, + base_tree: headTreeSha, + }) + ).data.sha; + } logger.info( - `Successfully created a tree with the desired changes with SHA ${treeSha}` + `Successfully created a tree with the desired changes with SHA ${headTreeSha}` ); - return treeSha; + return headTreeSha; } /** @@ -166,11 +181,18 @@ export async function commitAndPush( changes: Changes, originBranch: BranchDomain, commitMessage: string, - force: boolean + force: boolean, + filesPerCommit: number = DEFAULT_FILES_PER_COMMIT ) { try { const tree = generateTreeObjects(changes); - const treeSha = await createTree(octokit, originBranch, refHead, tree); + const treeSha = await createTree( + octokit, + originBranch, + refHead, + tree, + filesPerCommit + ); const commitSha = await createCommit( octokit, originBranch, diff --git a/src/index.ts b/src/index.ts index 6a906599..bd4ad371 100644 --- a/src/index.ts +++ b/src/index.ts @@ -189,7 +189,8 @@ async function createPullRequest( changes, originBranch, gitHubConfigs.message, - gitHubConfigs.force + gitHubConfigs.force, + gitHubConfigs.filesPerCommit ); const description: Description = { diff --git a/src/types.ts b/src/types.ts index e4572d4f..446634fc 100644 --- a/src/types.ts +++ b/src/types.ts @@ -101,6 +101,8 @@ export interface CreatePullRequestUserOptions { draft?: boolean; // Optional logger to set logger?: Logger; + // Optional number of files per commit + filesPerCommit?: number; } /** @@ -125,6 +127,8 @@ export interface CreatePullRequest { primary: string; // Whether or not maintainers can modify the PR. maintainersCanModify: boolean; + // Optional number of files per commit + filesPerCommit?: number; } /** diff --git a/test/cli.ts b/test/cli.ts index 47053b57..8570970c 100644 --- a/test/cli.ts +++ b/test/cli.ts @@ -91,6 +91,7 @@ describe('Mapping pr yargs to create PR options', () => { fork: true, labels: ['automerge'], logger: console, + filesPerCommit: 100, }; sandbox.stub(yargs, 'argv').value({_: ['pr'], ...options}); assert.deepStrictEqual(coerceUserCreatePullRequestOptions(), options); diff --git a/test/commit-and-push.ts b/test/commit-and-push.ts index 4318a904..408475eb 100644 --- a/test/commit-and-push.ts +++ b/test/commit-and-push.ts @@ -246,6 +246,10 @@ describe('Commit and push function', async () => { const sandbox = sinon.createSandbox(); const oldHeadSha = 'OLD-head-Sha-asdf1234'; const changes: Changes = new Map(); + changes.set('foo.txt', { + mode: '100755', + content: 'some file content', + }); const origin: RepoDomain = { owner: 'Foo', repo: 'Bar', @@ -313,7 +317,14 @@ describe('Commit and push function', async () => { sandbox.assert.calledWithExactly(stubCreateTree, { owner: origin.owner, repo: origin.repo, - tree: [], + tree: [ + { + path: 'foo.txt', + mode: '100755', + type: 'blob', + content: 'some file content', + }, + ], base_tree: getCommitResponse.data.tree.sha, }); sandbox.assert.calledOnceWithExactly(stubCreateCommit, { @@ -342,6 +353,62 @@ describe('Commit and push function', async () => { sandbox.stub(octokit.git, 'getCommit').resolves(getCommitResponse); sandbox.stub(octokit.git, 'createTree').rejects(error); // tests - await assert.rejects(handler.createTree(octokit, origin, '', []), error); + await assert.rejects( + handler.createTree(octokit, origin, '', [ + {path: 'foo.txt', type: 'blob', mode: '100755'}, + ]), + error + ); + }); + it('groups files into batches', async () => { + const tree: TreeObject[] = []; + for (let i = 0; i < 10; i++) { + tree.push({ + path: `path${i}`, + type: 'blob', + mode: '100755', + }); + } + sandbox.stub(octokit.git, 'getCommit').resolves(getCommitResponse); + const createTreeStub = sandbox + .stub(octokit.git, 'createTree') + .resolves({ + data: { + sha: 'createdsha1', + url: 'unused', + truncated: false, + tree: [ + { + path: 'path1', + type: 'blob', + mode: '100755', + }, + ], + }, + headers: {}, + status: 201, + url: 'unused', + }) + .onSecondCall() + .resolves({ + data: { + sha: 'createdsha2', + url: 'unused', + truncated: false, + tree: [ + { + path: 'path1', + type: 'blob', + mode: '100755', + }, + ], + }, + headers: {}, + status: 201, + url: 'unused', + }); + const headTreeSha = await handler.createTree(octokit, origin, '', tree, 6); + assert.equal(headTreeSha, 'createdsha2'); + sinon.assert.calledTwice(createTreeStub); }); }); From 3fc71695d3ec47eda3d81b7b0fe4990ed95ba5a2 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 24 Aug 2022 12:32:46 -0700 Subject: [PATCH 2/3] test: split on commits, not trees --- src/bin/workflow.ts | 1 + src/default-options-handler.ts | 1 + src/github/commit-and-push.ts | 91 ++++++++++--------------------- src/github/create-commit.ts | 48 +++++++++++++++++ src/index.ts | 1 + test/commit-and-push.ts | 95 +++++++++++++++------------------ test/main-pr-option-defaults.ts | 4 ++ 7 files changed, 127 insertions(+), 114 deletions(-) create mode 100644 src/github/create-commit.ts diff --git a/src/bin/workflow.ts b/src/bin/workflow.ts index 97b7064e..d95ee799 100644 --- a/src/bin/workflow.ts +++ b/src/bin/workflow.ts @@ -62,6 +62,7 @@ async function createCommand() { const options = coerceUserCreatePullRequestOptions(); const changes = await git.getChanges(yargs.argv['git-dir'] as string); const octokit = new Octokit({auth: process.env.ACCESS_TOKEN}); + console.log(options); await createPullRequest(octokit, changes, options); } diff --git a/src/default-options-handler.ts b/src/default-options-handler.ts index 090cb578..db66d569 100644 --- a/src/default-options-handler.ts +++ b/src/default-options-handler.ts @@ -48,6 +48,7 @@ export function addPullRequestDefaults( ? options.primary : DEFAULT_PRIMARY_BRANCH, maintainersCanModify: options.maintainersCanModify === false ? false : true, + filesPerCommit: options.filesPerCommit, }; return pullRequestSettings; } diff --git a/src/github/commit-and-push.ts b/src/github/commit-and-push.ts index a126861b..f287a0a6 100644 --- a/src/github/commit-and-push.ts +++ b/src/github/commit-and-push.ts @@ -21,6 +21,7 @@ import { } from '../types'; import {Octokit} from '@octokit/rest'; import {logger} from '../logger'; +import {createCommit} from './create-commit'; const DEFAULT_FILES_PER_COMMIT = 100; @@ -78,63 +79,28 @@ export async function createTree( octokit: Octokit, origin: RepoDomain, refHead: string, - tree: TreeObject[], - filesPerCommit: number = DEFAULT_FILES_PER_COMMIT + tree: TreeObject[] ): Promise { - let headTreeSha = ( + const oldTreeSha = ( await octokit.git.getCommit({ owner: origin.owner, repo: origin.repo, commit_sha: refHead, }) ).data.tree.sha; - logger.info(`Got the latest commit tree: ${headTreeSha}`); - for (const treeGroup of inGroupsOf(tree, filesPerCommit)) { - logger.debug(`Tree sha: ${headTreeSha}`); - headTreeSha = ( - await octokit.git.createTree({ - owner: origin.owner, - repo: origin.repo, - tree: treeGroup, - base_tree: headTreeSha, - }) - ).data.sha; - } - logger.info( - `Successfully created a tree with the desired changes with SHA ${headTreeSha}` - ); - return headTreeSha; -} - -/** - * Create a commit with a repo snapshot SHA on top of the reference HEAD - * and resolves with the SHA of the commit. - * Rejects if GitHub V3 API fails with the GitHub error response - * @param {Octokit} octokit The authenticated octokit instance - * @param {RepoDomain} origin the the remote repository to push changes to - * @param {string} refHead the base of the new commit(s) - * @param {string} treeSha the tree SHA that this commit will point to - * @param {string} message the message of the new commit - * @returns {Promise} the new commit SHA - */ -export async function createCommit( - octokit: Octokit, - origin: RepoDomain, - refHead: string, - treeSha: string, - message: string -): Promise { - const commitData = ( - await octokit.git.createCommit({ + logger.info('Got the latest commit tree'); + const treeSha = ( + await octokit.git.createTree({ owner: origin.owner, repo: origin.repo, - message, - tree: treeSha, - parents: [refHead], + tree, + base_tree: oldTreeSha, }) - ).data; - logger.info(`Successfully created commit. See commit at ${commitData.url}`); - return commitData.sha; + ).data.sha; + logger.info( + `Successfully created a tree with the desired changes with SHA ${treeSha}` + ); + return treeSha; } /** @@ -186,21 +152,22 @@ export async function commitAndPush( ) { try { const tree = generateTreeObjects(changes); - const treeSha = await createTree( - octokit, - originBranch, - refHead, - tree, - filesPerCommit - ); - const commitSha = await createCommit( - octokit, - originBranch, - refHead, - treeSha, - commitMessage - ); - await updateRef(octokit, originBranch, commitSha, force); + for (const treeGroup of inGroupsOf(tree, filesPerCommit)) { + const treeSha = await createTree( + octokit, + originBranch, + refHead, + treeGroup + ); + refHead = await createCommit( + octokit, + originBranch, + refHead, + treeSha, + commitMessage + ); + } + await updateRef(octokit, originBranch, refHead, force); } catch (err) { ( err as Error diff --git a/src/github/create-commit.ts b/src/github/create-commit.ts new file mode 100644 index 00000000..7529291a --- /dev/null +++ b/src/github/create-commit.ts @@ -0,0 +1,48 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Octokit} from '@octokit/rest'; +import {RepoDomain} from '../types'; +import {logger} from '../logger'; + +/** + * Create a commit with a repo snapshot SHA on top of the reference HEAD + * and resolves with the SHA of the commit. + * Rejects if GitHub V3 API fails with the GitHub error response + * @param {Octokit} octokit The authenticated octokit instance + * @param {RepoDomain} origin the the remote repository to push changes to + * @param {string} refHead the base of the new commit(s) + * @param {string} treeSha the tree SHA that this commit will point to + * @param {string} message the message of the new commit + * @returns {Promise} the new commit SHA + */ +export async function createCommit( + octokit: Octokit, + origin: RepoDomain, + refHead: string, + treeSha: string, + message: string +): Promise { + const commitData = ( + await octokit.git.createCommit({ + owner: origin.owner, + repo: origin.repo, + message, + tree: treeSha, + parents: [refHead], + }) + ).data; + logger.info(`Successfully created commit. See commit at ${commitData.url}`); + return commitData.sha; +} diff --git a/src/index.ts b/src/index.ts index bd4ad371..262f5565 100644 --- a/src/index.ts +++ b/src/index.ts @@ -124,6 +124,7 @@ async function createPullRequest( return 0; } const gitHubConfigs = addPullRequestDefaults(options); + console.log(gitHubConfigs); logger.info('Starting GitHub PR workflow...'); const upstream: RepoDomain = { owner: gitHubConfigs.upstreamOwner, diff --git a/test/commit-and-push.ts b/test/commit-and-push.ts index 408475eb..8afc8e86 100644 --- a/test/commit-and-push.ts +++ b/test/commit-and-push.ts @@ -20,7 +20,9 @@ import {octokit, setup} from './util'; import * as sinon from 'sinon'; import {GetResponseTypeFromEndpointMethod} from '@octokit/types'; import * as handler from '../src/github/commit-and-push'; +import * as createCommitModule from '../src/github/create-commit'; import {Changes, FileData, TreeObject, RepoDomain} from '../src/types'; +import {createCommit} from '../src/github/create-commit'; type GetCommitResponse = GetResponseTypeFromEndpointMethod< typeof octokit.git.getCommit @@ -194,13 +196,7 @@ describe('Commit', () => { .stub(octokit.git, 'createCommit') .resolves(createCommitResponse); // tests - const sha = await handler.createCommit( - octokit, - origin, - head, - treeSha, - message - ); + const sha = await createCommit(octokit, origin, head, treeSha, message); assert.strictEqual(sha, createCommitResponse.data.sha); sandbox.assert.calledOnceWithExactly(stubCreateCommit, { owner: origin.owner, @@ -246,10 +242,6 @@ describe('Commit and push function', async () => { const sandbox = sinon.createSandbox(); const oldHeadSha = 'OLD-head-Sha-asdf1234'; const changes: Changes = new Map(); - changes.set('foo.txt', { - mode: '100755', - content: 'some file content', - }); const origin: RepoDomain = { owner: 'Foo', repo: 'Bar', @@ -289,6 +281,10 @@ describe('Commit and push function', async () => { sandbox.restore(); }); it('When everything works it calls functions with correct parameter values', async () => { + changes.set('foo.txt', { + mode: '100755', + content: 'some file content', + }); // setup const stubGetCommit = sandbox .stub(octokit.git, 'getCommit') @@ -361,54 +357,49 @@ describe('Commit and push function', async () => { ); }); it('groups files into batches', async () => { - const tree: TreeObject[] = []; for (let i = 0; i < 10; i++) { - tree.push({ - path: `path${i}`, - type: 'blob', + changes.set(`path${i}`, { mode: '100755', + content: 'some file content', }); } sandbox.stub(octokit.git, 'getCommit').resolves(getCommitResponse); - const createTreeStub = sandbox - .stub(octokit.git, 'createTree') - .resolves({ - data: { - sha: 'createdsha1', - url: 'unused', - truncated: false, - tree: [ - { - path: 'path1', - type: 'blob', - mode: '100755', - }, - ], - }, - headers: {}, - status: 201, - url: 'unused', - }) + const createCommitStub = sandbox + .stub(createCommitModule, 'createCommit') + .resolves('commitsha1') .onSecondCall() - .resolves({ - data: { - sha: 'createdsha2', - url: 'unused', - truncated: false, - tree: [ - { - path: 'path1', - type: 'blob', - mode: '100755', - }, - ], - }, - headers: {}, - status: 201, + .resolves('commitsha2'); + const createTreeStub = sandbox.stub(octokit.git, 'createTree').resolves({ + data: { + sha: 'createdsha1', url: 'unused', - }); - const headTreeSha = await handler.createTree(octokit, origin, '', tree, 6); - assert.equal(headTreeSha, 'createdsha2'); + truncated: false, + tree: [ + { + path: 'path1', + type: 'blob', + mode: '100755', + }, + ], + }, + headers: {}, + status: 201, + url: 'unused', + }); + const updateRefStub = sandbox.stub(octokit.git, 'updateRef').resolves(); + + await handler.commitAndPush( + octokit, + oldHeadSha, + changes, + {branch: branchName, ...origin}, + message, + true, + 6 + ); + sinon.assert.calledTwice(createTreeStub); + sinon.assert.calledTwice(createCommitStub); + sinon.assert.calledOnce(updateRefStub); }); }); diff --git a/test/main-pr-option-defaults.ts b/test/main-pr-option-defaults.ts index b88aff02..bc537c4f 100644 --- a/test/main-pr-option-defaults.ts +++ b/test/main-pr-option-defaults.ts @@ -48,6 +48,7 @@ describe('addPullRequestDefaults', () => { message: 'chore: custom description', primary: 'main', maintainersCanModify: true, + filesPerCommit: undefined, }); const upstreamAndPrimary: CreatePullRequestUserOptions = { upstreamOwner: 'owner', @@ -68,6 +69,7 @@ describe('addPullRequestDefaults', () => { message: 'chore: custom description', primary: 'non-default-primary-branch', maintainersCanModify: true, + filesPerCommit: undefined, }); const upstreamAndPrDescription: CreatePullRequestUserOptions = { upstreamOwner: 'owner', @@ -87,6 +89,7 @@ describe('addPullRequestDefaults', () => { message: 'chore: custom code suggestions message', primary: 'main', maintainersCanModify: true, + filesPerCommit: undefined, }); }); it("Uses all of user's provided options", () => { @@ -100,6 +103,7 @@ describe('addPullRequestDefaults', () => { message: 'chore: code suggestions custom commit message', primary: 'non-default-primary-branch', maintainersCanModify: false, + filesPerCommit: 10, }; const gitHubPr = addPullRequestDefaults(options); assert.deepStrictEqual(gitHubPr, options); From 2b9299498dc008b33ed8c9e8cd8640020810785b Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 24 Aug 2022 12:36:49 -0700 Subject: [PATCH 3/3] chore: remove debug logging --- src/bin/workflow.ts | 1 - src/index.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/src/bin/workflow.ts b/src/bin/workflow.ts index d95ee799..97b7064e 100644 --- a/src/bin/workflow.ts +++ b/src/bin/workflow.ts @@ -62,7 +62,6 @@ async function createCommand() { const options = coerceUserCreatePullRequestOptions(); const changes = await git.getChanges(yargs.argv['git-dir'] as string); const octokit = new Octokit({auth: process.env.ACCESS_TOKEN}); - console.log(options); await createPullRequest(octokit, changes, options); } diff --git a/src/index.ts b/src/index.ts index 262f5565..bd4ad371 100644 --- a/src/index.ts +++ b/src/index.ts @@ -124,7 +124,6 @@ async function createPullRequest( return 0; } const gitHubConfigs = addPullRequestDefaults(options); - console.log(gitHubConfigs); logger.info('Starting GitHub PR workflow...'); const upstream: RepoDomain = { owner: gitHubConfigs.upstreamOwner,