Initial commits #1

pyokagan · 2017-06-14T08:25:57Z

No description provided.

All cool projects need to have a GitHub bot, so let's create our own. We'll use nodejs for the project since it can be easily deployed to Heroku, and we'll use Typescript in order to make working with Javascript more bearable[1]. We are also using the oss-generic Typescript coding guidelines, so let's setup tslint and editorconfig with the relevant rules to ensure that our code adheres to those guidelines. We will be using Heroku to host our bot, so let's add a Procfile as well, and document setup instructions in the README.adoc. [1] For more information on what makes Typescript better than plain Javascript, search for "Typescript rocks" or "Javascript sucks" on the internet.

The typescript compiler generates source maps for us, however node does not recognize them. As a result, we get error stack traces pointing to the generated .js files rather than the source .ts files: /se-edu-bot/dist/server.js:3 throw new Error('Some error'); ^ Error: Some error at Object.<anonymous> (/se-edu-bot/dist/server.js:3:11) ... This makes finding the exact line where the error occured in the source .ts files harder than it should be. As such, let's use the source-map-support package, which teaches node to use source maps to replace the paths and line numbers of source-mapped files with their original paths and line numbers. Using this package, we get error stack traces like this instead: /se-edu-bot/server.ts:5 throw new Error('Some error'); ^ Error: Some error at Object.<anonymous> (/se-edu-bot/server.ts:5:11) ...

We are going to be using environment variables to configure the bot, to store settings which do not belong in version control. Environment variables are used for configuration because they are strongly supported by Heroku. However, working with environment variables during development can be kind of unwieldy, as we will either need to set them up one by one on the command line every time we wish to develop se-edu-bot, or modify our system settings. It would be nice if we had a file where we could just dump all our environment variables inside, and server.ts would automatically pick them up when it runs. Fortunately, there is a popular package in the nodejs world called "dotenv" which does just that. It will load environment variables from the ".env" file in the project root when called to do so. Let's use that.

We'll need to implement a web server to respond to GitHub webhook events. As a step towards that goal, let's implement a simple web server using Koa[1] that responds with "Hello world!". Koa is used due to the simplicity of its core concepts (every middleware is a function that returns a promise). In later commits, we will be building on top of this web server code to fully flesh out se-edu-bot's behavior, including responding to GitHub webhook events. [1] https://github.com/koajs/koa

Tests are always good, as they can help weed out bugs and prevent regressions. While we don't specifically require our code to be well tested, it is still good to have a testing framework available at hand so that developers can implement tests when they want to. Let's use mocha[1] as the testing framework, and mocha-typescript[2] to make it pleasant to use in typescript. Set mocha up such that all *.spec.ts files are automatically run as tests. As an example of how to implement tests, write tests for server.ts as well. [1] https://mochajs.org/ [2] https://github.com/pana-cc/mocha-typescript

In order for se-edu-bot to be useful it will need to be aware of events which occur on GitHub. (e.g. it will need to be aware of when PRs are merged so that it can post comments to thank authors for their hard work). GitHub uses webhooks[1] to notify se-edu-bot of events that we are interested in. To receive these events, let's implement a webhook endpoint at /webhook. As an example of how to implement support for a webhook event type, let's also implement support for the "ping"[2] event. [1] https://developer.github.com/webhooks/ [2] https://developer.github.com/webhooks/#ping-event

We envision that se-edu-bot in the future will perform a large variety of tasks, and as such will have complex code implementing its behavior as well. To manage this eventual complexity, let's implement a modular architecture for defining the behavior of the bot, called "logic". Each module, which we shall call a "logic component", will implement one aspect of the bot's behavior. By composing logic components together, we can then implement complex bot behaviors while managing code complexity. As an example of a logic component, we also implement and use LogWebhookLogic, which will log to console all webhook payloads received by our web server.

Making unauthenticated GitHub API requests is simple enough that any user-friendly http client library such as request[1] should do the trick. On the other hand, making authenticated requests are tricky, yet in future commits we will need to use them since we will need to do things which requires authorization (e.g. posting a comment as the bot) in order for se-edu-bot to be useful. As such, let's implement support for making authenticated GitHub API requests via our lib/github library. Requests can be authenticated by App via JWT[2] or authorized by access token (users[3] and installations[4]). [1] https://github.com/request/request [2] https://developer.github.com/apps/building-integrations/setting-up-and-registering-github-apps/about-authentication-options-for-github-apps/#authenticating-as-a-github-app [3] https://developer.github.com/v3/#authentication [4] https://developer.github.com/apps/building-integrations/setting-up-and-registering-github-apps/about-authentication-options-for-github-apps/#authenticating-as-an-installation

