lagotto / alm-report

ALM Reports
http://almreports.plos.org/
MIT License
8 stars 3 forks source link

Add Exception Notification gem #19

Closed mfenner closed 10 years ago

mfenner commented 10 years ago

This pull requests adds support for the exception notification gem. The primary intention is to use webhooks to collect errors in the ALM application, but this could also support other notification, e.g. email. Will extend the ALM API to process the webhooks.

Removed the check for internal IP addresses when displaying errors, as this is really difficult to maintain in an open source application - we rather want to process the errors separately.

Made changes to the alm_report cookbook to generate a slightly different settings file. We don't need the IP ranges anymore, but want to store login credentials for the ALM server. These are unfortunately breaking changes.

Added some basic routing tests.

There are still issues with properly loading the httparty library needed for webhook notifications, but that only affects user who have provided login credentials.

jure commented 10 years ago

Nice Martin. I'd rather if this wasn't clumped together with a lot of other things. Looks like this is basically four things: errors (exception notification), internal IP stuff (errors display), Chef/Vagrant updates and routing tests.

Looks like there's a breaking change to the API URL/API key code, which causes Travis to fail, as Webmock is not catching the request anymore, specifically, the API key is missing (even a mocked one), so the Regex doesn't match anymore:

Regex:

/http://alm.plos.org/api/v3/articles/\?api_key=.*&ids=10.1371/journal.pcbi.1002727/

URL:

http://alm.plos.org/api/v3/articles?api_key=&ids=10.1371/journal.pcbi.1002727

Not sure why Travis can't read the encrypted ENV['ALM_API_KEY'] variable, as that was working fine before. It's set here: https://github.com/mfenner/alm-report/blob/530890d46daaafa6d2075af393735111879b6cc0/.travis.yml#L10 Hm, really, no idea. Does setting the API key in settings.yml and then accessing it from APP_CONFIG['alm']['api_key'] work for you locally?

There's also a failing spec because of the IpRanges changes.

Also, what's the reasoning behind 1024 -> 512 MB RAM reduction? If anything I've beefed up my VM locally, so things are a bit speedier :)

Fix the code so specs pass, and I'll merge straight away.

mfenner commented 10 years ago

Sorry, indeed too much lumped together. More a feature branch with some extra stuff than a good pull request. The IP address belongs in there, as it is really about some users being able to see an error backtrace in production. The change in configuration sort of belongs in there because I need a new configuration setting, but again I agree that there is too much stuff for a pull request.

I made a small change in the URL, removing the / before the ?, that might break your test. And the IP ranges don't need to be tested anymore. Let me push a fix to those two.

The RAM reduction is because I am running both ALM and ALM Reports in separate VMs so that they can talk to each other, and my MacBook only has 8 GB of RAM. But I can make this change locally without committing.

mfenner commented 10 years ago

BTW, I had to fix two issues with the cookbooks somewhat related to this (but obviously not in the pull requests):

jure commented 10 years ago

OK, it hadn't actually crossed my mind that you can't see Travis' builds. I've contacted Travis' support and they added another 50 builds to our initial 100 (we, well I guess I, now have 60/150 left until the trial expires). Let me see if I can do something about that, so you can see the results too.

Looking good now, by the way: image

With regards to mysql_rails, I actually ran 'RAILS_ENV=development bundle exec rake db:setup' and created a test DB just fine yesterday, what issues were you seeing? Are there any security considerations here?

mfenner commented 10 years ago

Don't worry about Travis, this will be a public repo in a few days.

There is no RAILS_ENV=development bundle exec rake db:setup in the Chef recipe, as it is not idempotent, and I haven't yet figured out how to check for that. So I create the database in MySQL and then run db:migrate and db:seed. Not pretty, but safer.

The problem is that with the user created by the mysql_rails cookbook you couldn't do db:test:prepare or RAILS_ENV=development bundle exec rake db:setup, as the user only had permissions for alm-report_development.