Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: group commit files into batches #398

Merged
merged 3 commits into from
Aug 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the use of the generator.

nit: since it's only used in one place, I wonder if it's a premature abstraction, might just have gone with function *commitBatch.

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(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved to a separate module for ease of mocking.

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