While logic components can receive GitHub webhook events, they can't really act on them as they do not have access to the GitHub API. Give logic components access to the GitHub API by passing them a RequestApi object which is authorized with the installation of the GitHub App on the se-edu organization. This is implemented by implementing two Koa middlewares, koaGhAppApi() and koaGhInstallationApi(). The former installs a RequestApi on the koa context which is authorized with the GitHub App, while the latter installs a RequestApi on the koa context which is authorized with the installation access token.

Thank them for their hard work. Hopefully they will come back again.

Logs are useful for debugging errors in production. While heroku does have logs, only people who have access to the heroku app can access them. Heroku does allow adding collaborators to an app who can then access the logs as well, however collaborators also have access to other sensitive operations such as deleting the app or adding or removing paid addons. To allow us to access the logs without needing to become a heroku app collaborator, let's implement our own logging system. We can then access the logs via the /logs endpoint. Note, however, that *anyone* can access the logs, which is dangerous since the logs may contain sensitive info. However, this is no implemented yet since we do not have an authentication system. In future commits, when the authentication system has been implemented, we can then tweak the implementation such that only se-edu organization members can access the logs.

Pagination[1] is a pretty common thing in the GitHub API. To make it easy for our code to handle pagination, let's implement a forEachPage() helper function. [1] https://developer.github.com/v3/#pagination

Some functionality of se-edu-bot that we wish to implement in the future requires se-edu-bot to be aware of the user's identity. For GitHub Apps which is what se-edu-bot is using, this requires us to identify users via OAuth[1], which in turn requires us to implement OAuth endpoints. We will also need to use cookies to keep track of the user's session so we can persistently identify the user. [1] https://developer.github.com/apps/building-integrations/setting-up-and-registering-github-apps/identifying-users-for-github-apps/

The bot's logs are accessible to everyone at the /logs endpoint. This is dangerous because the logs may contain sensitive info. Fix this by only allowing se-edu organization members to view the logs. Note that our access control doesn't actually check if the current user is a member of se-edu, but rather whether the user has access to the se-edu-bot installation in the se-edu organization. This should be equivalent.

CanIHasReview-bot · 2017-06-15T07:29:58Z

v1

@pyokagan submitted v1 for review.

[v1 1/14] Initial commit
[v1 2/14] server: add source-map-support
[v1 3/14] server: implement support for ".env" files
[v1 4/14] server: implement "Hello world!" web server
[v1 5/14] Implement testing support with mocha
[v1 6/14] lib/github: implement Github webhook support
[v1 7/14] logic: implement logic architecture
[v1 8/14] lib/github: support making authenticated API requests
[v1 9/14] logic: implement github API support
[v1 10/14] logic: praise PR authors when their PRs are merged
[v1 11/14] logs: implement logging
[v1 12/14] lib/github: implement forEachPage() helper function
[v1 13/14] lib/Auth: implement support for authenticated user sessions
[v1 14/14] logs: only allow organization members to access them

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/se-edu-bot.git refs/pr/1/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan · 2017-06-15T07:32:01Z

@yamgent Can help me run through the README and see if the Heroku installation instructions work. (Install to wang-leng-org)

pyokagan

[1/14]

Initial commit

All cool projects need to have a GitHub bot, so let's create our own.

We'll use nodejs for the project since it can be easily deployed to
Heroku, and we'll use Typescript in order to make working with
Javascript more bearable[1].

We are also using the oss-generic Typescript coding guidelines, so let's
setup tslint and editorconfig with the relevant rules to ensure that our
code adheres to those guidelines.

We will be using Heroku to host our bot, so let's add a Procfile as
well, and document setup instructions in the README.adoc.

It's probably better to format all of these paragraphs as bullet points.

Also need to mention the .travis.yml.

It would also be good if the README.adoc has a "Background reading" section which links to topics that developers need to know when working on this code base.

pyokagan · 2017-06-17T07:49:32Z

README.adoc

+
+=== Prerequisites
+
+. Node `7.9.0` or later.


Need to specify required npm version as well.

pyokagan · 2017-06-17T07:51:57Z

README.adoc

