groovecoder / discord

GitHub webhook that analyzes pull requests and adds comments about incompatible CSS
Mozilla Public License 2.0
29 stars 13 forks source link

Fix #100: Measure installs with Postgres #147

Closed openjck closed 9 years ago

openjck commented 9 years ago

Reviewing

Definitely go commit-by-commit. It's not that bad, but lots of lines were changed when config.js was modified and lots of files were moved when tests were added.

Testing

Follow the updated instructions in TESTING.md.

How to spot-check

  1. Push this code up to your personal Heroku
  2. heroku addons:create heroku-postgresql:hobby-dev
  3. Delete your webhook from your test repository
  4. Re-add your webhook
  5. Verify that a ping was recorded
    1. heroku pg:psql
    2. SELECT * FROM "Pings";
openjck commented 9 years ago

@groovecoder: If it's not too much trouble, I would be super interested in what you think, too. :smile:

groovecoder commented 9 years ago

First review done. Some of them are big-ish things, but we can just discuss them and/or file follow-up issues for them; dont need to fix them all in this PR.

groovecoder commented 9 years ago

@darkwing - want to take a look at this?

openjck commented 9 years ago

Updated! Only comment not addressed is the tests.js callback hell. Thoughts @darkwing?

openjck commented 9 years ago

I think I'll add promises to sendHookPayload. That should do the trick.

openjck commented 9 years ago

Decided not to spend too much time on flattening the test out. Ready to go!

groovecoder commented 9 years ago

When I first pushed this up, I got:

groovecord::DATABASE=> select * from "Pings";
ERROR:  relation "Pings" does not exist
LINE 1: select * from "Pings";

Because the migrations don't seem to run automatically. FWIW, my web: ./gulp run process was idle during the deploy, and is still idle. When I hit https://groovecord.herokuapp.com/, the logs show that un-idling the web process ran the migration when it was needed. So maybe just something to be aware of for future deploys: manually run migrations via heroku run to be sure.

But, then I got the dreaded ambiguous Application Error of death and the first payload delivery failed. :cry: And I'm having trouble getting detailed log output from the web process?

openjck commented 9 years ago

Strange. I can't reproduce that. Is Postgres installed?

@mrmakeit or @darkwing, does the same thing happen when you follow the steps in the How to spot-check section of the pull request description?

openjck commented 9 years ago

Nevermind. I think I got it.

Luke, can you try again? You'll need to destroy and re-create the database when the web dyno is not running.

  1. heroku ps:scale web=0
  2. heroku addons:destroy heroku-postgresql:hobby-dev
  3. heroku addons:create heroku-postgresql:hobby-dev
  4. Push up the latest code
  5. heroku ps:scale web=1
groovecoder commented 9 years ago

WORKS on a fresh app.

groovecord2::DATABASE=> SELECT * FROM "Pings";
 id |           repo           |         createdAt          |         updatedAt
----+--------------------------+----------------------------+----------------------------
  1 | groovecoder/discord-test | 2015-08-22 13:13:23.615+00 | 2015-08-22 13:13:23.615+00
(1 row)

Filed https://github.com/mdn/discord/issues/151 as a follow-up.