New: Added Pagerduty Integration (fixes #37)

[vegetableman]

fixes #37

An overview of this PR:-

1. Supports live updating of incidents.
2. Post notes to pagerduty on adding comment.

Things left:

If a user exists in both tools, an acknowledgement in PagerDuty assigns it to that user in banjaxed.

There is no support for other users in the app currently. How to go about it?
and tests.

To run:-

1. rake db:migrate
2. Following env variables are used:- PAGERDUTY_API_KEY, PAGERDUTY_SUBDOMAIN, PAGERDUTY_REQUESTER_ID.
3. Register on pagerduty and use the set subdomain for PAGERDUTY_SUBDOMAIN.
4. Go to https://<subdomain>.pagerduty.com/api_keys and create the api key for PAGERDUTY_API_KEY.
5. Go to https://<subdomain>.pagerduty.com/users/<user_id>. Use <user_id> for PAGERDUTY_REQUESTER_ID.
6. Add the callback:- https://<app_domain>.herokuapp.com/pagerduty/callback to pagerduty for webhook.

[eugeneius]

Hey @vegetableman, thanks for working on this!

On the features you said are left to do:

• There's no concept of a Banjaxed incident being assigned to a user, so I think we can skip this feature. If we add assignment in the future, the PagerDuty integration can be updated to work with it.
• There is support for multiple users in the app already: every user that authenticates with GitHub will create a separate User model. Right now you're configuring a single PagerDuty user who will create all notes, but I think the note in PagerDuty should be created by the same user that wrote it in Banjaxed, and likewise when PagerDuty incidents are mirrored into Banjaxed. More on this below.
• Tests for the new functionality would be good once the implementation is nailed down.

I think we need a way to let users link their Banjaxed and PagerDuty users together. The simplest way would be a form where they enter their PagerDuty user ID, but maybe we could pull a list of users from the PagerDuty API and let the user choose which one is them. We could even try to match the email address from their GitHub account to the one in PagerDuty, but that could be added later.

Some feedback on the implementation:

• Could we use a gem to interact with the PagerDuty API instead of using net/http directly?
• The IncidentStack class looks like it will only work when the app is running on a single server, and only for a single user. If I refresh the incident list, I'll pop all of the items off the stack and no one else will see them. Could we just load new entries from the database like when we poll for comments?
• There's a merge conflict, looks like it's in the JavaScript and will be simple enough to resolve.

I have a few other minor thoughts; I'll add them as inline comments on the code.

[eugeneius]

The with: :exception option here is important; it changes how invalid requests are handled, which can have security implications - without it you can no longer distinguish between a logged out user and a CSRF attack, for example.

Could you revert this change, and just skip the protection in the controller where it's necessary? As documented here: http://api.rubyonrails.org/classes/ActionController/RequestForgeryProtection/ClassMethods.html#method-i-protect_from_forgery

[vegetableman]

@eugeneius Thanks for the detailed review. Have addressed most of the issues.

This shouldn't be hardcoded - what if User 1 has been deleted?. We should provide a way to create a user to represent PagerDuty, maybe with a rake task.

Please correct me if am wrong, an incident from pagerduty could be created by any program/user, it does not correspond to a single user. And the webhook api does not provide details on the user/program creating the incident. So, what could be Opened by in this case?
Reference:- https://developer.pagerduty.com/documentation/rest/webhooks

The simplest way would be a form where they enter their PagerDuty user ID,

Agree with the form approach.

Could we use a gem to interact with the PagerDuty API

No pagerduty gem supports posting of notes as of yet.

[vegetableman]

Added migrate to travis to fix build issues.

[vegetableman]

@eugeneius Could you review the changes?

[eugeneius]

Travis CI shouldn't need to perform database migrations; you should commit the new version of schema.rb that is generated when you run the db:migrate rake task.

You're right: PagerDuty incidents can be triggered in several ways, and sometimes there is no user involved - like when an automated alarm creates an incident, for example. I think there should be a user in the database named "PagerDuty", and this user should be the opener for all Banjaxed incidents created automatically by callbacks from PagerDuty.

Here's a rake task that does that. I got the data from the GitHub API response for the PagerDuty organisation: https://api.github.com/users/PagerDuty

namespace :pagerduty do
  desc 'Create a PagerDuty user in the database'
  task create_user: :environment do
    PagerDutyGithubUser = Struct.new(:id, :login, :name, :avatar_url)

    pagerduty_github_user = PagerDutyGithubUser.new(
      766800,
      'PagerDuty',
      '',
      'https://avatars.githubusercontent.com/u/766800?v=3'
    )

    User.create_or_update_from_github_user(pagerduty_github_user)
  end
end

When creating an incident from a PagerDuty callback, you could look this user up by its github_id and set it as the opener.

[eugeneius]

Would you mind splitting the AJAX updates for the incident page into its own pull request? I think it's a good idea, but a few things need work (it doesn't play nicely with the incident status filters, for example). The PagerDuty-specific parts of this are pretty close to being done, and it would be great to be able to merge them without having to wait for the AJAX stuff to be ready.