+
+. Node `7.9.0` or later.
+. An editor with Typescript language service support is strongly recommended.
+  (e.g. https://code.visualstudio.com/[VS Code])


Maybe add a Recommended editors section.

Note that for VSCode, the editorconfig plugin must be installed for it to respect .editorconfig files.

pyokagan

[2/14]

server: add source-map-support

The typescript compiler generates source maps for us, however node does
not recognize them. As a result, we get error stack traces pointing to
the generated .js files rather than the source .ts files:

    /se-edu-bot/dist/server.js:3
        throw new Error('Some error');
        ^

    Error: Some error
        at Object.<anonymous> (/se-edu-bot/dist/server.js:3:11)
        ...

This makes finding the exact line where the error occured in the source
.ts files harder than it should be.

occured -> occurred

As such, let's use the source-map-support package, which teaches node to

Need to give the package URL.

use source maps to replace the paths and line numbers of source-mapped
files with their original paths and line numbers. Using this package, we
get error stack traces like this instead:

    /se-edu-bot/server.ts:5
        throw new Error('Some error');
              ^
    Error: Some error
        at Object.<anonymous> (/se-edu-bot/server.ts:5:11)
        ...

pyokagan

[3/14]

server: implement support for ".env" files

We are going to be using environment variables to configure the bot, to
store settings which do not belong in version control. Environment
variables are used for configuration because they are strongly supported
by Heroku.

However, working with environment variables during development can be
kind of unwieldy, as we will either need to set them up one by one on
the command line every time we wish to develop se-edu-bot, or modify our
system settings.

It would be nice if we had a file where we could just dump all our
environment variables inside, and server.ts would automatically pick
them up when it runs.

Fortunately, there is a popular package in the nodejs world called
"dotenv" which does just that. It will load environment variables from
the ".env" file in the project root when called to do so. Let's use
that.

Looks okay to me. Just need to add a link to the dotenv package in the commit message.

pyokagan

[4/14]

 server: implement "Hello world!" web server

pyokagan · 2017-06-20T02:37:44Z

README.adoc

+`PROXY`::
+(Required) Set to `true` if se-edu-bot is served behind a reverse proxy (e.g. ngrok or heroku).
+Given that we host se-edu-bot on heroku and use ngrok for development,
+this should usually be set to `true`.


Actually, given that for both development and production this should be set to true, we shouldn't even need this environment variable in the first place?

pyokagan · 2017-06-20T02:38:02Z

README.adoc

@@ -129,6 +129,17 @@ Below is a guide for setting up the Heroku application from scratch should there

 == Environment variables

+`PROXY`::
+(Required) Set to `true` if se-edu-bot is served behind a reverse proxy (e.g. ngrok or heroku).


Not required, but optional. Defaults to false.

pyokagan force-pushed the initial branch 17 times, most recently from ea01b44 to 4bc199a Compare June 14, 2017 16:39

pyokagan added 13 commits June 15, 2017 15:08

logic: praise PR authors when their PRs are merged

c35a85e

Thank them for their hard work. Hopefully they will come back again.

lib/github: implement forEachPage() helper function

627d9a4

Pagination[1] is a pretty common thing in the GitHub API. To make it easy for our code to handle pagination, let's implement a forEachPage() helper function. [1] https://developer.github.com/v3/#pagination

pyokagan force-pushed the initial branch from 4bc199a to 1eb4f04 Compare June 15, 2017 07:16

CanIHasReview-bot added the s.ToReview label Jun 15, 2017

pyokagan requested a review from yamgent June 15, 2017 07:31

README.adoc changes from dogfooding

d1fc430

pyokagan requested review from limmlingg, Zhiyuan-Amos and thenaesh June 15, 2017 08:30

pyokagan commented Jun 17, 2017

View reviewed changes

pyokagan commented Jun 20, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commits #1

Initial commits #1

pyokagan commented Jun 14, 2017

CanIHasReview-bot commented Jun 15, 2017

pyokagan commented Jun 15, 2017

pyokagan left a comment •

edited

Loading

pyokagan Jun 17, 2017

pyokagan Jun 17, 2017

pyokagan left a comment

pyokagan left a comment

pyokagan left a comment

pyokagan Jun 20, 2017

pyokagan Jun 20, 2017

Initial commits #1

Are you sure you want to change the base?

Initial commits #1

Conversation

pyokagan commented Jun 14, 2017

CanIHasReview-bot commented Jun 15, 2017

v1

pyokagan commented Jun 15, 2017

pyokagan left a comment • edited Loading

Choose a reason for hiding this comment

pyokagan Jun 17, 2017

Choose a reason for hiding this comment

pyokagan Jun 17, 2017

Choose a reason for hiding this comment

pyokagan left a comment

Choose a reason for hiding this comment

pyokagan left a comment

Choose a reason for hiding this comment

pyokagan left a comment

Choose a reason for hiding this comment

pyokagan Jun 20, 2017

Choose a reason for hiding this comment

pyokagan Jun 20, 2017

Choose a reason for hiding this comment

pyokagan left a comment •

edited

Loading