refactor(octokit): migrate from rest methods to direct use of octokit.request

Author: travi

lib/plugins/collaborators.js

@@ -12,21 +12,21 @@ export default class Collaborators extends Diffable {
     }
   }
 
-  find () {
-    return this.github.repos
-      .listCollaborators({ repo: this.repo.repo, owner: this.repo.owner, affiliation: 'direct' })
-      .then(res => {
-        return res.data.map(user => {
-          return {
-            // Force all usernames to lowercase to avoid comparison issues.
-            username: user.login.toLowerCase(),
-            permission:
-              (user.permissions.admin && 'admin') ||
-              (user.permissions.push && 'push') ||
-              (user.permissions.pull && 'pull')
-          }
-        })
-      })
+  async find () {
+    const { data: collaborators } = await this.github.request('GET /repos/{owner}/{repo}/collaborators', {
+      repo: this.repo.repo,
+      owner: this.repo.owner,
+      affiliation: 'direct'
+    })
+
+    return collaborators.map(collaborator => ({
+      // Force all usernames to lowercase to avoid comparison issues.
+      username: collaborator.login.toLowerCase(),
+      permission:
+        (collaborator.permissions.admin && 'admin') ||
+        (collaborator.permissions.push && 'push') ||
+        (collaborator.permissions.pull && 'pull')
+    }))
   }
 
   comparator (existing, attrs) {
@@ -42,10 +42,13 @@ export default class Collaborators extends Diffable {
   }
 
   add (attrs) {
-    return this.github.repos.addCollaborator(Object.assign({}, attrs, this.repo))
+    return this.github.request('PUT /repos/{owner}/{repo}/collaborators/{username}', { ...attrs, ...this.repo })
   }
 
   remove (existing) {
-    return this.github.repos.removeCollaborator(Object.assign({ username: existing.username }, this.repo))
+    return this.github.request('DELETE /repos/{owner}/{repo}/collaborators/{username}', {
+      username: existing.username,
+      ...this.repo
+    })
   }
 }

lib/plugins/labels.js

@@ -19,8 +19,7 @@ export default class Labels extends Diffable {
   }
 
   find () {
-    const options = this.github.issues.listLabelsForRepo.endpoint.merge(this.wrapAttrs({ per_page: 100 }))
-    return this.github.paginate(options)
+    return this.github.paginate('GET /repos/{owner}/{repo}/labels', this.wrapAttrs({ per_page: 100 }))
   }
 
   comparator (existing, attrs) {
@@ -32,15 +31,15 @@ export default class Labels extends Diffable {
   }
 
   update (existing, attrs) {
-    return this.github.issues.updateLabel(this.wrapAttrs(attrs))
+    return this.github.request('PATCH /repos/{owner}/{repo}/labels/{name}', this.wrapAttrs(attrs))
   }
 
   add (attrs) {
-    return this.github.issues.createLabel(this.wrapAttrs(attrs))
+    return this.github.request('POST /repos/{owner}/{repo}/labels', this.wrapAttrs(attrs))
   }
 
   remove (existing) {
-    return this.github.issues.deleteLabel(this.wrapAttrs({ name: existing.name }))
+    return this.github.request('DELETE /repos/{owner}/{repo}/labels/{name}', this.wrapAttrs({ name: existing.name }))
   }
 
   wrapAttrs (attrs) {

lib/plugins/milestones.js

@@ -14,10 +14,7 @@ export default class Milestones extends Diffable {
   }
 
   find () {
-    const options = this.github.issues.listMilestones.endpoint.merge(
-      Object.assign({ per_page: 100, state: 'all' }, this.repo)
-    )
-    return this.github.paginate(options)
+    return this.github.paginate('GET /repos/{owner}/{repo}/milestones', { per_page: 100, state: 'all', ...this.repo })
   }
 
   comparator (existing, attrs) {
@@ -31,20 +28,31 @@ export default class Milestones extends Diffable {
   update (existing, attrs) {
     const { owner, repo } = this.repo
 
-    return this.github.issues.updateMilestone(
-      Object.assign({ milestone_number: existing.number }, attrs, { owner, repo })
-    )
+    return this.github.request('PATCH /repos/{owner}/{repo}/milestones/{milestone_number}', {
+      milestone_number: existing.number,
+      ...attrs,
+      owner,
+      repo
+    })
   }
 
   add (attrs) {
     const { owner, repo } = this.repo
 
-    return this.github.issues.createMilestone(Object.assign({}, attrs, { owner, repo }))
+    return this.github.request('POST /repos/{owner}/{repo}/milestones', {
+      ...attrs,
+      owner,
+      repo
+    })
   }
 
   remove (existing) {
     const { owner, repo } = this.repo
 
-    return this.github.issues.deleteMilestone(Object.assign({ milestone_number: existing.number }, { owner, repo }))
+    return this.github.request('DELETE /repos/{owner}/{repo}/milestones/{milestone_number}', {
+      milestone_number: existing.number,
+      owner,
+      repo
+    })
   }
 }

lib/plugins/repository.js

@@ -3,33 +3,31 @@ const enableAutomatedSecurityFixes = ({ github, settings, enabled }) => {
     return Promise.resolve()
   }
 
-  const args = {
+  const verb = enabled ? 'PUT' : 'DELETE'
+
+  return github.request(`${verb} /repos/{owner}/{repo}/automated-security-fixes`, {
     owner: settings.owner,
     repo: settings.repo,
     mediaType: {
       previews: ['london']
     }
-  }
-  const methodName = enabled ? 'enableAutomatedSecurityFixes' : 'disableAutomatedSecurityFixes'
-
-  return github.repos[methodName](args)
+  })
 }
 
 const enableVulnerabilityAlerts = ({ github, settings, enabled }) => {
   if (enabled === undefined) {
     return Promise.resolve()
   }
 
-  const args = {
+  const verb = enabled ? 'PUT' : 'DELETE'
+
+  return github.request(`${verb} /repos/{owner}/{repo}/vulnerability-alerts`, {
     owner: settings.owner,
     repo: settings.repo,
     mediaType: {
       previews: ['dorian']
     }
-  }
-  const methodName = enabled ? 'enableVulnerabilityAlerts' : 'disableVulnerabilityAlerts'
-
-  return github.repos[methodName](args)
+  })
 }
 
 export default class Repository {
@@ -46,23 +44,23 @@ export default class Repository {
     delete this.settings.enable_automated_security_fixes
   }
 
-  sync () {
+  async sync () {
     this.settings.name = this.settings.name || this.settings.repo
-    return this.github.repos
-      .update(this.settings)
-      .then(() => {
-        if (this.topics) {
-          return this.github.repos.replaceAllTopics({
-            owner: this.settings.owner,
-            repo: this.settings.repo,
-            names: this.topics.split(/\s*,\s*/),
-            mediaType: {
-              previews: ['mercy']
-            }
-          })
+
+    await this.github.request('PATCH /repos/{owner}/{repo}', this.settings)
+
+    if (this.topics) {
+      await this.github.request('PUT /repos/{owner}/{repo}/topics', {
+        owner: this.settings.owner,
+        repo: this.settings.repo,
+        names: this.topics.split(/\s*,\s*/),
+        mediaType: {
+          previews: ['mercy']
         }
       })
-      .then(() => enableVulnerabilityAlerts({ enabled: this.enableVulnerabilityAlerts, ...this }))
-      .then(() => enableAutomatedSecurityFixes({ enabled: this.enableAutomatedSecurityFixes, ...this }))
+    }
+
+    await enableVulnerabilityAlerts({ enabled: this.enableVulnerabilityAlerts, ...this })
+    await enableAutomatedSecurityFixes({ enabled: this.enableAutomatedSecurityFixes, ...this })
   }
 }

lib/plugins/rulesets.js

@@ -4,10 +4,15 @@ import Diffable from './diffable.js'
 
 export default class Rulesets extends Diffable {
   async find () {
-    const { data: rulesets } = await this.github.repos.getRepoRulesets({ ...this.repo, includes_parents: false })
+    const { data: rulesets } = await this.github.request('GET /repos/{owner}/{repo}/rulesets', {
+      ...this.repo,
+      includes_parents: false
+    })
 
     const expandedRulesetsData = await Promise.all(
-      rulesets.map(({ id }) => this.github.repos.getRepoRuleset({ ...this.repo, ruleset_id: id }))
+      rulesets.map(({ id }) =>
+        this.github.request('GET /repos/{owner}/{repo}/rulesets/{ruleset_id}', { ...this.repo, ruleset_id: id })
+      )
     )
 
     return expandedRulesetsData.map(({ data }) => data)
@@ -34,14 +39,21 @@ export default class Rulesets extends Diffable {
   }
 
   update (existing, attrs) {
-    return this.github.repos.updateRepoRuleset({ ...this.repo, ruleset_id: existing.id, ...attrs })
+    return this.github.request('PUT /repos/{owner}/{repo}/rulesets/{ruleset_id}', {
+      ...this.repo,
+      ruleset_id: existing.id,
+      ...attrs
+    })
   }
 
   remove (existing) {
-    return this.github.repos.deleteRepoRuleset({ ...this.repo, ruleset_id: existing.id })
+    return this.github.request('DELETE /repos/{owner}/{repo}/rulesets/{ruleset_id}', {
+      ...this.repo,
+      ruleset_id: existing.id
+    })
   }
 
   async add (attrs) {
-    await this.github.repos.createRepoRuleset({ ...this.repo, ...attrs })
+    await this.github.request('POST /repos/{owner}/{repo}/rulesets', { ...this.repo, ...attrs })
   }
 }

lib/plugins/teams.js

@@ -2,11 +2,11 @@ import Diffable from './diffable.js'
 
 // it is necessary to use this endpoint until GitHub Enterprise supports
 // the modern version under /orgs
-const teamRepoEndpoint = '/teams/:team_id/repos/:owner/:repo'
+const teamRepoEndpoint = '/teams/{team_id}/repos/{owner}/{repo}'
 
 export default class Teams extends Diffable {
   find () {
-    return this.github.repos.listTeams(this.repo).then(res => res.data)
+    return this.github.request('GET /repos/{owner}/{repo}/teams', this.repo).then(res => res.data)
   }
 
   comparator (existing, attrs) {
@@ -22,7 +22,7 @@ export default class Teams extends Diffable {
   }
 
   async add (attrs) {
-    const { data: existing } = await this.github.request('GET /orgs/:org/teams/:team_slug', {
+    const { data: existing } = await this.github.request('GET /orgs/{org}/teams/{team_slug}', {
       org: this.repo.owner,
       team_slug: attrs.name
     })

test/unit/index.test.js

@@ -28,7 +28,7 @@ describe('plugin', () => {
       }
     }
 
-    app = new Probot({ secret: any.string(), Octokit })
+    app = new Probot({ secret: any.string(), Octokit, appId: any.string(), privateKey: any.string() })
 
     event = {
       name: 'push',

test/unit/lib/plugins/collaborators.test.js

@@ -1,21 +1,19 @@
-import Collaborators from '../../../../lib/plugins/collaborators'
 import { jest } from '@jest/globals'
+import { when } from 'jest-when'
+
+import Collaborators from '../../../../lib/plugins/collaborators'
 
 describe('Collaborators', () => {
   let github
+  const repoOwner = 'bkeepers'
+  const repoName = 'test'
 
   function configure (config) {
-    return new Collaborators(github, { owner: 'bkeepers', repo: 'test' }, config)
+    return new Collaborators(github, { owner: repoOwner, repo: repoName }, config)
   }
 
   beforeEach(() => {
-    github = {
-      repos: {
-        listCollaborators: jest.fn().mockImplementation(() => Promise.resolve([])),
-        removeCollaborator: jest.fn().mockImplementation(() => Promise.resolve()),
-        addCollaborator: jest.fn().mockImplementation(() => Promise.resolve())
-      }
-    }
+    github = { request: jest.fn().mockImplementation(() => Promise.resolve()) }
   })
 
   describe('sync', () => {
@@ -27,41 +25,41 @@ describe('Collaborators', () => {
         { username: 'DIFFERENTcase', permission: 'push' }
       ])
 
-      github.repos.listCollaborators.mockReturnValueOnce(
-        Promise.resolve({
+      when(github.request)
+        .calledWith('GET /repos/{owner}/{repo}/collaborators', {
+          repo: repoName,
+          owner: repoOwner,
+          affiliation: 'direct'
+        })
+        .mockResolvedValue({
           data: [
             { login: 'bkeepers', permissions: { admin: true, push: true, pull: true } },
             { login: 'updated-permission', permissions: { admin: false, push: false, pull: true } },
             { login: 'removed-user', permissions: { admin: false, push: true, pull: true } },
             { login: 'differentCase', permissions: { admin: false, push: true, pull: true } }
           ]
         })
-      )
 
       return plugin.sync().then(() => {
-        expect(github.repos.addCollaborator).toHaveBeenCalledWith({
+        expect(github.request).toHaveBeenCalledWith('PUT /repos/{owner}/{repo}/collaborators/{username}', {
           owner: 'bkeepers',
           repo: 'test',
           username: 'added-user',
           permission: 'push'
         })
 
-        expect(github.repos.addCollaborator).toHaveBeenCalledWith({
+        expect(github.request).toHaveBeenCalledWith('PUT /repos/{owner}/{repo}/collaborators/{username}', {
           owner: 'bkeepers',
           repo: 'test',
           username: 'updated-permission',
           permission: 'push'
         })
 
-        expect(github.repos.addCollaborator).toHaveBeenCalledTimes(2)
-
-        expect(github.repos.removeCollaborator).toHaveBeenCalledWith({
+        expect(github.request).toHaveBeenCalledWith('DELETE /repos/{owner}/{repo}/collaborators/{username}', {
           owner: 'bkeepers',
           repo: 'test',
           username: 'removed-user'
         })
-
-        expect(github.repos.removeCollaborator).toHaveBeenCalledTimes(1)
       })
     })
   })