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.

Sign up for GitHub

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

Properly extract commit sha from PullRequests #186

Closed
wants to merge 7 commits into from
Closed

Properly extract commit sha from PullRequests #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9898643
chore: fail the action upon error
nulltoken May 1, 2020
995ee8d
chore: enhance logging on error
nulltoken May 1, 2020
cb92023
chore: log event that triggered the action
nulltoken May 1, 2020
bcc3ea9
fix: use dedicated sha extractions per kind of event
nulltoken May 1, 2020
7c10d15
chore: run analysis before creating the check run
nulltoken May 1, 2020
0575095
chore: produce a textual output of the linting
nulltoken May 1, 2020
a9669f3
chore: do not fail the build on insufficient permissions
nulltoken May 1, 2020
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
149 changes: 70 additions & 79 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import { promises as fs } from 'fs';
import { array } from 'fp-ts/lib/Array';
import { flatten } from 'lodash';
import { Config } from './config';
import { runSpectral, createSpectral, fileWithContent } from './spectral';
import { runSpectral, createSpectral, fileWithContent, logTextualReport } from './spectral';
import { pluralizer } from './utils';
import { createGithubCheck, createOctokitInstance, getRepositoryInfoFromEvent, updateGithubCheck } from './octokit';
import glob from 'fast-glob';
import { error, info, setFailed } from '@actions/core';
import { info, setFailed } from '@actions/core';
import * as IOEither from 'fp-ts/lib/IOEither';
import * as IO from 'fp-ts/lib/IO';
import * as TE from 'fp-ts/lib/TaskEither';
Expand All @@ -29,51 +29,35 @@ const createSpectralAnnotations = (ruleset: string, parsed: fileWithContent[], b
pipe(
createSpectral(ruleset),
TE.chain(spectral => {
const spectralRuns = parsed.map(v =>
pipe(
runSpectral(spectral, v),
TE.map(results => {
info(`Done linting '${v.path}'`);

if (results.length === 0) {
info(' No issue detected');
} else {
info(` /!\\ ${pluralizer(results.length, 'issue')} detected`);
}

return { path: v.path, results };
})
)
);
const spectralRuns = parsed.map(v => pipe(runSpectral(spectral, v)));
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're running only a single operation, you do not need to pipe

Suggested change
const spectralRuns = parsed.map(v => pipe(runSpectral(spectral, v)));
const spectralRuns = parsed.map(v => runSpectral(spectral, v));

return array.sequence(TE.taskEither)(spectralRuns);
}),
TE.map(results =>
flatten(
results.map(validationResult => {
return validationResult.results.map<ChecksUpdateParamsOutputAnnotations>(vl => {
const annotation_level: ChecksUpdateParamsOutputAnnotations['annotation_level'] =
vl.severity === DiagnosticSeverity.Error
? 'failure'
: vl.severity === DiagnosticSeverity.Warning
? 'warning'
: 'notice';

const sameLine = vl.range.start.line === vl.range.end.line;

return {
annotation_level,
message: vl.message,
title: vl.code as string,
start_line: 1 + vl.range.start.line,
end_line: 1 + vl.range.end.line,
start_column: sameLine ? vl.range.start.character : undefined,
end_column: sameLine ? vl.range.end.character : undefined,
path: path.relative(basePath, validationResult.path),
};
});
TE.map(results => {
const flattened = flatten(results);
logTextualReport(flattened);

return flattened
.map<ChecksUpdateParamsOutputAnnotations>(vl => {
const annotation_level: ChecksUpdateParamsOutputAnnotations['annotation_level'] =
vl.severity === DiagnosticSeverity.Error
? 'failure'
: vl.severity === DiagnosticSeverity.Warning
? 'warning'
: 'notice';
const sameLine = vl.range.start.line === vl.range.end.line;
return {
annotation_level,
message: vl.message,
title: vl.code as string,
start_line: 1 + vl.range.start.line,
end_line: 1 + vl.range.end.line,
start_column: sameLine ? vl.range.start.character : undefined,
end_column: sameLine ? vl.range.end.character : undefined,
path: path.relative(basePath, vl.source!),
};
})
).sort((a, b) => (a.start_line > b.start_line ? 1 : -1))
)
.sort((a, b) => (a.start_line > b.start_line ? 1 : -1));
})
);

const readFilesToAnalyze = (pattern: string, workingDir: string) => {
Expand Down Expand Up @@ -130,51 +114,58 @@ const program = pipe(
INPUT_SPECTRAL_RULESET,
}) =>
pipe(
getRepositoryInfoFromEvent(GITHUB_EVENT_PATH, INPUT_EVENT_NAME),
TE.chain(event =>
pipe(
getRepositoryInfoFromEvent(GITHUB_EVENT_PATH, INPUT_EVENT_NAME),
TE.map(event => ({ event }))
),
TE.chain(({ event }) =>
pipe(
readFilesToAnalyze(INPUT_FILE_GLOB, GITHUB_WORKSPACE),
TE.chain(fileContents => createSpectralAnnotations(INPUT_SPECTRAL_RULESET, fileContents, GITHUB_WORKSPACE)),
TE.map(annotations => ({ event, annotations }))
)
),
TE.chain(({ event, annotations }) =>
Comment on lines +117 to +128
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I'd say this is going in the wrong direction. I'll probably have to check this out.

pipe(
createOctokitInstance(INPUT_REPO_TOKEN),
TE.map(octokit => ({ octokit, event }))
TE.map(octokit => ({ octokit, event, annotations }))
)
),
TE.chain(({ octokit, event }) =>
TE.chain(({ octokit, event, annotations }) =>
pipe(
createGithubCheck(octokit, event, `${CHECK_NAME} (${event.eventName})`),
TE.map(check => ({ octokit, event, check }))
TE.map(check => ({ octokit, event, check, annotations }))
)
),
TE.chain(({ octokit, event, check }) =>
TE.chain(({ octokit, event, check, annotations }) =>
pipe(
readFilesToAnalyze(INPUT_FILE_GLOB, GITHUB_WORKSPACE),
TE.chain(fileContents => createSpectralAnnotations(INPUT_SPECTRAL_RULESET, fileContents, GITHUB_WORKSPACE)),
TE.chain(annotations =>
pipe(
updateGithubCheck(
octokit,
check,
event,
annotations,
annotations.findIndex(f => f.annotation_level === 'failure') === -1 ? 'success' : 'failure'
),
TE.map(checkResponse => {
info(
`Check run '${checkResponse.data.name}' concluded with '${checkResponse.data.conclusion}' (${checkResponse.data.html_url})`
);
info(
`Commit ${event.sha} has been annotated (https://github.com/${event.owner}/${event.repo}/commit/${event.sha})`
);

const fatalErrors = annotations.filter(a => a.annotation_level === 'failure');
if (fatalErrors.length > 0) {
setFailed(`${pluralizer(fatalErrors.length, 'fatal issue')} detected. Failing the process.`);
}

return checkResponse;
})
)
updateGithubCheck(
octokit,
check,
event,
annotations,
annotations.findIndex(f => f.annotation_level === 'failure') === -1 ? 'success' : 'failure'
),
TE.map(checkResponse => {
if (checkResponse !== undefined) {
info(
`Check run '${checkResponse.data.name}' concluded with '${checkResponse.data.conclusion}' (${checkResponse.data.html_url})`
);
info(
`Commit ${event.sha} has been annotated (https://github.com/${event.owner}/${event.repo}/commit/${event.sha})`
);
}

const fatalErrors = annotations.filter(a => a.annotation_level === 'failure');
if (fatalErrors.length > 0) {
setFailed(`${pluralizer(fatalErrors.length, 'fatal issue')} detected. Failing the process.`);
}

return checkResponse;
}),

TE.orElse(e => {
setFailed(e.message);
setFailed(`${e.message}\n${e.stack}`);
return updateGithubCheck(octokit, check, event, [], 'failure', e.message);
})
)
Expand All @@ -187,7 +178,7 @@ program().then(result =>
pipe(
result,
E.fold(
e => error(e.message),
e => setFailed(`${e.message}\n${e.stack}`),
() => info('Analysis is complete')
)
)
Expand Down
135 changes: 92 additions & 43 deletions src/octokit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
import { GitHub } from '@actions/github';
import { info, warning } from '@actions/core';
import * as TE from 'fp-ts/lib/TaskEither';
import * as E from 'fp-ts/lib/Either';
import { pipe } from 'fp-ts/lib/pipeable';
import { ChecksCreateResponse, ChecksUpdateParamsOutputAnnotations, ChecksUpdateParams, Response } from '@octokit/rest';
import {
ChecksCreateResponse,
ChecksUpdateParamsOutputAnnotations,
ChecksUpdateParams,
Response,
ChecksUpdateResponse,
} from '@octokit/rest';

type Event = {
after: string;
Expand All @@ -16,18 +23,33 @@ type Event = {

export const createOctokitInstance = (token: string) => TE.fromEither(E.tryCatch(() => new GitHub(token), E.toError));

const tryCreateGithubCheck = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XVincentX Could you please help me rewrite this in a more fp-ts way? I haven't found the proper way to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

You really can't, since the underlying API has side-effects. You can wrap it (as you're doing) with tryCatch later

octokit: GitHub,
event: IRepositoryInfo,
name: string
): Promise<Response<ChecksCreateResponse> | undefined> => {
try {
return await octokit.checks.create({
owner: event.owner,
repo: event.repo,
name,
head_sha: event.sha,
status: 'in_progress',
});
} catch (e) {
if (e.message === 'Resource not accessible by integration') {
// not enough permissions to create annotations

warning('Skipping creation of checkrun because insufficient rights.');
return undefined;
}

throw e;
}
};

export const createGithubCheck = (octokit: GitHub, event: IRepositoryInfo, name: string) =>
TE.tryCatch(
() =>
octokit.checks.create({
owner: event.owner,
repo: event.repo,
name,
head_sha: event.sha,
status: 'in_progress',
}),
E.toError
);
TE.tryCatch(() => tryCreateGithubCheck(octokit, event, name), E.toError);

export interface IRepositoryInfo {
owner: string;
Expand All @@ -36,54 +58,81 @@ export interface IRepositoryInfo {
sha: string;
}

const extractSha = (eventName: string, event: any): string => {
switch (eventName) {
case 'pull_request':
return event.pull_request.head.sha;
case 'push':
return event.after;
default:
warning(event);
throw new Error(`Unsupported event '${eventName}'`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the idea of fp-ts is not to throw exception, so this is not ok. This should return an Either and a Left instance when something goes wrong.

}
};

export const getRepositoryInfoFromEvent = (
eventPath: string,
eventName: string
): TE.TaskEither<Error, IRepositoryInfo> =>
pipe(
TE.fromEither(E.tryCatch<Error, Event>(() => require(eventPath), E.toError)),
TE.map(event => {
const { repository, after } = event;
info(`Responding to event '${eventName}'`);
const sha = extractSha(eventName, event);
info(`Processing commit '${sha}'`);

const { repository } = event;
const {
owner: { login: owner },
} = repository;
const { name: repo } = repository;
return { owner, repo, eventName, sha: after };

return { owner, repo, eventName, sha };
})
);

export const updateGithubCheck = (
export const tryUpdateGithubCheck = async (
octokit: GitHub,
check: Response<ChecksCreateResponse>,
check: Response<ChecksCreateResponse> | undefined,
event: IRepositoryInfo,
annotations: ChecksUpdateParamsOutputAnnotations[],
conclusion: ChecksUpdateParams['conclusion'],
message?: string
) =>
TE.tryCatch(
() =>
octokit.checks.update({
check_run_id: check.data.id,
owner: event.owner,
name: check.data.name,
repo: event.repo,
status: 'completed',
conclusion,
completed_at: new Date().toISOString(),
output: {
title: check.data.name,
summary: message
? message
: conclusion === 'success'
? 'Lint completed successfully'
: 'Lint completed with some errors',
): Promise<Response<ChecksUpdateResponse> | undefined> => {
if (check === undefined) {
return undefined;
}

// TODO: Split calls when annotations.length > 50
// From https://octokit.github.io/rest.js/v17#checks-update
// => "The Checks API limits the number of annotations to a maximum of 50 per API request.
// To create more than 50 annotations, you have to make multiple requests to the Update a check run endpoint."
annotations,
},
}),
E.toError
);
return octokit.checks.update({
check_run_id: check.data.id,
owner: event.owner,
name: check.data.name,
repo: event.repo,
status: 'completed',
conclusion,
completed_at: new Date().toISOString(),
output: {
title: check.data.name,
summary: message
? message
: conclusion === 'success'
? 'Lint completed successfully'
: 'Lint completed with some errors',

// TODO: Split calls when annotations.length > 50
// From https://octokit.github.io/rest.js/v17#checks-update
// => "The Checks API limits the number of annotations to a maximum of 50 per API request.
// To create more than 50 annotations, you have to make multiple requests to the Update a check run endpoint."
annotations,
},
});
};

export const updateGithubCheck = (
octokit: GitHub,
check: Response<ChecksCreateResponse> | undefined,
event: IRepositoryInfo,
annotations: ChecksUpdateParamsOutputAnnotations[],
conclusion: ChecksUpdateParams['conclusion'],
message?: string
) => TE.tryCatch(() => tryUpdateGithubCheck(octokit, check, event, annotations, conclusion, message), E.toError);
Loading