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

Jump to bottom

[escalation] Single recipient per rule #58

Merged
merged 8 commits into from
Jul 25, 2024
Merged

[escalation] Single recipient per rule #58

merged 8 commits into from
Jul 25, 2024

Conversation

diraol
Copy link
Contributor

@diraol diraol commented Jun 12, 2024 •
edited
Loading

what

Rename recipients to recipient into Escalation rules.

And add the possibility to add multiple rules per escalation.

why

Opsgenie only accepts a single recipient per escalation rule.

Worst than that, if we send more than one recipient, OpsGenie API will accept the payload will add only the first recipient and will return 200.

references

opsgenie_escalation terraform module

@diraol diraol requested review from a team as code owners June 12, 2024 16:03
@diraol diraol requested review from johncblandii and nitrocode June 12, 2024 16:03
@mergify mergify bot added the triage Needs triage label Jun 12, 2024
Benbentwo
Benbentwo approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@Benbentwo Benbentwo left a comment

Choose a reason for hiding this comment

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

LGTM, the opsgenie api and docs can be a bit off - nice catch on the result if multiple recipients are sent

@Benbentwo
Copy link
Member

/terratest

@mergify mergify bot removed the triage Needs triage label Jul 22, 2024
Benbentwo
Benbentwo requested changes Jul 22, 2024
View reviewed changes
Copy link
Member

@Benbentwo Benbentwo left a comment

Choose a reason for hiding this comment

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

CleanShot 2024-07-22 at 09 25 53
Tests failing. looks like the submodule needs some changes

modules/escalation/main.tf Outdated Show resolved Hide resolved
@mergify mergify bot added the triage Needs triage label Jul 22, 2024
@Benbentwo
Copy link
Member

/terratest

@diraol diraol force-pushed the dro/update_escalation branch 2 times, most recently from f3e8ced to 3f2784d Compare July 23, 2024 17:44
@diraol
[escalation] Single recipient per rule
3479e92 
Opsgenie only accepts a single recipient per rule.
Worst than that, if we send more than one recipient, OpsGenie API will
accept the payload will add only the first recipient and will return
200.
@diraol diraol force-pushed the dro/update_escalation branch from 3f2784d to 3479e92 Compare July 23, 2024 17:56
Benbentwo
Benbentwo requested changes Jul 23, 2024
View reviewed changes
Copy link
Member

@Benbentwo Benbentwo left a comment

Choose a reason for hiding this comment

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

I think if we are going to make rules dynamic (which it should be) and an array we should do it across the board for consistency. lots of places should be updated to use rules instead of rule

modules/escalation/main.tf Outdated Show resolved Hide resolved
examples/escalation/main.tf Show resolved Hide resolved
modules/config/escalations.tf Show resolved Hide resolved
modules/config/README.md Show resolved Hide resolved
@diraol diraol force-pushed the dro/update_escalation branch 3 times, most recently from 9908e1d to 835049e Compare July 23, 2024 19:08
@diraol diraol force-pushed the dro/update_escalation branch from 835049e to 59ab19b Compare July 23, 2024 19:18
@Benbentwo
Copy link
Member

/terratest

@diraol diraol requested a review from Benbentwo July 25, 2024 15:46
@Benbentwo
Copy link
Member

/terratest

Benbentwo
Benbentwo requested changes Jul 25, 2024
View reviewed changes
examples/escalation/main.tf Show resolved Hide resolved
modules/escalation/main.tf Outdated Show resolved Hide resolved
@Benbentwo
Copy link
Member

Tests are currently failing because delay is non optional based on the missing try()

@Benbentwo
Copy link
Member

/terratest

@Benbentwo
Copy link
Member

/terratest

@Benbentwo
Copy link
Member

/terratest

@Benbentwo
Copy link
Member

This is now failling due to test collisions and not cleaning up, going to merge and fix tests in later PR

@Benbentwo Benbentwo enabled auto-merge (squash) July 25, 2024 18:22
Benbentwo
Benbentwo approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@Benbentwo Benbentwo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is now failing due to test collisions and not cleaning up, going to merge and fix tests in future PR

@mergify mergify bot removed the triage Needs triage label Jul 25, 2024
@Benbentwo Benbentwo merged commit f574bfe into cloudposse:main Jul 25, 2024
69 of 71 checks passed
@diraol diraol deleted the dro/update_escalation branch July 25, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Benbentwo Benbentwo Benbentwo approved these changes

@osterman osterman osterman approved these changes

@johncblandii johncblandii Awaiting requested review from johncblandii johncblandii is a code owner automatically assigned from cloudposse/contributors

@nitrocode nitrocode Awaiting requested review from nitrocode nitrocode is a code owner automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@diraol @Benbentwo @osterman