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.

Sign up for GitHub

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 ability to automatically close incident in PagerDuty #486

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Adding ability to automatically close incident in PagerDuty #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ab9493
Adding ability to automatically close incident in PagerDuty, when moi…
tantra35 Jan 16, 2020
0a94442
remove debuging code
tantra35 Jan 16, 2020
c1beff2
remove unused variable res
tantra35 Jan 17, 2020
3a539dd
Modify code according comments in PR
tantra35 Jan 17, 2020
7c230df
remove unnecessary empty line
tantra35 Jan 17, 2020
a6bba55
rename variable name lAction to action, so it possible to pass CI
tantra35 Jan 23, 2020
086e388
modify test for pagerduty sender
tantra35 Feb 18, 2020
093c90d
Merge branch 'master' into pagerduty-autoresolve
tantra35 Feb 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion senders/pagerduty/send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ func (sender *Sender) SendEvents(events moira.NotificationEvents, contact moira.
if err != nil {
return fmt.Errorf("failed to post the event to the pagerduty contact %s : %s. ", contact.Value, err)
}

return nil
}

Expand Down Expand Up @@ -61,10 +62,16 @@ func (sender *Sender) buildEvent(events moira.NotificationEvents, contact moira.
Details: details,
}

action := "trigger"
Copy link
Contributor

Choose a reason for hiding this comment

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

May we move this string literals to constants?

Copy link
Author

Choose a reason for hiding this comment

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

Hm, I doesn't understand, why this needed? In official library which you use (https://github.com/PagerDuty/go-pagerduty), those doesn't defined as constants, so it's very strange to invent abstractions if the authors of official library did not provide them themselves

Copy link
Contributor

Choose a reason for hiding this comment

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

I just do not like string literals and magic numbers placed in random places in the code. The reason to place this values in constants is that it will be easier to change values in one place if pagerduty will change their API. I know that right now this values are placed only in two places but in future we can have more complex condition of action choose.

if events.GetSubjectState() == moira.StateOK {
action = "resolve"
}

event := pagerduty.V2Event{
RoutingKey: contact.Value,
Action: "trigger",
Action: action,
Payload: payload,
DedupKey: trigger.ID,
}

if len(plot) > 0 && sender.imageStoreConfigured {
Expand Down
1 change: 1 addition & 0 deletions senders/pagerduty/send_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func TestBuildEvent(t *testing.T) {
baseExpected := pagerduty.V2Event{
RoutingKey: contact.Value,
Action: "trigger",
DedupKey: "TriggerID",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see more complex example of switching states between trigger and resolved in tests

Copy link
Author

Choose a reason for hiding this comment

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

@litleleprikon Sorry but i can't imagine wthat you want:-), can you clarify? Dedupkey is simple trigger id, it doesn't add additional logic

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not about the DeduceKey it's about whole tests I just found a first change in tests file to add my comment. My point is that we want to see that action set correctly and this lead us to this additional tests:

  • with trigger in ERROR state and action set to trigger
  • with trigger in OK state and action set to resolve
  • with trigger in NODATA state
  • with trigger in WARN state

In two last cases the action will be trigger but is it the right state?

Payload: &pagerduty.V2Payload{
Summary: "NODATA Trigger Name [tag1][tag2]",
Severity: "warning",
Expand Down