[vegetableman]

Updated. PR #50 for ajax related changes. Also, couldn't find any issues with status filters. Could you provide more details?

[vegetableman]

Is this ready to be merged?. Let me know if it's alright, I could write a few test cases.

[eugeneius]

This is related to the AJAX updating functionality, could you remove it here?

[eugeneius]

Sorry for the delay @vegetableman. I just made a few more small comments, but I think this is good to merge after that!

If you're going to write tests, I think the part that most needs coverage is the PagerDuty controller - that bug with the code still running even if the PagerDuty user doesn't exist would have been caught by a test, for example.

[eugeneius]

Making the id column of the pagerduty_incidents table a string makes sense, but unfortunately Rails doesn't fully support non-integer primary keys. I ran the migrations on my development database, and the table was created correctly:

$ echo "\d pagerduty_incidents" | bundle exec rails dbconsole
                                   Table "public.pagerduty_incidents"
   Column    |          Type          |                            Modifiers                             
-------------+------------------------+------------------------------------------------------------------
 id          | character varying(255) | not null default nextval('pagerduty_incidents_id_seq'::regclass)
 incident_id | integer                | 
Indexes:
    "pagerduty_incidents_pkey" PRIMARY KEY, btree (id)
    "index_pagerduty_incidents_on_incident_id" btree (incident_id)

However when I ran the db:test:prepare rake task to update my test schema, the id column ended up as an integer:

$ echo "\d pagerduty_incidents" | RAILS_ENV=test bundle exec rails dbconsole
                            Table "public.pagerduty_incidents"
   Column    |  Type   |                            Modifiers                             
-------------+---------+------------------------------------------------------------------
 id          | integer | not null default nextval('pagerduty_incidents_id_seq'::regclass)
 incident_id | integer | 
Indexes:
    "pagerduty_incidents_pkey" PRIMARY KEY, btree (id)
    "index_pagerduty_incidents_on_incident_id" btree (incident_id)

This will cause problems in tests, since string IDs won't be saved correctly. This will also happen when using db:schema:load to set up a new instance of the app.

I think you should leave the id column as an autoincrementing integer, and add something like pagerduty_id to store the ID of the incident in PagerDuty.

It looks like full support for non-integer primary keys will be added in Rails 5: rails/rails@3628025

[vegetableman]

@eugeneius Have updated the code with tests and refactored it a bit. Thanks again for the review and the detailed pointer on primary keys. sure learnt a lot.

[vegetableman]

Could this be merged?

[eugeneius]

This test passes, but the behaviour seems wrong. Shouldn't this be the ID of the incident that was created? https://github.com/vegetableman/banjaxed/blob/6fae9440b07489a61d715240b88dca4495d7af0e/app/services/pagerduty.rb#L17

[vegetableman]

Since, the data get's regenerated every time the spec runs. was assuming it to be 0. Pushed a fix 👍 .

[vegetableman]

hey @eugeneius , Sorry for troubling you. Would love to see this merged and make myself some bounty 😄 . Let me know if anything's left.

[eugeneius]

Thanks for sticking with this @vegetableman!

[vegetableman]

Thanks again @eugeneius. you were awesome !.