Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 81cfc18

Browse files
committedOct 2, 2019
git-node: add GitHub status as a CI option
Some project in the org don't use Jenkins, which means PRChecker will never succeed for pull requests on those projects. These projects usually have Travis, AppVeyor or other CI systems in place, and those systems will publish the status to GitHub, which can be retrieved via API. This commit adds GitHub status as an optional way to validate if a PR satisfies the CI requirement. We need to check for the CI status in two fields returned by our GraphQL query: commit.status for services using the old GitHub integration, and commits.checkSuites for services using the new GitHub integration via GitHub Apps.
1 parent e60bc6a commit 81cfc18

19 files changed

+610
-3
lines changed
 

‎lib/pr_checker.js

+63-1
Original file line numberDiff line numberDiff line change
@@ -184,9 +184,20 @@ class PRChecker {
184184
return cis.find(ci => isFullCI(ci) || isLiteCI(ci));
185185
}
186186

187+
checkCI() {
188+
const ciType = this.argv.ciType || 'jenkins';
189+
if (ciType === 'jenkins') {
190+
return this.checkJenkinsCI();
191+
} else if (ciType === 'github-check') {
192+
return this.checkGitHubCI();
193+
}
194+
this.cli.error(`Invalid ciType: ${ciType}`);
195+
return false;
196+
}
197+
187198
// TODO: we might want to check CI status when it's less flaky...
188199
// TODO: not all PR requires CI...labels?
189-
checkCI() {
200+
checkJenkinsCI() {
190201
const { cli, commits, argv } = this;
191202
const { maxCommits } = argv;
192203
const thread = this.data.getThread();
@@ -248,6 +259,57 @@ class PRChecker {
248259
return status;
249260
}
250261

262+
checkGitHubCI() {
263+
const { cli, commits } = this;
264+
265+
if (!commits) {
266+
cli.error('No commits detected');
267+
return false;
268+
}
269+
270+
// NOTE(mmarchini): we only care about the last commit. Maybe in the future
271+
// we'll want to check all commits for a successful CI.
272+
const { commit } = commits[commits.length - 1];
273+
274+
this.CIStatus = false;
275+
const checkSuites = commit.checkSuites || { nodes: [] };
276+
if (!commit.status && checkSuites.nodes.length === 0) {
277+
cli.error('No CI runs detected');
278+
return false;
279+
}
280+
281+
// GitHub new Check API
282+
for (let { status, conclusion } of checkSuites.nodes) {
283+
if (status !== 'COMPLETED') {
284+
cli.error('CI is still running');
285+
return false;
286+
}
287+
288+
if (!['SUCCESS', 'NEUTRAL'].includes(conclusion)) {
289+
cli.error('Last CI failed');
290+
return false;
291+
}
292+
}
293+
294+
// GitHub old commit status API
295+
if (commit.status) {
296+
const { state } = commit.status;
297+
if (state === 'PENDING') {
298+
cli.error('CI is still running');
299+
return false;
300+
}
301+
302+
if (!['SUCCESS', 'EXPECTED'].includes(state)) {
303+
cli.error('Last CI failed');
304+
return false;
305+
}
306+
}
307+
308+
cli.info('Last CI run was successful');
309+
this.CIStatus = true;
310+
return true;
311+
}
312+
251313
checkAuthor() {
252314
const { cli, commits, pr } = this;
253315

‎lib/queries/PRCommits.gql

+9
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,15 @@ query Commits($prid: Int!, $owner: String!, $repo: String!, $after: String) {
2525
message
2626
messageHeadline
2727
authoredByCommitter
28+
checkSuites(first: 10) {
29+
nodes {
30+
conclusion,
31+
status
32+
}
33+
}
34+
status {
35+
state
36+
}
2837
}
2938
}
3039
}

‎lib/request.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,8 @@ class Request {
6868
method: 'POST',
6969
headers: {
7070
'Authorization': `Basic ${githubCredentials}`,
71-
'User-Agent': 'node-core-utils'
71+
'User-Agent': 'node-core-utils',
72+
'Accept': 'application/vnd.github.antiope-preview+json'
7273
},
7374
body: JSON.stringify({
7475
query: query,

‎lib/session.js

+5
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ class Session {
5050
upstream: this.upstream,
5151
branch: this.branch,
5252
readme: this.readme,
53+
ciType: this.ciType,
5354
prid: this.prid
5455
};
5556
}
@@ -78,6 +79,10 @@ class Session {
7879
return this.config.readme;
7980
}
8081

82+
get ciType() {
83+
return this.config.ciType || 'jenkins';
84+
}
85+
8186
get pullName() {
8287
return `${this.owner}/${this.repo}/pulls/${this.prid}`;
8388
}

‎test/fixtures/data.js

+14-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
'use strict';
22

3-
const { readJSON, patchPrototype, readFile } = require('./index');
3+
const { basename } = require('path');
4+
const { readdirSync } = require('fs');
5+
6+
const { readJSON, patchPrototype, readFile, path } = require('./index');
47
const { Collaborator } = require('../../lib/collaborators');
58
const { Review } = require('../../lib/reviews');
69

@@ -82,6 +85,15 @@ const readmeNoCollaborators = readFile('./README/README_no_collaborators.md');
8285
const readmeNoCollaboratorE = readFile('./README/README_no_collaboratorE.md');
8386
const readmeUnordered = readFile('./README/README_unordered.md');
8487

88+
const githubCI = {};
89+
90+
for (let item of readdirSync(path('./github-ci'))) {
91+
if (!item.endsWith('.json')) {
92+
continue;
93+
}
94+
githubCI[basename(item, '.json')] = readJSON(`./github-ci/${item}`);
95+
};
96+
8597
module.exports = {
8698
approved,
8799
requestedChanges,
@@ -94,6 +106,7 @@ module.exports = {
94106
commentsWithLiteCI,
95107
commentsWithLGTM,
96108
oddCommits,
109+
githubCI,
97110
incorrectGitConfigCommits,
98111
simpleCommits,
99112
singleCommitAfterReview,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"status": {
11+
"state": "FAILURE"
12+
},
13+
"checkSuites": {
14+
"nodes": [
15+
{
16+
"status": "COMPLETED",
17+
"conclusion": "FAILURE"
18+
}
19+
]
20+
}
21+
}
22+
}
23+
]
24+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"status": {
11+
"state": "SUCCESS"
12+
},
13+
"checkSuites": {
14+
"nodes": [
15+
{
16+
"status": "COMPLETED",
17+
"conclusion": "SUCCESS"
18+
}
19+
]
20+
}
21+
}
22+
}
23+
]
24+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"checkSuites": {
11+
"nodes": [
12+
{
13+
"status": "COMPLETED",
14+
"conclusion": "FAILURE"
15+
}
16+
]
17+
}
18+
}
19+
}
20+
]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"checkSuites": {
11+
"nodes": [
12+
{
13+
"status": "IN_PROGRESS"
14+
}
15+
]
16+
}
17+
}
18+
}
19+
]
20+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"checkSuites": {
11+
"nodes": [
12+
{
13+
"status": "COMPLETED",
14+
"conclusion": "SUCCESS"
15+
}
16+
]
17+
}
18+
}
19+
}
20+
]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"status": {
11+
"state": "FAILURE"
12+
}
13+
}
14+
}
15+
]
16+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"status": {
11+
"state": "PENDING"
12+
}
13+
}
14+
}
15+
]
16+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"status": {
11+
"state": "SUCCESS"
12+
}
13+
}
14+
}
15+
]
16+
+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
}
10+
}
11+
}
12+
]
13+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"status": {
11+
"state": "FAILURE"
12+
},
13+
"checkSuites": {
14+
"nodes": [
15+
{
16+
"status": "COMPLETED",
17+
"conclusion": "SUCCESS"
18+
}
19+
]
20+
}
21+
}
22+
}
23+
]
24+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-26T12:10:20Z",
5+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
6+
"messageHeadline": "doc: add api description README",
7+
"author": {
8+
"login": "foo"
9+
},
10+
"status": {
11+
"state": "SUCCESS"
12+
},
13+
"checkSuites": {
14+
"nodes": [
15+
{
16+
"status": "COMPLETED",
17+
"conclusion": "FAILURE"
18+
}
19+
]
20+
}
21+
}
22+
}
23+
]
24+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-25T11:27:02Z",
5+
"oid": "6c0945cbeea2cbbc97d13a3d9e3fe68bd145b985",
6+
"messageHeadline": "fixup: adjust spelling",
7+
"author": {
8+
"login": "bar"
9+
},
10+
"status": {
11+
"state": "SUCCESS"
12+
}
13+
}
14+
},{
15+
"commit": {
16+
"committedDate": "2017-10-26T12:10:20Z",
17+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
18+
"messageHeadline": "doc: add api description README",
19+
"author": {
20+
"login": "foo"
21+
}
22+
}
23+
}
24+
]
25+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
[
2+
{
3+
"commit": {
4+
"committedDate": "2017-10-25T11:27:02Z",
5+
"oid": "6c0945cbeea2cbbc97d13a3d9e3fe68bd145b985",
6+
"messageHeadline": "fixup: adjust spelling",
7+
"author": {
8+
"login": "bar"
9+
}
10+
}
11+
},{
12+
"commit": {
13+
"committedDate": "2017-10-26T12:10:20Z",
14+
"oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d",
15+
"messageHeadline": "doc: add api description README",
16+
"author": {
17+
"login": "foo"
18+
},
19+
"status": {
20+
"state": "SUCCESS"
21+
}
22+
}
23+
}
24+
]
25+

