Extract GitHubApp logic

[bkeepers]

Now that we're migrated to TypeScript, this is another attempt (#410) to extract some of the GitHub App logic so it's more encapsulated and reusable. The goal is to get to where Application only has an event emitter and router, but no GitHub-specific logic.

There are a few things driving this refactoring:

1. People doing custom stuff can reach directly for an instance of GitHubApp instead of having to jump through hoops to get app.auth() and friends to work.
2. As we look to make it easier to run Probot in serverless environments, we override parts of GitHubApp (e.g. change how webhooks are delivered)
3. We could make it easier for other platforms to implement similar adapters.

TODO:

• Create a class to encapsulate all GitHub App logic: GitHubApp
• define createContext method and move to class
• Move jwt creation into class
• Move webhook handling into class WIP: Move Webhooks to GitHubApp #565
• Properly deprecate app.auth and figure out replacement

[bkeepers]

FIXME: extract this into a method so client creation can be reused.

[bkeepers]

FIXME: extract into a method so installation token logic can be reused.

[bkeepers]

FIXME: move to GitHubApp class

[tcbyrd]

minor nit, just to make sure it's consistent with the name of the TS type

@returns {Promise<GitHubAPI>} 

[tcbyrd]

@bkeepers I know there's more left to do here, but wanted to add that I tested invite-koa locally with this branch and it's 👍 so far.

Properly deprecate app.auth and figure out replacement

app.adapter.auth() is working for me when I do

const app = new Probot({
  id: process.env.APP_ID,
  cert: process.env.PRIVATE_KEY
})

const github = await app.adapter.auth()

In fact when I do that, app.auth() doesn't work at all. I might be missing some context here, though.

---

Do we want to encourage import {GitHubApp} from 'probot' so users can create the adapter without the logging and express server? Mainly I'm trying to think of ways to avoid things like in github-robot, where they disable the built-in logger so it can use the features of the Firebase platform:

// < /src/index.ts >
// disable probot logging
bot.logger.streams.splice(0, 1);
// Use node console as the output stream
bot.logger.addStream(consoleStream(store));

// < /src/util.js >
/**
 * Stream Probot logs to console for Firebase
 */
export const consoleStream = (store: FirebaseFirestore.Firestore) => ({
  type: "raw",
  level: "debug",
  stream: new Stream(store)
});

I think this is where you're going with making everything modular, but it could be nice if everything in Probot could be imported as needed, then probot run gives you the server to run it all on top of when you don't have an http router otherwise (in an existing App, in Serverless environments, etc.)

[gr2m]

Could you update documentation for the feature?

I think it might be useful to align work with @octokit/app? Sounds like ideally we would use that module eventually? See octokit/app.js#1

[tcbyrd]

@gr2m Yeh definitely looks like we could just move to that module. The API is the exact same. Is there a line we need to draw somewhere on the what's in Probot vs what's in Octokit? Once you have apps, pagination, & graphql in Octokit, it seems like Probot becomes mainly an opinionated way of running an Express server with Octokit.

[gr2m]

It seems like Probot becomes mainly an opinionated way of running an Express server with Octokit

Yeah that sounds about right. I think I’d also leave an http server out of Octokit so folks can use servers other than express.

Thanks for reviewing!

[hiimbex]

Is there still work to be done on this or was it handled in another ts pr?

I misunderstood this PR! 🙈

[bkeepers]

@probot/maintainers While there's a lot more I'd like to do here, I'd like to try to get this merged and finish the rest of the refactoring in #565.

Here are the notable changes in this PR:

• app.auth() and app.app() have been deprecated and will print these warnings when called:

      Method `app` has been deprecated, use `github.jwt` instead.
          at /Users/bkeepers/projects/probot/probot/test/application.test.ts:262:18
  
     Method `auth` has been deprecated, use `github.auth` instead.
          at /Users/bkeepers/projects/probot/probot/test/application.test.ts:268:24
  

• app.github is an instance of GitHubApp, which has:

  • jwt() - returns a new JWT for the GitHub app
  • auth(id?: number) - returns a new authenticated octokit client for the given installation id
  • createContext(event: WebhookEvent) - creates a new Context for the given webhook event

So an example of this being used:

module.exports = async app => {
  // returns a new octokit instance
  const github = await app.github.auth()
}

[hiimbex]

LGTM! Few minor comments

[hiimbex]

I know this is just copy pasted form the old but maybe here would be a good place to clarify that GitHub client = authenticated as an installation: https://developer.github.com/apps/building-github-apps/authenticating-with-github-apps/#authenticating-as-an-installation

[hiimbex]

Can we nix this comment? declaration is already true and allowJs is already false. Also json doens't support comments in the first place.

[hiimbex]

These tests would be clearer to me if we used app.github.auth to replace app.auth instead of github.auth.

Is there reasoning for not having it that way?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jamime]

@bkeepers What's the latest on this, happy to contribute.

[gr2m]

done in v10