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

Add support for PagerDuty #527

Merged
merged 1 commit into from
Jun 27, 2023
Merged

Add support for PagerDuty #527

merged 1 commit into from
Jun 27, 2023

Conversation

MartiUK
Copy link
Contributor

@MartiUK MartiUK commented May 12, 2023

This PR adds support for PagerDuty.

There are some assumptions made in regards to what should trigger an incident, I'm happy to be corrected on this.

It would be nice to allow Alerts to override some Provider settings e.g. override the channel (integration key) to send alerts to different PagerDuty integrations without needed to create a Provider for each.

internal/notifier/pagerduty.go Show resolved Hide resolved
internal/notifier/pagerduty.go Outdated Show resolved Hide resolved
internal/notifier/pagerduty.go Outdated Show resolved Hide resolved
internal/notifier/pagerduty.go Outdated Show resolved Hide resolved
internal/notifier/pagerduty.go Outdated Show resolved Hide resolved
@Whisper40
Copy link

Thanks for this PR @MartiUK !

@MartiUK
Copy link
Contributor Author

MartiUK commented Jun 2, 2023

Hi @makkes, @stefanprodan is there anything else needed to progress this PR further?

Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

Just a couple of nits, otherwise lgtm

internal/notifier/pagerduty.go Outdated Show resolved Hide resolved
internal/notifier/pagerduty.go Outdated Show resolved Hide resolved
internal/notifier/pagerduty.go Outdated Show resolved Hide resolved
internal/notifier/pagerduty.go Outdated Show resolved Hide resolved
internal/notifier/pagerduty_test.go Outdated Show resolved Hide resolved
@makkes
Copy link
Member

makkes commented Jun 22, 2023

Please resolve the conflicts with main and squash the commits into one.

@MartiUK MartiUK force-pushed the pagerduty branch 3 times, most recently from 1bdba27 to 5fa67ce Compare June 22, 2023 12:07
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! 🥳

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

This looks great, some tiny nits only from me.

docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
@makkes
Copy link
Member

makkes commented Jun 22, 2023

Your generated CRD manifests don't match the code. Please run make manifests and commit the changes. Also please squash your commits.

@MartiUK MartiUK force-pushed the pagerduty branch 2 times, most recently from c029a27 to 22c1a39 Compare June 22, 2023 16:33
@stefanprodan stefanprodan added enhancement New feature or request area/alerting Alerting related issues and PRs labels Jun 22, 2023
@makkes makkes force-pushed the pagerduty branch 2 times, most recently from 34fca2e to 090038c Compare June 27, 2023 08:50
Signed-off-by: Martin Kemp <me@martinke.mp>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @MartiUK 🏅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Alerting related issues and PRs enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants