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: check CI failures before land #422

Merged
merged 1 commit into from
May 26, 2020
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
7 changes: 4 additions & 3 deletions components/metadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ const fs = require('fs');

module.exports = async function getMetadata(argv, cli) {
const credentials = await auth({
github: true
github: true,
jenkins: true
});
const request = new Request(credentials);

Expand All @@ -38,8 +39,8 @@ module.exports = async function getMetadata(argv, cli) {
cli.write(metadata);
cli.separator();

const checker = new PRChecker(cli, data, argv);
const status = checker.checkAll(argv.checkComments);
const checker = new PRChecker(cli, data, request, argv);
const status = await checker.checkAll(argv.checkComments);
return {
status,
request,
Expand Down
6 changes: 6 additions & 0 deletions lib/ci/ci_type_parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ const CI_TYPE_ENUM = {
JOB_CI: 1 << 2
};

const CI_PROVIDERS = {
JENKINS: 'jenkins',
GITHUB: 'github-check'
};

const { JOB_CI, FULL_CI, LITE_CI } = CI_TYPE_ENUM;

const CI_TYPES = new Map([
Expand Down Expand Up @@ -209,6 +214,7 @@ module.exports = {
CITGM, PR, COMMIT, BENCHMARK, LIBUV, V8, NOINTL,
LINTER, LITE_PR, LITE_COMMIT
},
CI_PROVIDERS,
isFullCI,
isLiteCI,
JobParser,
Expand Down
64 changes: 44 additions & 20 deletions lib/pr_checker.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ const {
const {
JobParser,
CI_TYPES,
CI_PROVIDERS,
isLiteCI,
isFullCI
} = require('./ci/ci_type_parser');
const { PRBuild } = require('./ci/ci_result_parser');

const GIT_CONFIG_GUIDE_URL = 'https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork';

Expand All @@ -31,8 +33,9 @@ class PRChecker {
* @param {{}} cli
* @param {PRData} data
*/
constructor(cli, data, argv) {
constructor(cli, data, request, argv) {
this.cli = cli;
this.request = request;
this.data = data;
const {
pr, reviewers, comments, reviews, commits, collaborators
Expand Down Expand Up @@ -66,10 +69,10 @@ class PRChecker {
return this.argv.waitTimeMultiApproval;
}

checkAll(checkComments = false) {
async checkAll(checkComments = false) {
const status = [
this.checkCommitsAfterReview(),
this.checkCI(),
await this.checkCI(),
this.checkReviewsAndWait(new Date(), checkComments),
this.checkMergeableState(),
this.checkPRState(),
Expand Down Expand Up @@ -201,32 +204,42 @@ class PRChecker {
return cis.find(ci => isFullCI(ci) || isLiteCI(ci));
}

checkCI() {
const ciType = this.argv.ciType || 'jenkins';
if (ciType === 'jenkins') {
return this.checkJenkinsCI();
} else if (ciType === 'github-check') {
return this.checkGitHubCI();
async checkCI() {
const ciType = this.argv.ciType || CI_PROVIDERS.JENKINS;
const providers = Object.values(CI_PROVIDERS);

if (!providers.includes(ciType)) {
this.cli.error(
`Invalid ciType ${ciType} - must be one of ${providers.join(', ')}`);
return false;
}
this.cli.error(`Invalid ciType: ${ciType}`);
return false;

let status = false;
if (ciType === CI_PROVIDERS.JENKINS) {
status = await this.checkJenkinsCI();
} else if (ciType === CI_PROVIDERS.GITHUB) {
status = this.checkGitHubCI();
}

return status;
}

// TODO: we might want to check CI status when it's less flaky...
// TODO: not all PR requires CI...labels?
checkJenkinsCI() {
const { cli, commits, argv } = this;
async checkJenkinsCI() {
const { cli, commits, request, argv } = this;
const { maxCommits } = argv;
const thread = this.data.getThread();
const ciMap = new JobParser(thread).parse();

let status = true;
if (!ciMap.size) {
cli.error('No CI runs detected');
this.CIStatus = false;
return false;
} else if (!this.hasFullOrLiteCI(ciMap)) {
status = false;
cli.error('No full CI runs or lite CI runs detected');
cli.error('No full or lite Jenkins CI runs detected');
}

let lastCI;
Expand All @@ -236,7 +249,8 @@ class PRChecker {
if (!lastCI || lastCI.date < ci.date) {
lastCI = {
typeName: name,
date: ci.date
date: ci.date,
jobId: ci.jobid
};
}
}
Expand Down Expand Up @@ -270,6 +284,16 @@ class PRChecker {
cli.warn(infoMsg);
}
}

// Check the last CI run for its results.
const build = new PRBuild(cli, request, lastCI.jobId);
const { result, failures } = await build.getResults();

if (result === 'FAILURE') {
cli.error(
`${failures.length} failure(s) on the last Jenkins CI run`);
status = false;
}
}

this.CIStatus = status;
Expand Down Expand Up @@ -298,12 +322,12 @@ class PRChecker {
// GitHub new Check API
for (const { status, conclusion } of checkSuites.nodes) {
if (status !== 'COMPLETED') {
cli.error('CI is still running');
cli.error('GitHub CI is still running');
return false;
}

if (!['SUCCESS', 'NEUTRAL'].includes(conclusion)) {
cli.error('Last CI failed');
cli.error('Last GitHub CI failed');
return false;
}
}
Expand All @@ -312,17 +336,17 @@ class PRChecker {
if (commit.status) {
const { state } = commit.status;
if (state === 'PENDING') {
cli.error('CI is still running');
cli.error('GitHub CI is still running');
return false;
}

if (!['SUCCESS', 'EXPECTED'].includes(state)) {
cli.error('Last CI failed');
cli.error('Last GitHub CI failed');
return false;
}
}

cli.info('Last CI run was successful');
cli.info('Last GitHub CI successful');
this.CIStatus = true;
return true;
}
Expand Down
Loading