From 5dd4ee078bbd5e3cd64e616ff1350206c4e9b6c1 Mon Sep 17 00:00:00 2001 From: Paul Melnikow Date: Fri, 9 Nov 2018 16:22:48 -0500 Subject: [PATCH] Start on the Github rewrite, with [GithubPullRequestCheckState] (#2253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GitHub service family is the largest, and as yet untouched by our service rewrite. I thought I would start the process by tackling one service. This pull request has a few things going on: 1. Rename pull-request-status to pull-request-check-state. We have another badge called pull request status. It seems like the checks are called one thing in the UI and another thing in the API, which is unfortunate. If other folks have strong feelings about the name, I’ll defer. 2. Move its tests and tighten up the syntax. 3. Move its badge examples including the doc string. 4. Add a new helper `errorMessagesFor` to use in the new services in place of `githubCheckErrorResponse`. It seems like we didn’t really use the `errorMessages` parameter to `githubCheckErrorResponse`, so I pared this down. I’m not sure if this is the function we’ll ultimately want, but it seems like a good place to start. 5. Pull fetch code I _know_ we use in other places into `github-common-fetch`. As in the PR I just opened for azure-devops, this takes a functional approach to the shared code, which is more direct, nimble, and easy to reason about than inheritance. 6. Create `GithubAuthService` which functions identically to BaseJsonService, except for one thing, which is that it uses the token pool. I accomplished this by adding a `_requestFetcher` property to BaseService, which is initialized to `sendAndCacheRequest` in the constructor, and can be overridden in subclasses. Since we weren’t using `_sendAndCacheRequest` directly except in BaseService and tests, I removed that property. I like this approach to patching in the GitHub auth because it’s very simple and creates no new API exposure. However, the way we’re doing the dependency injection feels a bit odd. Maybe the eventual refactor of request-handler would be a godo time to revisit this. The GitHub requests go through many, many layers of indirection at this point. Later on it would be good to shave some of these off, perhaps once the legacy GitHub services have been converted, or when all the services are done and we can take another look at the base service hierarchy. The work in #2021 and #1205 is also related. --- lib/all-badge-examples.js | 25 +---- services/base-json.js | 1 + services/base.js | 6 +- services/github/github-auth-service.js | 18 +++ services/github/github-common-fetch.js | 22 ++++ services/github/github-helpers.js | 26 +++-- ...github-pull-request-check-state.service.js | 105 ++++++++++++++++++ .../github-pull-request-check-state.tester.js | 32 ++++++ .../github-pull-request-status.service.js | 85 -------------- services/github/github.tester.js | 12 -- 10 files changed, 202 insertions(+), 130 deletions(-) create mode 100644 services/github/github-auth-service.js create mode 100644 services/github/github-common-fetch.js create mode 100644 services/github/github-pull-request-check-state.service.js create mode 100644 services/github/github-pull-request-check-state.tester.js delete mode 100644 services/github/github-pull-request-status.service.js diff --git a/lib/all-badge-examples.js b/lib/all-badge-examples.js index 7f903e697053c..6f85ea358e260 100644 --- a/lib/all-badge-examples.js +++ b/lib/all-badge-examples.js @@ -1,16 +1,9 @@ 'use strict' const { loadServiceClasses } = require('../services') - -const githubDoc = ` -

- If your GitHub badge errors, it might be because you hit GitHub's rate limits. -
- You can increase Shields.io's rate limit by - going to this page to add - Shields as a GitHub application on your GitHub account. -

-` +const { + documentation: githubDoc, +} = require('../services/github/github-helpers') const allBadgeExamples = [ { @@ -653,18 +646,6 @@ const allBadgeExamples = [ keywords: ['GitHub', 'release', 'date'], documentation: githubDoc, }, - { - title: 'GitHub pull request check state', - previewUrl: '/github/status/s/pulls/badges/shields/1110.svg', - keywords: ['GitHub', 'pullrequest', 'detail', 'check'], - documentation: githubDoc, - }, - { - title: 'GitHub pull request check contexts', - previewUrl: '/github/status/contexts/pulls/badges/shields/1110.svg', - keywords: ['GitHub', 'pullrequest', 'detail', 'check'], - documentation: githubDoc, - }, { title: 'GitHub commit merge status', previewUrl: diff --git a/services/base-json.js b/services/base-json.js index dde261d0f1e5b..f0e0a0258ff73 100644 --- a/services/base-json.js +++ b/services/base-json.js @@ -22,6 +22,7 @@ class BaseJsonService extends BaseService { try { json = JSON.parse(buffer) } catch (err) { + logTrace(emojic.dart, 'Response JSON (unparseable)', json) throw new InvalidResponse({ prettyMessage: 'unparseable json response', underlyingError: err, diff --git a/services/base.js b/services/base.js index bde4b3ac83ad4..87869ddefb56d 100644 --- a/services/base.js +++ b/services/base.js @@ -24,7 +24,7 @@ const trace = require('./trace') class BaseService { constructor({ sendAndCacheRequest }, { handleInternalErrors }) { - this._sendAndCacheRequest = sendAndCacheRequest + this._requestFetcher = sendAndCacheRequest this._handleInternalErrors = handleInternalErrors } @@ -363,6 +363,8 @@ class BaseService { const serviceInstance = new ServiceClass( { sendAndCacheRequest: request.asPromise, + sendAndCacheRequestWithCallbacks: request, + githubApiProvider, }, serviceConfig ) @@ -416,7 +418,7 @@ class BaseService { async _request({ url, options = {}, errorMessages = {} }) { const logTrace = (...args) => trace.logTrace('fetch', ...args) logTrace(emojic.bowAndArrow, 'Request', url, '\n', options) - const { res, buffer } = await this._sendAndCacheRequest(url, options) + const { res, buffer } = await this._requestFetcher(url, options) logTrace(emojic.dart, 'Response status code', res.statusCode) return checkErrorResponse.asPromise(errorMessages)({ buffer, res }) } diff --git a/services/github/github-auth-service.js b/services/github/github-auth-service.js new file mode 100644 index 0000000000000..55dc996260d14 --- /dev/null +++ b/services/github/github-auth-service.js @@ -0,0 +1,18 @@ +'use strict' + +const BaseJsonService = require('../base-json') + +module.exports = class GithubAuthService extends BaseJsonService { + constructor(context, config) { + super(context, config) + + const { sendAndCacheRequestWithCallbacks, githubApiProvider } = context + + this._requestFetcher = async (url, { qs }) => + githubApiProvider.requestAsPromise( + sendAndCacheRequestWithCallbacks, + url, + qs + ) + } +} diff --git a/services/github/github-common-fetch.js b/services/github/github-common-fetch.js new file mode 100644 index 0000000000000..5347f32b479ee --- /dev/null +++ b/services/github/github-common-fetch.js @@ -0,0 +1,22 @@ +'use strict' + +const Joi = require('joi') +const { errorMessagesFor } = require('./github-helpers') + +const issueSchema = Joi.object({ + head: Joi.object({ + sha: Joi.string().required(), + }).required(), +}).required() + +async function fetchIssue(serviceInstance, { user, repo, number }) { + return serviceInstance._requestJson({ + schema: issueSchema, + url: `/repos/${user}/${repo}/pulls/${number}`, + errorMessages: errorMessagesFor('pull request or repo not found'), + }) +} + +module.exports = { + fetchIssue, +} diff --git a/services/github/github-helpers.js b/services/github/github-helpers.js index f7dd512cfdfee..75ad012c7f5a8 100644 --- a/services/github/github-helpers.js +++ b/services/github/github-helpers.js @@ -5,17 +5,25 @@ const { checkErrorResponse: standardCheckErrorResponse, } = require('../../lib/error-helper') +const documentation = ` +

+ If your GitHub badge errors, it might be because you hit GitHub's rate limits. +
+ You can increase Shields.io's rate limit by + going to this page to add + Shields as a GitHub application on your GitHub account. +

+` + function stateColor(s) { return { open: '2cbe4e', closed: 'cb2431', merged: '6f42c1' }[s] } -function checkStateColor(s) { +function errorMessagesFor(notFoundMessage = 'repo not found') { return { - pending: 'dbab09', - success: '2cbe4e', - failure: 'cb2431', - error: 'cb2431', - }[s] + 404: notFoundMessage, + 422: notFoundMessage, + } } function checkErrorResponse( @@ -27,16 +35,16 @@ function checkErrorResponse( ) { return standardCheckErrorResponse(badgeData, err, res, { ...errorMessages, - 404: notFoundMessage, - 422: notFoundMessage, + ...errorMessagesFor(notFoundMessage), }) } const commentsColor = colorScale([1, 3, 10, 25], undefined, true) module.exports = { + documentation, stateColor, - checkStateColor, commentsColor, + errorMessagesFor, checkErrorResponse, } diff --git a/services/github/github-pull-request-check-state.service.js b/services/github/github-pull-request-check-state.service.js new file mode 100644 index 0000000000000..f52196f1b60d4 --- /dev/null +++ b/services/github/github-pull-request-check-state.service.js @@ -0,0 +1,105 @@ +'use strict' + +const Joi = require('joi') +const countBy = require('lodash.countby') +const GithubAuthService = require('./github-auth-service') +const { fetchIssue } = require('./github-common-fetch') +const { documentation, errorMessagesFor } = require('./github-helpers') + +const schema = Joi.object({ + state: Joi.equal('failure', 'pending', 'success').required(), + statuses: Joi.array() + .items( + Joi.object({ + state: Joi.equal('error', 'failure', 'pending', 'success').required(), + }) + ) + .default([]), +}).required() + +module.exports = class GithubPullRequestCheckState extends GithubAuthService { + static get category() { + return 'other' + } + + static get route() { + return { + base: 'github/status', + pattern: ':which(s|contexts)/pulls/:user/:repo/:number(\\d+)', + } + } + + static get examples() { + return [ + { + title: 'GitHub pull request check state', + urlPattern: 's/pulls/:user/:repo/:number', + staticExample: this.render({ which: 's', state: 'pending' }), + exampleUrl: 's/pulls/badges/shields/1110', + keywords: ['GitHub', 'pullrequest', 'detail', 'check'], + documentation, + }, + { + title: 'GitHub pull request check contexts', + urlPattern: 'contexts/pulls/:user/:repo/:number', + staticExample: this.render({ + which: 'contexts', + state: 'pending', + stateCounts: { passed: 5, pending: 1 }, + }), + exampleUrl: 'contexts/pulls/badges/shields/1110', + keywords: ['GitHub', 'pullrequest', 'detail', 'check'], + documentation, + }, + ] + } + + static get defaultBadgeData() { + return { + label: 'checks', + logo: 'github', + } + } + + static render({ which, state, stateCounts }) { + let message + if (which === 'contexts') { + message = Object.entries(stateCounts) + .map(([state, count]) => `${count} ${state}`) + .join(', ') + } else { + message = state + } + + const color = { + pending: 'dbab09', + success: '2cbe4e', + failure: 'cb2431', + }[state] + + return { message, color } + } + + static transform({ state, statuses }) { + return { + state, + stateCounts: countBy(statuses, 'state'), + } + } + + async handle({ which, user, repo, number }) { + const { + head: { sha: ref }, + } = await fetchIssue(this, { user, repo, number }) + + // https://developer.github.com/v3/repos/statuses/#get-the-combined-status-for-a-specific-ref + const json = await this._requestJson({ + schema, + url: `/repos/${user}/${repo}/commits/${ref}/status`, + errorMessages: errorMessagesFor('commit not found'), + }) + const { state, stateCounts } = this.constructor.transform(json) + + return this.constructor.render({ which, state, stateCounts }) + } +} diff --git a/services/github/github-pull-request-check-state.tester.js b/services/github/github-pull-request-check-state.tester.js new file mode 100644 index 0000000000000..277d8982d1780 --- /dev/null +++ b/services/github/github-pull-request-check-state.tester.js @@ -0,0 +1,32 @@ +'use strict' + +const t = require('../create-service-tester')() +module.exports = t + +t.create('github pull request check state') + .get('/s/pulls/badges/shields/1110.json') + .expectJSON({ name: 'checks', value: 'failure' }) + +t.create('github pull request check state (pull request not found)') + .get('/s/pulls/badges/shields/5110.json') + .expectJSON({ name: 'checks', value: 'pull request or repo not found' }) + +t.create( + "github pull request check state (ref returned by github doens't exist" +) + .get('/s/pulls/badges/shields/1110.json') + .intercept( + nock => + nock('https://api.github.com', { allowUnmocked: true }) + .get('/repos/badges/shields/pulls/1110') + .reply(200, JSON.stringify({ head: { sha: 'abc123' } })) // Looks like a real ref, but isn't. + ) + .networkOn() + .expectJSON({ + name: 'checks', + value: 'commit not found', + }) + +t.create('github pull request check contexts') + .get('/contexts/pulls/badges/shields/1110.json') + .expectJSON({ name: 'checks', value: '1 failure' }) diff --git a/services/github/github-pull-request-status.service.js b/services/github/github-pull-request-status.service.js deleted file mode 100644 index b15d7528c6e72..0000000000000 --- a/services/github/github-pull-request-status.service.js +++ /dev/null @@ -1,85 +0,0 @@ -'use strict' - -const countBy = require('lodash.countby') -const LegacyService = require('../legacy-service') -const { - makeColorB, - makeBadgeData: getBadgeData, - makeLogo: getLogo, -} = require('../../lib/badge-data') -const { - checkStateColor: githubCheckStateColor, - checkErrorResponse: githubCheckErrorResponse, -} = require('./github-helpers') - -module.exports = class GithubPullRequestStatus extends LegacyService { - static registerLegacyRouteHandler({ camp, cache, githubApiProvider }) { - camp.route( - /^\/github\/status\/(s|contexts)\/pulls\/([^/]+)\/([^/]+)\/(\d+)\.(svg|png|gif|jpg|json)$/, - cache((queryParams, match, sendBadge, request) => { - const [, which, owner, repo, number, format] = match - const issueUri = `/repos/${owner}/${repo}/pulls/${number}` - const badgeData = getBadgeData('checks', queryParams) - if (badgeData.template === 'social') { - badgeData.logo = getLogo('github', queryParams) - } - githubApiProvider.request(request, issueUri, {}, (err, res, buffer) => { - if ( - githubCheckErrorResponse( - badgeData, - err, - res, - 'pull request or repo not found' - ) - ) { - sendBadge(format, badgeData) - return - } - try { - const parsedData = JSON.parse(buffer) - const ref = parsedData.head.sha - const statusUri = `/repos/${owner}/${repo}/commits/${ref}/status` - githubApiProvider.request( - request, - statusUri, - {}, - // eslint-disable-next-line handle-callback-err - (err, res, buffer) => { - try { - const parsedData = JSON.parse(buffer) - const state = (badgeData.text[1] = parsedData.state) - badgeData.colorscheme = null - badgeData.colorB = makeColorB( - githubCheckStateColor(state), - queryParams - ) - switch (which) { - case 's': - badgeData.text[1] = state - break - case 'contexts': { - const counts = countBy(parsedData.statuses, 'state') - badgeData.text[1] = Object.keys(counts) - .map(k => `${counts[k]} ${k}`) - .join(', ') - break - } - default: - throw Error('Unreachable due to regex') - } - sendBadge(format, badgeData) - } catch (e) { - badgeData.text[1] = 'invalid' - sendBadge(format, badgeData) - } - } - ) - } catch (e) { - badgeData.text[1] = 'invalid' - sendBadge(format, badgeData) - } - }) - }) - ) - } -} diff --git a/services/github/github.tester.js b/services/github/github.tester.js index 0c47b1ccc2466..5d8b9083bbd5a 100644 --- a/services/github/github.tester.js +++ b/services/github/github.tester.js @@ -762,18 +762,6 @@ t.create('github issue update') Joi.object().keys({ name: 'updated', value: isFormattedDate }) ) -t.create('github pull request check state') - .get('/status/s/pulls/badges/shields/1110.json') - .expectJSONTypes(Joi.object().keys({ name: 'checks', value: 'failure' })) - -t.create('github pull request check state (pull request not found)') - .get('/status/s/pulls/badges/shields/5110.json') - .expectJSON({ name: 'checks', value: 'pull request or repo not found' }) - -t.create('github pull request check contexts') - .get('/status/contexts/pulls/badges/shields/1110.json') - .expectJSONTypes(Joi.object().keys({ name: 'checks', value: '1 failure' })) - t.create('top language') .get('/languages/top/badges/shields.json') .expectJSONTypes(