Skip to content

Commit

Permalink
feat: group commit files into batches (#398)
Browse files Browse the repository at this point in the history
* feat: group commit files into batches

* test: split on commits, not trees

* chore: remove debug logging
  • Loading branch information
chingor13 committed Aug 24, 2022
1 parent 85dc6a0 commit e3815d5
Show file tree
Hide file tree
Showing 10 changed files with 163 additions and 51 deletions.
5 changes: 5 additions & 0 deletions src/bin/code-suggester.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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': {
Expand Down
1 change: 1 addition & 0 deletions src/bin/workflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}

Expand Down
1 change: 1 addition & 0 deletions src/default-options-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export function addPullRequestDefaults(
? options.primary
: DEFAULT_PRIMARY_BRANCH,
maintainersCanModify: options.maintainersCanModify === false ? false : true,
filesPerCommit: options.filesPerCommit,
};
return pullRequestSettings;
}
Expand Down
71 changes: 30 additions & 41 deletions src/github/commit-and-push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ import {
} from '../types';
import {Octokit} from '@octokit/rest';
import {logger} from '../logger';
import {createCommit} from './create-commit';

const DEFAULT_FILES_PER_COMMIT = 100;

/**
* Generate and return a GitHub tree object structure
Expand Down Expand Up @@ -53,6 +56,15 @@ export function generateTreeObjects(changes: Changes): TreeObject[] {
return tree;
}

function* inGroupsOf<T>(
all: T[],
groupSize: number
): Generator<T[], void, void> {
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.
Expand Down Expand Up @@ -91,37 +103,6 @@ export async function createTree(
return treeSha;
}

/**
* 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<string>} the new commit SHA
*/
export async function createCommit(
octokit: Octokit,
origin: RepoDomain,
refHead: string,
treeSha: string,
message: string
): Promise<string> {
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;
}

/**
* Update a reference to a SHA
* Rejects if GitHub V3 API fails with the GitHub error response
Expand Down Expand Up @@ -166,19 +147,27 @@ 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 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
Expand Down
48 changes: 48 additions & 0 deletions src/github/create-commit.ts
Original file line number Diff line number Diff line change
@@ -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<string>} the new commit SHA
*/
export async function createCommit(
octokit: Octokit,
origin: RepoDomain,
refHead: string,
treeSha: string,
message: string
): Promise<string> {
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;
}
3 changes: 2 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,8 @@ async function createPullRequest(
changes,
originBranch,
gitHubConfigs.message,
gitHubConfigs.force
gitHubConfigs.force,
gitHubConfigs.filesPerCommit
);

const description: Description = {
Expand Down
4 changes: 4 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ export interface CreatePullRequestUserOptions {
draft?: boolean;
// Optional logger to set
logger?: Logger;
// Optional number of files per commit
filesPerCommit?: number;
}

/**
Expand All @@ -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;
}

/**
Expand Down
1 change: 1 addition & 0 deletions test/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
76 changes: 67 additions & 9 deletions test/commit-and-push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -285,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')
Expand Down Expand Up @@ -313,7 +313,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, {
Expand Down Expand Up @@ -342,6 +349,57 @@ 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 () => {
for (let i = 0; i < 10; i++) {
changes.set(`path${i}`, {
mode: '100755',
content: 'some file content',
});
}
sandbox.stub(octokit.git, 'getCommit').resolves(getCommitResponse);
const createCommitStub = sandbox
.stub(createCommitModule, 'createCommit')
.resolves('commitsha1')
.onSecondCall()
.resolves('commitsha2');
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 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);
});
});
4 changes: 4 additions & 0 deletions test/main-pr-option-defaults.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ describe('addPullRequestDefaults', () => {
message: 'chore: custom description',
primary: 'main',
maintainersCanModify: true,
filesPerCommit: undefined,
});
const upstreamAndPrimary: CreatePullRequestUserOptions = {
upstreamOwner: 'owner',
Expand All @@ -68,6 +69,7 @@ describe('addPullRequestDefaults', () => {
message: 'chore: custom description',
primary: 'non-default-primary-branch',
maintainersCanModify: true,
filesPerCommit: undefined,
});
const upstreamAndPrDescription: CreatePullRequestUserOptions = {
upstreamOwner: 'owner',
Expand All @@ -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", () => {
Expand All @@ -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);
Expand Down

0 comments on commit e3815d5

Please sign in to comment.