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

feat: adding webhooks as a notification channel type #2809

Merged
merged 49 commits into from
Feb 24, 2023

Conversation

nataliekorzh
Copy link
Contributor

  • Identified the issue which this PR solves.
  • Read the CONTRIBUTING document.
  • Code builds clean without any errors or warnings.
  • Added appropriate tests for any new functionality.
  • All new and existing tests passed.
  • Added comments in the code, where necessary.
  • Ran make check to catch common errors. Fixed any that came up.

Description:
Adds support for webhooks as a notification channel.

Which issue(s) this PR fixes:
Part of #2691
Splitting up #2774

Copy link
Member

@mastercactapus mastercactapus left a comment

Choose a reason for hiding this comment

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

As I'm going through this, I wonder if we should qualify Webhook everywhere to reduce ambiguity; nc-webhook and user-webhook vs. webhook on its own for either one.

Thoughts?

app/app.go Outdated Show resolved Hide resolved
app/startup.go Outdated Show resolved Hide resolved
alert/alertlog/store.go Outdated Show resolved Hide resolved
nataliekorzh and others added 2 commits February 6, 2023 14:41
Co-authored-by: Nathaniel Caza <mastercactapus@gmail.com>
@AlaricWhitney
Copy link
Collaborator

Since there will be more than one type of Webhook, we should ensure that there's a prefix to help everyone know what kind of webhook it is.

@nataliekorzh
Copy link
Contributor Author

As I'm going through this, I wonder if we should qualify Webhook everywhere to reduce ambiguity; nc-webhook and user-webhook vs. webhook on its own for either one.

Thoughts?

Just added the new chanWebhook type

graphql2/schema.graphql Outdated Show resolved Hide resolved
assignment/targettype.go Outdated Show resolved Hide resolved
assignment/targettype.go Outdated Show resolved Hide resolved
app/startup.go Outdated Show resolved Hide resolved
nataliekorzh and others added 6 commits February 10, 2023 12:38
Co-authored-by: Nathaniel Caza <mastercactapus@gmail.com>
Co-authored-by: Nathaniel Caza <mastercactapus@gmail.com>
Co-authored-by: Nathaniel Caza <mastercactapus@gmail.com>
Co-authored-by: Nathaniel Caza <mastercactapus@gmail.com>
@mastercactapus
Copy link
Member

Trying to validate this functionally, I'm not able to create a webhook channel via the API -- I'm attempting to do so via creating an EP target:

image

@nataliekorzh
Copy link
Contributor Author

nataliekorzh commented Feb 10, 2023

If I add the new escalation/store.go file included in the original PR (#2774), webhooks can be created successfully via the API. I was planning to include that file in a future PR where I set up the backend for adding a webhook to an escalation policy step. This PR was just for setting webhooks up as a new notification channel type. Do you think it would be better to include that file in this PR and allow for the functionality now?

@mastercactapus
Copy link
Member

I see; having it here would certainly help validate the API and DB changes -- is it possible to include just the minimal API <-> DB bits? If not, we can leave it out of scope.

@nataliekorzh
Copy link
Contributor Author

Sure, just added it.

escalation/store.go Outdated Show resolved Hide resolved
@mastercactapus mastercactapus merged commit e447cac into master Feb 24, 2023
@mastercactapus mastercactapus deleted the feat/webhook-notification-channel branch February 24, 2023 16:15
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.

5 participants