[WB-25] Handle installation events #20

Druotic · 2017-10-02T01:03:14Z

Doesn't distinguish between create/delete yet - just tries to always insert. Next PR will delete/update as well.

…lete yet

Druotic · 2017-10-02T01:04:45Z

package.json

    "eslint-plugin-promise": "^3.5.0",
    "eslint-plugin-standard": "^3.0.1",
    "git-hooks": "^1.1.8",
    "nyc": "^11.1.0",
    "proxyquire": "^1.8.0",
-    "sinon": "^2.4.1"
+    "sinon": "^2.4.1",
+    "uuid": "^3.1.0"


Ugh, forgot to remove this, not used.

Druotic · 2017-10-02T01:05:09Z

package.json

  },
  "dependencies": {
    "amqplib": "^0.5.1",
    "bluebird": "^3.5.0",
    "browser-refresh": "^1.7.2",
-    "fashion-model-defaults": "^1.0.2",
+    "bunyan": "^1.8.12",


Ugh, forgot to remove this, not used.

Druotic · 2017-10-02T01:05:18Z

package.json

-    "fashion-model-defaults": "^1.0.2",
+    "bunyan": "^1.8.12",
+    "fashion-model-defaults": "^1.1.0",
+    "github": "^10.1.0",


Not used yet, but will be in the next PR.

Druotic · 2017-10-02T01:06:28Z

src/config/Config.js

+
+    userAgent: {
+      description: 'windbreaker User-Agent to use when making API calls',
+      default: 'Windbreaker'


Technically not used in this PR - will be used in the next PR.

Druotic · 2017-10-02T01:08:54Z

src/dao/createDatabase.js

@@ -0,0 +1,26 @@
+require('require-self-ref')


This whole file is gross but exists due to limitations with knex/migrations. You can't create databases within a migration because it fails on the initial connect attempting to connect to a database that doesn't exist. Instead, we run this script just before the migration to ensure the database exists for the migrations.

Yeah that sucks. Maybe we should open an issue on knex to see if we can't get it supported.

Might be good to move this into another folder, possibly along the lines of src/dao/_migration-helpers. Just to make it obvious that it's a utility that shouldn't be imported.

Druotic · 2017-10-02T01:11:31Z

src/dao/index.js

+const FashionKnex = require('windbreaker-service-util/dao/FashionKnex')
+
+// TODO: move this base implementation class to shared utils?
+class BaseDao extends FashionKnex {


This approach is a little different from the dao pattern in hooks. I needed a way to extend common functionality for each dao/table and opted for this over using createDaoHelper.

It might be better to hide the underlying implementation, so that if we ever want to swap/change it in utils, it will be changed everywhere. Thoughts on just overriding the createDaoHelper function?

const createDaoUtil = require('windbreaker-service-util/dao') const config = require('~/src/config') const logger = require('~/src/logging').logger(module) module.exports = function createDaoHelper (options) { return createDaoUtil({ ...options, logger, knexConfig: config.getKnex() }) }

But I agree that ultimately this functionality should be overwritten since you're using it in multiple places.

I went with this approach because it naturally felt like inheritance - there's some base behavior, and we want to extend it per dao type. Currently, there isn't much common functionality here but thought there might be eventually. I originally extended BaseDao for each specific Dao, but it started to trend toward a singleton pattern and I'm not a huge fan of singletons. It does seem odd to use a BaseDao and not extend it, though. I could be convinced either way.

Druotic · 2017-10-02T01:12:23Z

src/dao/migrations/20170909162220_create-installation-table.js

+      // to the Installation model
+      table.integer('id').unsigned().primary()
+      table.integer('app_id').notNullable()
+      table.timestamps(true, true) // adds created_at/updated_at columns and default to now


Not actually sure if we'll ever care about these timestamps, but seemed like it'd be good to know create/updated times.

Druotic · 2017-10-02T01:17:16Z

test/integration/helpers/setup.js

@@ -0,0 +1,20 @@
+const startupTasks = require('~/src/startup-tasks')


FYI - this file doesn't get run as a regular test. Ava ignores all directories named helpers by default. A bad choice imo, but since exclude patterns don't work, may as well take advantage of it for test helpers.

See issue/code:

https://github.com/avajs/ava/blob/87eef846392d04b727b57981a16f7e8429a04195/lib/ava-files.js#L76

Only exclude helpers directory when inside a test directory avajs/ava#909

https://github.com/avajs/ava/blob/bdc57f4f9d69d6998fa7b764590b8e4d1f0fbe3a/test/watcher.js#L585

Yeah I agree that it was a bad decision. Definitely a random "gotcha" that adds little value.

austinkelleher

Nice work on this. Most of my comments are optional. Just thoughts that I wanted to record as I was reviewing. I think this looks pretty good.

austinkelleher · 2017-10-02T12:03:58Z

src/dao/createDatabase.js

+const knex = Knex(knexConfig)
+
+logger.info(`Attempting to create database '${dbName}'`)
+module.exports = knex


We should consider moving this part of this functionality to util

austinkelleher · 2017-10-02T12:04:21Z

src/dao/createDatabase.js

@@ -0,0 +1,26 @@
+require('require-self-ref')


Yeah that sucks. Maybe we should open an issue on knex to see if we can't get it supported.

austinkelleher · 2017-10-02T12:10:03Z

src/dao/github/installation.js

+  return dao.upsert({ id, app_id })
+}
+
+exports.findById = async function (id) {


I realize that this is just copied over from hooks. What do you about if a method is not being overridden in any way, we just reference the original:

exports.findById = dao.findById exports.close = dao.destroy

I don't feel strongly about this. Just a thought. I may actually prefer it the way that it is because it explicitly shows that the methods are async.

Ideally, we would just be extending the BaseDao here and everything would be a passthrough. I'm not sure these extra layers provide much value.

austinkelleher · 2017-10-02T12:20:15Z

src/dao/index.js

+const FashionKnex = require('windbreaker-service-util/dao/FashionKnex')
+
+// TODO: move this base implementation class to shared utils?
+class BaseDao extends FashionKnex {


It might be better to hide the underlying implementation, so that if we ever want to swap/change it in utils, it will be changed everywhere. Thoughts on just overriding the createDaoHelper function?

const createDaoUtil = require('windbreaker-service-util/dao') const config = require('~/src/config') const logger = require('~/src/logging').logger(module) module.exports = function createDaoHelper (options) { return createDaoUtil({ ...options, logger, knexConfig: config.getKnex() }) }

austinkelleher · 2017-10-02T12:21:16Z

src/dao/index.js

+const FashionKnex = require('windbreaker-service-util/dao/FashionKnex')
+
+// TODO: move this base implementation class to shared utils?
+class BaseDao extends FashionKnex {


But I agree that ultimately this functionality should be overwritten since you're using it in multiple places.

austinkelleher · 2017-10-02T12:21:53Z

src/dao/migrations/20170909162220_create-installation-table.js

+      // to the Installation model
+      table.integer('id').unsigned().primary()
+      table.integer('app_id').notNullable()
+      table.timestamps(true, true) // adds created_at/updated_at columns and default to now


austinkelleher · 2017-10-02T12:35:42Z

src/dao/migrations/20170909165723_create-repository-table.js

+
+  return knex
+    .schema
+    .createTableIfNotExists(REPOSITORY_TABLE_NAME, (table) => {


I think this is a good start, but it might be nice if we included more information so that we could do some statistics on the data later on. I think a few more that would be nice to have are:

private: Boolean # Whether this repo is private or public (this is subject to change...can we detect that?)

topics: String[] # Topics associated with this repo e.g. dom, vdom, front-end, javascript

created_at: Date # The original data when the repo was created (I think)

language: String # Main programming language of the repo

stargazers_count: Int # The number of repo stars at the time the integration was enabled

Is this supposed to serve as a generic table for all source control providers? If so, some of the above may not be applicable to all, but I think it's still worth having for the ones that do support it.

Some additional info regarding the data from GitHub: https://developer.github.com/v3/repos/#list-organization-repositories

This table is merely meant for the installation:repository (1:many) relationship. Metadata about a repository should be in a different table. The primary key (id) can also be a foreign key into that repository metadata table since it's a 1:1 relationship. If we go with a setup like that, it might be better to call this table something like InstallationRepositories (or snakecased to be consistent)

austinkelleher · 2017-10-02T12:41:37Z

src/dao/github/repository.js

+* @param full_name {String} - owner + repository name e.g. druotic/cool-repo
+* @param installation_id {Number} - foreign key id for associated installation
+*/
+exports.upsert = async function ({ id, name, full_name, installation_id }) {


FashionKnex.prototype.upsert already allows you to pass a Model. It might be cleaner to just do that. You may have to change up the way the id is passed though.

For consistency, I was merely following your existing pattern in hooks of providing a passthrough for everything. This suggestion seems like a good candidate for refactoring PRs across the projects to help maintain consistency.

austinkelleher · 2017-10-02T12:43:10Z

test/integration/helpers/setup.js

@@ -0,0 +1,20 @@
+const startupTasks = require('~/src/startup-tasks')


Yeah I agree that it was a bad decision. Definitely a random "gotcha" that adds little value.

ctdio · 2017-10-11T21:30:24Z

src/dao/migrations/20170909162220_create-installation-table.js

+const INSTALLATION_TABLE_NAME = Installation.typeName
+
+exports.up = function (knex, Promise) {
+  logger.info('Attempting to run "up" on "create-installation-table"')


Is this an old debug log?

No -I followed the existing pattern here: https://github.com/windbreaker-io/windbreaker-hooks/blob/master/src/dao/migrations/20170714072908_create-webhook-table.js I left it in intentionally for debugging purposes (only runs on startup).

Gotcha, it seemed like a pretty similar message to the one below.

Ah, now I see that you're pointing out the fact that there are two log statements, rather than suggesting to remove them entirely. These could be combined, I suppose. A future PR could combine them, but I was just being consistent.

ctdio · 2017-10-11T21:30:38Z

src/dao/migrations/20170909165723_create-repository-table.js

+const REPOSITORY_TABLE_NAME = Repository.typeName
+
+exports.up = function (knex, Promise) {
+  logger.info('Attempting to run "up" on "create-repository-table"')


ctdio · 2017-10-11T21:37:28Z

test/integration/messeges/events/webhook/github/github-installation-test.js

+  const repositories = [{
+    id: randomNum(),
+    name: 'three-mocha-puppeteers',
+    full_name: 'charlieDugong/three-mocha-puppeteers'


very funny...

ctdio · 2017-10-11T21:38:58Z

src/dao/knexfile.js

+
+const environment = config.getEnvironment().name().toLowerCase()
+let exported = {}
+exported[environment] = config.getKnex().clean()


minor: why not just do exports[environment] = config.getKnex().clean()?

Will do - copy/pasta OP.

ctdio · 2017-10-11T21:44:15Z

test/integration/helpers/setup.js

+  test.after(async () => {
+    await startupTasks.stopAll()
+  })
+}


Nice, I like this

ctdio · 2017-10-11T21:48:44Z

package.json

+        "transform-object-rest-spread"
+      ]
+    },
+    "failFast": true


I think I would rather not have this mode enabled. If we ever encounter a test failure, we could potentially miss out on running related tests that would better describe the root of the problem (ex. an integration test fails in a way that isn't obvious, but ends up being easier to debug because of related unit test failures).

I generally find that the first failure is usually the relevant failure and everything else tends to be noise and/or spams output that causes the root issue to be harder to debug. It's a minor preference, though. I can just enable it locally only.

ctdio · 2017-10-11T21:52:53Z

src/messages/events/webhook/github/github-installation.js

@@ -0,0 +1,32 @@
+require('require-self-ref')


I don't think this is needed.

require-self-ref is pretty much the first thing that is loaded upon starting up the server and is always required in at the start of every ava test file (from package.json). Unless there is something strange that happens during the event handler loading, I don't see why we would need to explicitly import require-self-ref again.

I usually prefer to be explicit as possible so there's less magic, but I'm okay with removing it if this is how it's most commonly used.

ctdio · 2017-10-11T22:02:41Z

src/dao/createDatabase.js

@@ -0,0 +1,26 @@
+require('require-self-ref')


Might be good to move this into another folder, possibly along the lines of src/dao/_migration-helpers. Just to make it obvious that it's a utility that shouldn't be imported.

ctdio · 2017-10-11T22:10:44Z

src/dao/github/repository.js

+* @param id {Number} - github repository id
+* @param name {String} - name of repository
+* @param full_name {String} - owner + repository name e.g. druotic/cool-repo
+* @param installation_id {Number} - foreign key id for associated installation


Ugghh... not a fan of mixing cases like this. Do you think it's worth normalizing this data in our models? (using fashion model's coerce)

I think it's worth discussing how we want our db models to look now so that we don't end up having mixed casing.

Can you give an example? It seems like coerce is meant for type coercion rather than coercing property names, or the docs certainly don't give any examples.

Ultimately, I don't care at all about how fashion model/typing is used, I just want it to be obvious what the casing of a payload looks like without an excessive number of abstractions.

Sorry, was thinking of init not coerce.

Here's an example using it.

const MyModel = require('fashion-model').extend({ properties: { name: String, property: String }, init (input) { this.setProperty(input.some_property) } }) const model = MyModel.wrap({ name: 'Jams', some_property: 'biebers' })

Adds some extra cruft so not sure if worth or not (might be more performant to just change the data going in before it is passed into the model). I just really don't like having the mixed casing going on.

ctdio · 2017-10-12T02:01:24Z

test/unit/dao/index-test.js

+
+test('should pass options through to FashionKnex', function (t) {
+  const { BaseDao, argsStub } = t.context
+  new BaseDao({ rin: 'okumura' })


… directory naming

ctdio

Aside from the whole mixed casing thing, this looks fine to me. I'm cool with that being discussed later on (preferrably sooner rather than later). But for now, I'm okay with merging this so that no one ends up getting blocked.

Druotic added 3 commits October 1, 2017 20:49

[WB-25] Handle installation events - no distinction between create/de…

0216f38

…lete yet

[WB-25] remove linking, add unsigned id

57c23e9

[WB-25] update lockfile

6ff2fbf

Druotic requested review from austinkelleher, Epitrochoid, mdaum and ctdio October 2, 2017 01:03

Druotic commented Oct 2, 2017

View reviewed changes

austinkelleher requested changes Oct 2, 2017

View reviewed changes

ctdio requested changes Oct 11, 2017

View reviewed changes

ctdio reviewed Oct 12, 2017

View reviewed changes

Druotic added 2 commits October 13, 2017 14:04

[WB-25] Remove extra require-self-refs, move createDatabase to utils,…

d8cf3a1

… directory naming

[WB-25] Simply exports

a081b75

ctdio approved these changes Oct 13, 2017

View reviewed changes

Druotic added 2 commits October 14, 2017 13:51

[WB-25] bump package-lock to pick up latest service util changes

b431ac8

[WB-25] Remove pre-migration custom table creation

a459c64

Druotic merged commit bc8cebc into master Oct 14, 2017

		@@ -0,0 +1,20 @@
		const startupTasks = require('~/src/startup-tasks')

[WB-25] Handle installation events #20

[WB-25] Handle installation events #20

Conversation

Druotic commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Druotic Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Druotic Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Druotic Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinkelleher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Druotic Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctdio left a comment

Choose a reason for hiding this comment

Druotic Oct 2, 2017 •

edited

Loading

Druotic Oct 2, 2017 •

edited

Loading

Druotic Oct 2, 2017 •

edited

Loading

Druotic Oct 13, 2017 •

edited

Loading