From aecac726a25e3e78c31384726e91febc64f138c0 Mon Sep 17 00:00:00 2001 From: Matheus Marchini Date: Mon, 9 Mar 2020 15:37:09 -0700 Subject: [PATCH] git-node: add GitHub status as a CI option (#369) 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 | 263 ++++++++++++++++++ 19 files changed, 623 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 771edcc9..c4b0532a 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -201,9 +201,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(); @@ -265,6 +276,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 (const { 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 6a3fa255..3aff3e99 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 0666ac0c..81087fa3 100644 --- a/lib/session.js +++ b/lib/session.js @@ -55,6 +55,7 @@ class Session { readme: this.readme, waitTimeSingleApproval: this.waitTimeSingleApproval, waitTimeMultiApproval: this.waitTimeMultiApproval, + ciType: this.ciType, prid: this.prid }; } @@ -91,6 +92,10 @@ class Session { return this.config.waitTimeMultiApproval; } + 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 9c85dfb4..d8ee3f85 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'); @@ -88,6 +91,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 (const item of readdirSync(path('./github-ci'))) { + if (!item.endsWith('.json')) { + continue; + } + githubCI[basename(item, '.json')] = readJSON(`./github-ci/${item}`); +}; + module.exports = { approved, requestedChanges, @@ -101,6 +113,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 a94194dd..dca6e5a9 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, noReviewers, commentsWithCI, @@ -841,6 +842,268 @@ 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 commits = githubCI['no-status']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['both-apis-failure']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['both-apis-success']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['check-suite-failure']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['check-suite-pending']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['check-suite-success']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['commit-status-only-failure']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['commit-status-only-pending']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['commit-status-only-success']; + const data = Object.assign({}, baseData, { commits }); + + const checker = new PRChecker(cli, data, testArgv); + + const status = checker.checkCI(); + assert(status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error if Check succeeded but commit status failed ', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['Last CI failed'] + ] + }; + + const commits = githubCI['status-failure-check-suite-succeed']; + const data = Object.assign({}, baseData, { commits }); + + 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 failed ', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['Last CI failed'] + ] + }; + + const commits = githubCI['status-succeed-check-suite-failure']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['two-commits-first-ci']; + const data = Object.assign({}, baseData, { commits }); + + 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 commits = githubCI['two-commits-last-ci']; + const data = Object.assign({}, baseData, { commits }); + + 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();