Skip to content

Commit

Permalink
Remove references to payload as much as possible
Browse files Browse the repository at this point in the history
  • Loading branch information
@ktrz @NathanielRN
ktrz authored and NathanielRN committed Sep 23, 2021
1 parent bd200b2 commit 0f3dd79
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 109 deletions.
74 changes: 69 additions & 5 deletions src/extract.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,16 +145,28 @@ function getHumanReadableUnitValue(seconds: number): [number, string] {
}
}

function getCommitFromPr(pr: PullRequest): Commit {
function getCommitFromWebhookPayload(): Commit {
/* eslint-disable @typescript-eslint/camelcase */
if (github.context.payload.head_commit) {
return github.context.payload.head_commit;
}

const pr = github.context.payload.pull_request;
if (!pr) {
throw new Error(
`No commit information is found in payload: ${JSON.stringify(github.context.payload, null, 2)}.`,
);
}

// On pull_request hook, head_commit is not available
const message: string = pr.title;
const id: string = pr.head.sha;
const timestamp: string = pr.head.repo.updated_at;
const url = `${pr.html_url}/commits/${id}`;
const name: string = pr.head.user.login;
const username: string = pr.head.user.login;
const user = {
name,
username: name, // XXX: Fallback, not correct
name: username, // XXX: Fallback, not correct
username,
};

return {
Expand Down Expand Up @@ -222,6 +234,44 @@ async function getCommit(githubToken?: string): Promise<Commit> {
return await getHeadCommit(githubToken);
}

async function getCommitFromGitHubREST(githubToken: string): Promise<Commit> {
// On `schedule:` and `workflow_dispatch:` events, the `.head_commit` and
// `.pull_request` objects are not available on the `github.context.payload`
// object.
// Therefore, we try to get the commit data of the current HEAD via GitHub
// REST
const octocat = new github.GitHub(githubToken);

const { status, data } = await octocat.repos.getCommit({
owner: github.context.repo.owner,
repo: github.context.repo.repo,
ref: github.context.ref,
});

if (!(status === 200 || status === 304)) {
throw new Error(`Could not fetch the head commit. Received code: ${status}`);
}

const { commit } = data;

return {
author: {
name: commit.author.name,
username: data.author.login,
email: commit.author.email,
},
committer: {
name: commit.committer.name,
username: data.committer.login,
email: commit.committer.email,
},
id: data.sha,
message: commit.message,
timestamp: commit.author.date,
url: data.html_url,
};
}

function extractCargoResult(output: string): BenchmarkResult[] {
const lines = output.split(/\r?\n/g);
const ret = [];
Expand Down Expand Up @@ -496,7 +546,21 @@ export async function extractResult(config: Config): Promise<Benchmark> {
throw new Error(`No benchmark result was found in ${config.outputFilePath}. Benchmark output was '${output}'`);
}

const commit = await getCommit(githubToken);
let commit;

try {
commit = getCommitFromWebhookPayload();
} catch (err) {
if (!githubToken) {
if (err instanceof Error) {
throw new Error(`${err.message} No github-token provided, could not fallback to GitHub API Request.`);
} else {
throw err;
}
}

commit = await getCommitFromGitHubREST(githubToken);
}

return {
commit,
Expand Down
10 changes: 1 addition & 9 deletions src/git.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,7 @@ export async function cmd(...args: string[]): Promise<string> {
}

function getRemoteUrl(token: string): string {
/* eslint-disable @typescript-eslint/camelcase */
const fullName = github.context.payload.repository?.full_name;
/* eslint-enable @typescript-eslint/camelcase */

if (!fullName) {
throw new Error(`Repository info is not available in payload: ${JSON.stringify(github.context.payload)}`);
}

return `https://x-access-token:${token}@github.com/${fullName}.git`;
return `https://x-access-token:${token}@github.com/${github.context.repo.owner}/${github.context.repo.repo}.git`;
}

export async function push(token: string, branch: string, ...options: string[]): Promise<string> {
Expand Down
33 changes: 18 additions & 15 deletions src/write.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,15 @@ function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number
return alerts;
}

function getCurrentRepo() {
const repo = github.context.payload.repository;
if (!repo) {
throw new Error(
`Repository information is not available in payload: ${JSON.stringify(github.context.payload, null, 2)}`,
);
}
return repo;
function getCurrentRepoMetadata() {
return {
name: github.context.repo.repo,
owner: {
login: github.context.repo.owner,
},
// eslint-disable-next-line @typescript-eslint/camelcase
html_url: `https://github.com/${github.context.repo.owner}/${github.context.repo.repo}`,
};
}

function floatStr(n: number) {
Expand All @@ -137,9 +138,9 @@ function strVal(b: BenchmarkResult): string {
}

function commentFooter(): string {
const repo = getCurrentRepo();
const repoMetadata = getCurrentRepoMetadata();
// eslint-disable-next-line @typescript-eslint/camelcase
const repoUrl = repo.html_url ?? '';
const repoUrl = repoMetadata.html_url ?? '';
const actionUrl = repoUrl + '/actions?query=workflow%3A' + encodeURIComponent(github.context.workflow);

return `This comment was automatically generated by [workflow](${actionUrl}) using [github-action-benchmark](https://github.com/marketplace/actions/continuous-benchmark).`;
Expand Down Expand Up @@ -219,13 +220,13 @@ function buildAlertComment(
async function leaveComment(commitId: string, body: string, token: string) {
core.debug('Sending comment:\n' + body);

const repo = getCurrentRepo();
const repoMetadata = getCurrentRepoMetadata();
// eslint-disable-next-line @typescript-eslint/camelcase
const repoUrl = repo.html_url ?? '';
const repoUrl = repoMetadata.html_url ?? '';
const client = new github.GitHub(token);
const res = await client.repos.createCommitComment({
owner: repo.owner.login,
repo: repo.name,
owner: repoMetadata.owner.login,
repo: repoMetadata.name,
// eslint-disable-next-line @typescript-eslint/camelcase
commit_sha: commitId,
body,
Expand Down Expand Up @@ -313,7 +314,8 @@ function addBenchmarkToDataJson(
maxItems: number | null,
): Benchmark | null {
// eslint-disable-next-line @typescript-eslint/camelcase
const htmlUrl = github.context.payload.repository?.html_url ?? '';
const repoMetadata = getCurrentRepoMetadata();
const htmlUrl = repoMetadata.html_url ?? '';

let prevBench: Benchmark | null = null;
data.lastUpdate = Date.now();
Expand Down Expand Up @@ -369,6 +371,7 @@ async function writeBenchmarkToGitHubPagesWithRetry(
maxItemsInChart,
} = config;
const dataPath = path.join(benchmarkDataDirPath, 'data.js');
// FIXME: This payload is not available on `schedule:` or `workflow_dispatch:` events.
const isPrivateRepo = github.context.payload.repository?.private ?? false;

if (!skipFetchGhPages && (!isPrivateRepo || githubToken)) {
Expand Down
21 changes: 16 additions & 5 deletions test/extract.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,14 +342,23 @@ describe('extractResult()', function() {
it('collects the commit information from current head via REST API as fallback when githubToken is provided', async function() {
dummyGitHubContext.payload = {};
dummyCommitData = {
author: {
login: 'testAuthorLogin',
},
committer: {
login: 'testCommitterLogin',
},
commit: {
author: {
name: 'test author',
date: 'repo updated at timestamp',
date: 'author updated at timestamp',
email: '[email protected]',
},
committer: {
name: 'test committer',
date: 'repo updated at timestamp',
// We use the `author.date` instead.
// date: 'committer updated at timestamp',
email: '[email protected]',
},
message: 'test message',
},
Expand All @@ -368,15 +377,17 @@ describe('extractResult()', function() {
const expectedCommit = {
id: 'abcd1234',
message: 'test message',
timestamp: 'repo updated at timestamp',
timestamp: 'author updated at timestamp',
url: 'https://github.com/dymmy/repo/commit/abcd1234',
author: {
name: 'test author',
username: 'test author',
username: 'testAuthorLogin',
email: '[email protected]',
},
committer: {
name: 'test committer',
username: 'test committer',
username: 'testCommitterLogin',
email: '[email protected]',
},
};
A.deepEqual(commit, expectedCommit);
Expand Down
40 changes: 6 additions & 34 deletions test/git.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,14 @@ class FakedExec {

const fakedExec = new FakedExec();
const gitHubContext = {
payload: {
repository: {
full_name: 'user/repo',
},
repo: {
repo: 'repo',
owner: 'user',
},
} as {
payload: {
repository: {
full_name: string;
} | null;
repo: {
repo: string;
owner: string;
};
};

Expand Down Expand Up @@ -126,10 +124,6 @@ describe('git', function() {
});

describe('push()', function() {
afterEach(function() {
gitHubContext.payload.repository = { full_name: 'user/repo' };
});

it('runs `git push` with given branch and options', async function() {
const stdout = await push('this-is-token', 'my-branch', 'opt1', 'opt2');
const args = fakedExec.lastArgs;
Expand All @@ -149,22 +143,9 @@ describe('git', function() {
]),
);
});

it('raises an error when repository info is not included in payload', async function() {
gitHubContext.payload.repository = null;
await A.rejects(
() => push('this-is-token', 'my-branch', 'opt1', 'opt2'),
/^Error: Repository info is not available in payload/,
);
eq(fakedExec.lastArgs, null);
});
});

describe('pull()', function() {
afterEach(function() {
gitHubContext.payload.repository = { full_name: 'user/repo' };
});

it('runs `git pull` with given branch and options with token', async function() {
const stdout = await pull('this-is-token', 'my-branch', 'opt1', 'opt2');
const args = fakedExec.lastArgs;
Expand Down Expand Up @@ -193,14 +174,5 @@ describe('git', function() {
eq(args[0], 'git');
eq(args[1], userArgs.concat(['pull', 'origin', 'my-branch', 'opt1', 'opt2']));
});

it('raises an error when repository info is not included in payload', async function() {
gitHubContext.payload.repository = null;
await A.rejects(
() => pull('this-is-token', 'my-branch', 'opt1', 'opt2'),
/^Error: Repository info is not available in payload/,
);
eq(fakedExec.lastArgs, null);
});
});
});
48 changes: 7 additions & 41 deletions test/write.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,27 +76,19 @@ class GitSpy {
}
const gitSpy = new GitSpy();

interface RepositoryPayload {
owner: {
login: string;
};
name: string;
full_name: string;
html_url: string;
interface RepositoryPayloadSubset {
private: boolean;
}

const gitHubContext = {
repo: {
repo: 'repo',
owner: 'user',
},
payload: {
repository: {
owner: {
login: 'user',
},
name: 'repo',
full_name: 'user/repo',
html_url: 'https://github.com/user/repo',
private: false,
} as RepositoryPayload | null,
} as RepositoryPayloadSubset | null,
},
workflow: 'Workflow name',
};
Expand Down Expand Up @@ -219,7 +211,7 @@ describe('writeBenchmark()', function() {
added: Benchmark;
error?: string[];
commitComment?: string;
repoPayload?: null | RepositoryPayload;
repoPayload?: null | RepositoryPayloadSubset;
}> = [
{
it: 'appends new result to existing data',
Expand Down Expand Up @@ -567,32 +559,6 @@ describe('writeBenchmark()', function() {
error: ["'comment-on-alert' input is set but 'github-token' input is not set"],
commitComment: undefined,
},
{
it: 'throws an error when repository payload cannot be obtained from context',
config: defaultCfg,
data: {
lastUpdate,
repoUrl: '', // When repository is null repoUrl will be empty
entries: {
'Test benchmark': [
{
commit: commit('prev commit id'),
date: lastUpdate - 1000,
tool: 'cargo',
benches: [bench('bench_fib_10', 100)],
},
],
},
},
added: {
commit: commit('current commit id'),
date: lastUpdate,
tool: 'cargo',
benches: [bench('bench_fib_10', 210)], // Exceeds 2.0 threshold
},
repoPayload: null,
error: ['Repository information is not available in payload: {', ' "repository": null', '}'],
},
{
it: 'truncates data items if it exceeds max-items-in-chart',
config: { ...defaultCfg, maxItemsInChart: 1 },
Expand Down

0 comments on commit 0f3dd79

Please sign in to comment.