From 81cfc18863bd2f09113fc2a8a586da05ca54ab93 Mon Sep 17 00:00:00 2001 From: Matheus Marchini Date: Tue, 1 Oct 2019 18:12:45 -0700 Subject: [PATCH] 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. --- lib/pr_checker.js | 64 ++++- lib/queries/PRCommits.gql | 9 + lib/request.js | 3 +- lib/session.js | 5 + test/fixtures/data.js | 15 +- .../fixtures/github-ci/both-apis-failure.json | 24 ++ .../fixtures/github-ci/both-apis-success.json | 24 ++ .../github-ci/check-suite-failure.json | 20 ++ .../github-ci/check-suite-pending.json | 20 ++ .../github-ci/check-suite-success.json | 20 ++ .../github-ci/commit-status-only-failure.json | 16 ++ .../github-ci/commit-status-only-pending.json | 16 ++ .../github-ci/commit-status-only-success.json | 16 ++ test/fixtures/github-ci/no-status.json | 13 + .../status-failure-check-suite-succeed.json | 24 ++ .../status-succeed-check-suite-failure.json | 24 ++ .../github-ci/two-commits-first-ci.json | 25 ++ .../github-ci/two-commits-last-ci.json | 25 ++ test/unit/pr_checker.test.js | 250 ++++++++++++++++++ 19 files changed, 610 insertions(+), 3 deletions(-) create mode 100644 test/fixtures/github-ci/both-apis-failure.json create mode 100644 test/fixtures/github-ci/both-apis-success.json create mode 100644 test/fixtures/github-ci/check-suite-failure.json create mode 100644 test/fixtures/github-ci/check-suite-pending.json create mode 100644 test/fixtures/github-ci/check-suite-success.json create mode 100644 test/fixtures/github-ci/commit-status-only-failure.json create mode 100644 test/fixtures/github-ci/commit-status-only-pending.json create mode 100644 test/fixtures/github-ci/commit-status-only-success.json create mode 100644 test/fixtures/github-ci/no-status.json create mode 100644 test/fixtures/github-ci/status-failure-check-suite-succeed.json create mode 100644 test/fixtures/github-ci/status-succeed-check-suite-failure.json create mode 100644 test/fixtures/github-ci/two-commits-first-ci.json create mode 100644 test/fixtures/github-ci/two-commits-last-ci.json diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 88f58cb1..976c4f91 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -184,9 +184,20 @@ 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(); + } + this.cli.error(`Invalid ciType: ${ciType}`); + return false; + } + // TODO: we might want to check CI status when it's less flaky... // TODO: not all PR requires CI...labels? - checkCI() { + checkJenkinsCI() { const { cli, commits, argv } = this; const { maxCommits } = argv; const thread = this.data.getThread(); @@ -248,6 +259,57 @@ class PRChecker { return status; } + checkGitHubCI() { + const { cli, commits } = this; + + if (!commits) { + cli.error('No commits detected'); + return false; + } + + // NOTE(mmarchini): we only care about the last commit. Maybe in the future + // we'll want to check all commits for a successful CI. + const { commit } = commits[commits.length - 1]; + + this.CIStatus = false; + const checkSuites = commit.checkSuites || { nodes: [] }; + if (!commit.status && checkSuites.nodes.length === 0) { + cli.error('No CI runs detected'); + return false; + } + + // GitHub new Check API + for (let { status, conclusion } of checkSuites.nodes) { + if (status !== 'COMPLETED') { + cli.error('CI is still running'); + return false; + } + + if (!['SUCCESS', 'NEUTRAL'].includes(conclusion)) { + cli.error('Last CI failed'); + return false; + } + } + + // GitHub old commit status API + if (commit.status) { + const { state } = commit.status; + if (state === 'PENDING') { + cli.error('CI is still running'); + return false; + } + + if (!['SUCCESS', 'EXPECTED'].includes(state)) { + cli.error('Last CI failed'); + return false; + } + } + + cli.info('Last CI run was successful'); + this.CIStatus = true; + return true; + } + checkAuthor() { const { cli, commits, pr } = this; diff --git a/lib/queries/PRCommits.gql b/lib/queries/PRCommits.gql index 4eced126..f5a0647f 100644 --- a/lib/queries/PRCommits.gql +++ b/lib/queries/PRCommits.gql @@ -25,6 +25,15 @@ query Commits($prid: Int!, $owner: String!, $repo: String!, $after: String) { message messageHeadline authoredByCommitter + checkSuites(first: 10) { + nodes { + conclusion, + status + } + } + status { + state + } } } } diff --git a/lib/request.js b/lib/request.js index 48c98b1f..48b7bed3 100644 --- a/lib/request.js +++ b/lib/request.js @@ -68,7 +68,8 @@ class Request { method: 'POST', headers: { 'Authorization': `Basic ${githubCredentials}`, - 'User-Agent': 'node-core-utils' + 'User-Agent': 'node-core-utils', + 'Accept': 'application/vnd.github.antiope-preview+json' }, body: JSON.stringify({ query: query, diff --git a/lib/session.js b/lib/session.js index 0d6d1dc8..1aa71968 100644 --- a/lib/session.js +++ b/lib/session.js @@ -50,6 +50,7 @@ class Session { upstream: this.upstream, branch: this.branch, readme: this.readme, + ciType: this.ciType, prid: this.prid }; } @@ -78,6 +79,10 @@ class Session { return this.config.readme; } + get ciType() { + return this.config.ciType || 'jenkins'; + } + get pullName() { return `${this.owner}/${this.repo}/pulls/${this.prid}`; } diff --git a/test/fixtures/data.js b/test/fixtures/data.js index 678d85f1..81c31f21 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -1,6 +1,9 @@ 'use strict'; -const { readJSON, patchPrototype, readFile } = require('./index'); +const { basename } = require('path'); +const { readdirSync } = require('fs'); + +const { readJSON, patchPrototype, readFile, path } = require('./index'); const { Collaborator } = require('../../lib/collaborators'); const { Review } = require('../../lib/reviews'); @@ -82,6 +85,15 @@ const readmeNoCollaborators = readFile('./README/README_no_collaborators.md'); const readmeNoCollaboratorE = readFile('./README/README_no_collaboratorE.md'); const readmeUnordered = readFile('./README/README_unordered.md'); +const githubCI = {}; + +for (let item of readdirSync(path('./github-ci'))) { + if (!item.endsWith('.json')) { + continue; + } + githubCI[basename(item, '.json')] = readJSON(`./github-ci/${item}`); +}; + module.exports = { approved, requestedChanges, @@ -94,6 +106,7 @@ module.exports = { commentsWithLiteCI, commentsWithLGTM, oddCommits, + githubCI, incorrectGitConfigCommits, simpleCommits, singleCommitAfterReview, diff --git a/test/fixtures/github-ci/both-apis-failure.json b/test/fixtures/github-ci/both-apis-failure.json new file mode 100644 index 00000000..3866f56d --- /dev/null +++ b/test/fixtures/github-ci/both-apis-failure.json @@ -0,0 +1,24 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "status": { + "state": "FAILURE" + }, + "checkSuites": { + "nodes": [ + { + "status": "COMPLETED", + "conclusion": "FAILURE" + } + ] + } + } + } +] + diff --git a/test/fixtures/github-ci/both-apis-success.json b/test/fixtures/github-ci/both-apis-success.json new file mode 100644 index 00000000..86fb337c --- /dev/null +++ b/test/fixtures/github-ci/both-apis-success.json @@ -0,0 +1,24 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "status": { + "state": "SUCCESS" + }, + "checkSuites": { + "nodes": [ + { + "status": "COMPLETED", + "conclusion": "SUCCESS" + } + ] + } + } + } +] + diff --git a/test/fixtures/github-ci/check-suite-failure.json b/test/fixtures/github-ci/check-suite-failure.json new file mode 100644 index 00000000..9f34c110 --- /dev/null +++ b/test/fixtures/github-ci/check-suite-failure.json @@ -0,0 +1,20 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "checkSuites": { + "nodes": [ + { + "status": "COMPLETED", + "conclusion": "FAILURE" + } + ] + } + } + } +] diff --git a/test/fixtures/github-ci/check-suite-pending.json b/test/fixtures/github-ci/check-suite-pending.json new file mode 100644 index 00000000..de284a9a --- /dev/null +++ b/test/fixtures/github-ci/check-suite-pending.json @@ -0,0 +1,20 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "checkSuites": { + "nodes": [ + { + "status": "IN_PROGRESS" + } + ] + } + } + } +] + diff --git a/test/fixtures/github-ci/check-suite-success.json b/test/fixtures/github-ci/check-suite-success.json new file mode 100644 index 00000000..ed7f3d6b --- /dev/null +++ b/test/fixtures/github-ci/check-suite-success.json @@ -0,0 +1,20 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "checkSuites": { + "nodes": [ + { + "status": "COMPLETED", + "conclusion": "SUCCESS" + } + ] + } + } + } +] diff --git a/test/fixtures/github-ci/commit-status-only-failure.json b/test/fixtures/github-ci/commit-status-only-failure.json new file mode 100644 index 00000000..077b754d --- /dev/null +++ b/test/fixtures/github-ci/commit-status-only-failure.json @@ -0,0 +1,16 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "status": { + "state": "FAILURE" + } + } + } +] + diff --git a/test/fixtures/github-ci/commit-status-only-pending.json b/test/fixtures/github-ci/commit-status-only-pending.json new file mode 100644 index 00000000..15c4ba64 --- /dev/null +++ b/test/fixtures/github-ci/commit-status-only-pending.json @@ -0,0 +1,16 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "status": { + "state": "PENDING" + } + } + } +] + diff --git a/test/fixtures/github-ci/commit-status-only-success.json b/test/fixtures/github-ci/commit-status-only-success.json new file mode 100644 index 00000000..53d332c0 --- /dev/null +++ b/test/fixtures/github-ci/commit-status-only-success.json @@ -0,0 +1,16 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "status": { + "state": "SUCCESS" + } + } + } +] + diff --git a/test/fixtures/github-ci/no-status.json b/test/fixtures/github-ci/no-status.json new file mode 100644 index 00000000..8ef841d3 --- /dev/null +++ b/test/fixtures/github-ci/no-status.json @@ -0,0 +1,13 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + } + } + } +] + diff --git a/test/fixtures/github-ci/status-failure-check-suite-succeed.json b/test/fixtures/github-ci/status-failure-check-suite-succeed.json new file mode 100644 index 00000000..98b03e69 --- /dev/null +++ b/test/fixtures/github-ci/status-failure-check-suite-succeed.json @@ -0,0 +1,24 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "status": { + "state": "FAILURE" + }, + "checkSuites": { + "nodes": [ + { + "status": "COMPLETED", + "conclusion": "SUCCESS" + } + ] + } + } + } +] + diff --git a/test/fixtures/github-ci/status-succeed-check-suite-failure.json b/test/fixtures/github-ci/status-succeed-check-suite-failure.json new file mode 100644 index 00000000..cef60ce6 --- /dev/null +++ b/test/fixtures/github-ci/status-succeed-check-suite-failure.json @@ -0,0 +1,24 @@ +[ + { + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "status": { + "state": "SUCCESS" + }, + "checkSuites": { + "nodes": [ + { + "status": "COMPLETED", + "conclusion": "FAILURE" + } + ] + } + } + } +] + diff --git a/test/fixtures/github-ci/two-commits-first-ci.json b/test/fixtures/github-ci/two-commits-first-ci.json new file mode 100644 index 00000000..84fc6fd3 --- /dev/null +++ b/test/fixtures/github-ci/two-commits-first-ci.json @@ -0,0 +1,25 @@ +[ + { + "commit": { + "committedDate": "2017-10-25T11:27:02Z", + "oid": "6c0945cbeea2cbbc97d13a3d9e3fe68bd145b985", + "messageHeadline": "fixup: adjust spelling", + "author": { + "login": "bar" + }, + "status": { + "state": "SUCCESS" + } + } + },{ + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + } + } + } +] + diff --git a/test/fixtures/github-ci/two-commits-last-ci.json b/test/fixtures/github-ci/two-commits-last-ci.json new file mode 100644 index 00000000..8f0b6c9f --- /dev/null +++ b/test/fixtures/github-ci/two-commits-last-ci.json @@ -0,0 +1,25 @@ +[ + { + "commit": { + "committedDate": "2017-10-25T11:27:02Z", + "oid": "6c0945cbeea2cbbc97d13a3d9e3fe68bd145b985", + "messageHeadline": "fixup: adjust spelling", + "author": { + "login": "bar" + } + } + },{ + "commit": { + "committedDate": "2017-10-26T12:10:20Z", + "oid": "9d098ssiskj8dhd39js0sjd0cn2ng4is9n40sj12d", + "messageHeadline": "doc: add api description README", + "author": { + "login": "foo" + }, + "status": { + "state": "SUCCESS" + } + } + } +] + diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index bd2f38bd..aa3672b4 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -19,6 +19,7 @@ const { singleGreenReviewer, requestedChangesReviewers, approvingReviews, + githubCI, requestingChangesReviews, commentsWithCI, commentsWithLiteCI, @@ -737,6 +738,255 @@ describe('PRChecker', () => { }); }); + describe('checkGitHubCI', () => { + const baseData = { + pr: firstTimerPR, + reviewers: allGreenReviewers, + comments: commentsWithLGTM, + reviews: approvingReviews, + collaborators, + authorIsNew: () => true, + getThread() { + return PRData.prototype.getThread.call(this); + } + }; + const testArgv = Object.assign({}, argv, { ciType: 'github-check' }); + + it('should error if no CI runs detected', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['No CI runs detected'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['no-status'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error if both statuses failed', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['Last CI failed'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['both-apis-failure'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should succeed if both statuses succeeded', () => { + const cli = new TestCLI(); + + const expectedLogs = { + info: [ + ['Last CI run was successful'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['both-apis-success'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error if Check suite failed', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['Last CI failed'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['check-suite-failure'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error if Check suite pending', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['CI is still running'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['check-suite-pending'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should succeed if Check suite succeeded', () => { + const cli = new TestCLI(); + + const expectedLogs = { + info: [ + ['Last CI run was successful'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['check-suite-success'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error if commit status failed', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['Last CI failed'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['commit-status-only-failure'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error if commit status pending', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['CI is still running'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['commit-status-only-pending'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should succeed if commit status succeeded', () => { + const cli = new TestCLI(); + + const expectedLogs = { + info: [ + ['Last CI run was successful'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['commit-status-only-success'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error if check suite succeeded but commit status failed ', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['Last CI failed'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['status-failure-check-suite-succeed'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error if commit status succeeded but check suite failed ', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['Last CI failed'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['status-succeed-check-suite-failure'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error if last commit doesnt have CI', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['No CI runs detected'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['two-commits-first-ci'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should succeed with two commits if last one has CI', () => { + const cli = new TestCLI(); + + const expectedLogs = { + info: [ + ['Last CI run was successful'] + ] + }; + + const data = Object.assign({}, baseData, { commits: githubCI['two-commits-last-ci'] }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(status); + cli.assertCalledWith(expectedLogs); + }); + }); + describe('checkAuthor', () => { it('should check odd commits for first timers', () => { const cli = new TestCLI();