Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding pagerduty target #71

Merged
merged 4 commits into from
Oct 20, 2016
Merged

Adding pagerduty target #71

merged 4 commits into from
Oct 20, 2016

Conversation

nevins-b
Copy link
Contributor

@nevins-b nevins-b commented Oct 19, 2016

This adds the ability to create pagerduty incidents from alerts

@nevins-b nevins-b changed the title Adding pagerduty targer Adding pagerduty target Oct 19, 2016
Copy link
Contributor

@kiwiz kiwiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Can you resolve the comments and add a isAccessible() method like in the Jira Target: https://github.com/etsy/411/blob/master/phplib/Target/Jira.php#L22-L25

This hides the Target from the UI if it hasn't been configured.

class PagerDuty_Target extends Target {
public static $TYPE = 'pagerduty';

public static $DESC = 'Create a incident for this alert and assign to <assignee>.';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicky: "an incident"

$curl = new \Curl\Curl;
$curl->setHeader('Content-type', 'application/json');
$ret = $curl->post(
'https://events.pagerduty.com/generic/2010-04-15/create_event.json',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that... the api version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so? They don't call it out specifically in their API docs: https://v2.developer.pagerduty.com/v2/docs/trigger-events

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

json_encode($event_data)
);

if($curl->httpStatusCode < 200 || $curl->httpStatusCode >= 300) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the API return non 200 responses on success? If not, you should just check for 200 here.

);

if($curl->httpStatusCode < 200 || $curl->httpStatusCode >= 300) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the Jira Target to give better error info when a request fails: https://github.com/etsy/411/blob/master/phplib/Target/Jira.php#L83

@nevins-b
Copy link
Contributor Author

So I'm not sure what to do for the isAccessible() function. The only configuration that is required for this target is an API key which I've set up as configurable per search since it sends it to different escalation policies. So I guess just return true?

@kiwiz
Copy link
Contributor

kiwiz commented Oct 20, 2016

Do you mean the API token? https://v2.developer.pagerduty.com/docs/authentication
I don't see anything in the code code that would set that, just the service key.

@nevins-b
Copy link
Contributor Author

So when you create an event via the API (https://v2.developer.pagerduty.com/docs/trigger-events) you have to pass in a "service_key" which is associated with the service and escalation policy that's defined for your teams in PagerDuty, it basically acts as both authentication and internal routing. There's no additional api keys or tokens which are required to call their API.

@kiwiz
Copy link
Contributor

kiwiz commented Oct 20, 2016

Ah, I see. No need to implement isAccessible, then.

@kiwiz kiwiz merged commit 31f98a9 into etsy:master Oct 20, 2016
@kiwiz
Copy link
Contributor

kiwiz commented Oct 20, 2016

Merged, thanks! :]

@quintanilhar quintanilhar mentioned this pull request Sep 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants