Start on the Github rewrite, with [GithubPullRequestCheckState] (badg…

…es#2253)

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 badges#2021 and badges#1205 is also related.

lib/all-badge-examples.js

@@ -1,16 +1,9 @@
 'use strict'
 
 const { loadServiceClasses } = require('../services')
-
-const githubDoc = `
-<p>
-  If your GitHub badge errors, it might be because you hit GitHub's rate limits.
-  <br>
-  You can increase Shields.io's rate limit by
-  <a href="https://img.shields.io/github-auth">going to this page</a> to add
-  Shields as a GitHub application on your GitHub account.
-</p>
-`
+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:

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,

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 })
   }

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
+      )
+  }
+}

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,
+}

services/github/github-helpers.js

@@ -5,17 +5,25 @@ const {
   checkErrorResponse: standardCheckErrorResponse,
 } = require('../../lib/error-helper')
 
+const documentation = `
+<p>
+  If your GitHub badge errors, it might be because you hit GitHub's rate limits.
+  <br>
+  You can increase Shields.io's rate limit by
+  <a href="https://img.shields.io/github-auth">going to this page</a> to add
+  Shields as a GitHub application on your GitHub account.
+</p>
+`
+
 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,
 }

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 })
+  }
+}

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' })