‎test/unit/pr_checker.test.js

+250
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ const {
1919
singleGreenReviewer,
2020
requestedChangesReviewers,
2121
approvingReviews,
22+
githubCI,
2223
requestingChangesReviews,
2324
commentsWithCI,
2425
commentsWithLiteCI,
@@ -737,6 +738,255 @@ describe('PRChecker', () => {
737738
});
738739
});
739740

741+
describe('checkGitHubCI', () => {
742+
const baseData = {
743+
pr: firstTimerPR,
744+
reviewers: allGreenReviewers,
745+
comments: commentsWithLGTM,
746+
reviews: approvingReviews,
747+
collaborators,
748+
authorIsNew: () => true,
749+
getThread() {
750+
return PRData.prototype.getThread.call(this);
751+
}
752+
};
753+
const testArgv = Object.assign({}, argv, { ciType: 'github-check' });
754+
755+
it('should error if no CI runs detected', () => {
756+
const cli = new TestCLI();
757+
758+
const expectedLogs = {
759+
error: [
760+
['No CI runs detected']
761+
]
762+
};
763+
764+
const data = Object.assign({}, baseData, { commits: githubCI['no-status'] });
765+
766+
const checker = new PRChecker(cli, data, testArgv);
767+
768+
const status = checker.checkCI();
769+
assert(!status);
770+
cli.assertCalledWith(expectedLogs);
771+
});
772+
773+
it('should error if both statuses failed', () => {
774+
const cli = new TestCLI();
775+
776+
const expectedLogs = {
777+
error: [
778+
['Last CI failed']
779+
]
780+
};
781+
782+
const data = Object.assign({}, baseData, { commits: githubCI['both-apis-failure'] });
783+
784+
const checker = new PRChecker(cli, data, testArgv);
785+
786+
const status = checker.checkCI();
787+
assert(!status);
788+
cli.assertCalledWith(expectedLogs);
789+
});
790+
791+
it('should succeed if both statuses succeeded', () => {
792+
const cli = new TestCLI();
793+
794+
const expectedLogs = {
795+
info: [
796+
['Last CI run was successful']
797+
]
798+
};
799+
800+
const data = Object.assign({}, baseData, { commits: githubCI['both-apis-success'] });
801+
802+
const checker = new PRChecker(cli, data, testArgv);
803+
804+
const status = checker.checkCI();
805+
assert(status);
806+
cli.assertCalledWith(expectedLogs);
807+
});
808+
809+
it('should error if Check suite failed', () => {
810+
const cli = new TestCLI();
811+
812+
const expectedLogs = {
813+
error: [
814+
['Last CI failed']
815+
]
816+
};
817+
818+
const data = Object.assign({}, baseData, { commits: githubCI['check-suite-failure'] });
819+
820+
const checker = new PRChecker(cli, data, testArgv);
821+
822+
const status = checker.checkCI();
823+
assert(!status);
824+
cli.assertCalledWith(expectedLogs);
825+
});
826+
827+
it('should error if Check suite pending', () => {
828+
const cli = new TestCLI();
829+
830+
const expectedLogs = {
831+
error: [
832+
['CI is still running']
833+
]
834+
};
835+
836+
const data = Object.assign({}, baseData, { commits: githubCI['check-suite-pending'] });
837+
838+
const checker = new PRChecker(cli, data, testArgv);
839+
840+
const status = checker.checkCI();
841+
assert(!status);
842+
cli.assertCalledWith(expectedLogs);
843+
});
844+
845+
it('should succeed if Check suite succeeded', () => {
846+
const cli = new TestCLI();
847+
848+
const expectedLogs = {
849+
info: [
850+
['Last CI run was successful']
851+
]
852+
};
853+
854+
const data = Object.assign({}, baseData, { commits: githubCI['check-suite-success'] });
855+
856+
const checker = new PRChecker(cli, data, testArgv);
857+
858+
const status = checker.checkCI();
859+
assert(status);
860+
cli.assertCalledWith(expectedLogs);
861+
});
862+
863+
it('should error if commit status failed', () => {
864+
const cli = new TestCLI();
865+
866+
const expectedLogs = {
867+
error: [
868+
['Last CI failed']
869+
]
870+
};
871+
872+
const data = Object.assign({}, baseData, { commits: githubCI['commit-status-only-failure'] });
873+
874+
const checker = new PRChecker(cli, data, testArgv);
875+
876+
const status = checker.checkCI();
877+
assert(!status);
878+
cli.assertCalledWith(expectedLogs);
879+
});
880+
881+
it('should error if commit status pending', () => {
882+
const cli = new TestCLI();
883+
884+
const expectedLogs = {
885+
error: [
886+
['CI is still running']
887+
]
888+
};
889+
890+
const data = Object.assign({}, baseData, { commits: githubCI['commit-status-only-pending'] });
891+
892+
const checker = new PRChecker(cli, data, testArgv);
893+
894+
const status = checker.checkCI();
895+
assert(!status);
896+
cli.assertCalledWith(expectedLogs);
897+
});
898+
899+
it('should succeed if commit status succeeded', () => {
900+
const cli = new TestCLI();
901+
902+
const expectedLogs = {
903+
info: [
904+
['Last CI run was successful']
905+
]
906+
};
907+
908+
const data = Object.assign({}, baseData, { commits: githubCI['commit-status-only-success'] });
909+
910+
const checker = new PRChecker(cli, data, testArgv);
911+
912+
const status = checker.checkCI();
913+
assert(status);
914+
cli.assertCalledWith(expectedLogs);
915+
});
916+
917+
it('should error if check suite succeeded but commit status failed ', () => {
918+
const cli = new TestCLI();
919+
920+
const expectedLogs = {
921+
error: [
922+
['Last CI failed']
923+
]
924+
};
925+
926+
const data = Object.assign({}, baseData, { commits: githubCI['status-failure-check-suite-succeed'] });
927+
928+
const checker = new PRChecker(cli, data, testArgv);
929+
930+
const status = checker.checkCI();
931+
assert(!status);
932+
cli.assertCalledWith(expectedLogs);
933+
});
934+
935+
it('should error if commit status succeeded but check suite failed ', () => {
936+
const cli = new TestCLI();
937+
938+
const expectedLogs = {
939+
error: [
940+
['Last CI failed']
941+
]
942+
};
943+
944+
const data = Object.assign({}, baseData, { commits: githubCI['status-succeed-check-suite-failure'] });
945+
946+
const checker = new PRChecker(cli, data, testArgv);
947+
948+
const status = checker.checkCI();
949+
assert(!status);
950+
cli.assertCalledWith(expectedLogs);
951+
});
952+
953+
it('should error if last commit doesnt have CI', () => {
954+
const cli = new TestCLI();
955+
956+
const expectedLogs = {
957+
error: [
958+
['No CI runs detected']
959+
]
960+
};
961+
962+
const data = Object.assign({}, baseData, { commits: githubCI['two-commits-first-ci'] });
963+
964+
const checker = new PRChecker(cli, data, testArgv);
965+
966+
const status = checker.checkCI();
967+
assert(!status);
968+
cli.assertCalledWith(expectedLogs);
969+
});
970+
971+
it('should succeed with two commits if last one has CI', () => {
972+
const cli = new TestCLI();
973+
974+
const expectedLogs = {
975+
info: [
976+
['Last CI run was successful']
977+
]
978+
};
979+
980+
const data = Object.assign({}, baseData, { commits: githubCI['two-commits-last-ci'] });
981+
982+
const checker = new PRChecker(cli, data, testArgv);
983+
984+
const status = checker.checkCI();
985+
assert(status);
986+
cli.assertCalledWith(expectedLogs);
987+
});
988+
});
989+
740990
describe('checkAuthor', () => {
741991
it('should check odd commits for first timers', () => {
742992
const cli = new TestCLI();

0 commit comments

Comments
 (0)
Please sign in to comment.