Adding ability to automatically close incident in PagerDuty #486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ra decided that alert is resolved(send OK status). This change conform incident lifecycle, that described in https://v2.developer.pagerduty.com/docs/send-an-event-events-api-v2
litleleprikon
suggested changes
Jan 16, 2020
litleleprikon
suggested changes
Jan 22, 2020
|
You have failing tests. Please fix it before we can merge your PR into master. CI log: |
|
@tantra35 Would you like to fix tests for this PR? |
|
@litleleprikon we will try |
|
@litleleprikon how we can launch |
|
@tantra35 which version of go are you using? |
|
@litleleprikon we use 1.13 |
|
@tantra35 I do not know why you are not able to run linter but my general advice will be to use docker container to run linter |
litleleprikon
suggested changes
Feb 26, 2020
| @@ -61,10 +62,16 @@ func (sender *Sender) buildEvent(events moira.NotificationEvents, contact moira. | |||
| Details: details, | |||
| } | |||
|
|
|||
| action := "trigger" | |||
There was a problem hiding this comment.
May we move this string literals to constants?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| @@ -43,6 +43,7 @@ func TestBuildEvent(t *testing.T) { | |||
| baseExpected := pagerduty.V2Event{ | |||
| RoutingKey: contact.Value, | |||
| Action: "trigger", | |||
| DedupKey: "TriggerID", | |||
There was a problem hiding this comment.
I would like to see more complex example of switching states between trigger and resolved in tests
There was a problem hiding this comment.
@litleleprikon Sorry but i can't imagine wthat you want:-), can you clarify? Dedupkey is simple trigger id, it doesn't add additional logic
There was a problem hiding this comment.
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
ERRORstate and action set totrigger - with trigger in
OKstate and action set toresolve - with trigger in
NODATAstate - with trigger in
WARNstate
In two last cases the action will be trigger but is it the right state?
litleleprikon
suggested changes
Mar 10, 2020
| @@ -61,10 +62,16 @@ func (sender *Sender) buildEvent(events moira.NotificationEvents, contact moira. | |||
| Details: details, | |||
| } | |||
|
|
|||
| action := "trigger" | |||
There was a problem hiding this comment.
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.
|
Hi @tantra35 this PR is almost ready to be merged into master. Would you like to rebase on a new master and fix issues? |
|
@tantra35 please complete this PR. Personally, we are really looking forward to the appearance of this PR in the master branch. Thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding ability to automatically close incident in PagerDuty when moira decided that alert is resolved(send OK status).
This change conform incident lifecycle, that described in https://v2.developer.pagerduty.com/docs/send-an-event-events-api-v2
For playing with api link below can be used:
https://v2.developer.pagerduty.com/docs/send-an-event-events-api-v2