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: webhook on call notification #3036

Merged
merged 19 commits into from
Jun 7, 2023

Conversation

andrewbenington
Copy link
Contributor

@andrewbenington andrewbenington commented May 22, 2023

  • 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 webooks as a possible destination for Schedule On-Call Notifications.

  • ScheduleOnCallNotificationsForm's value uses a channelFields sub-object instead of including slackChannel and slackUserGroup in the base object
    • Dropdown added to select type of channel (slack/webhook)
    • Slack Channel/User Group form fields moved to their own component, which is able to be swapped for the webhook field
  • On-call notifications sent to a webhook include a JSON body with the following
    • User (ID, URL, Name)
    • Schedule ID
    • Schedule Name
    • Schedule URL

Which issue(s) this PR fixes:
#2914

Describe any introduced user-facing changes:
Users can specify a webhook destination for Schedule On-Call Notifications if webhooks are enabled by an admin. Under a Schedule, the Quick Link for On-Call Notifications will now show up if webhooks are enabled and the chan-webhook experimental flag is set. When creating/editing an On-Call Notification rule, there is now a dropdown to select between Slack and a webhook, which will also display the appropriate form fields.

Describe any introduced API changes:
setScheduleOnCallNotificationRules can now accept a rule with a target of type "chanWebhook", if it includes an ID that conforms with an approved webhook URL.

UI Screenshots:
Screenshot 2023-06-07 at 1 05 39 PM
Screenshot 2023-06-07 at 1 06 04 PM

Makefile Outdated Show resolved Hide resolved
@andrewbenington andrewbenington changed the title Webhook on call notification feat: webhook on call notification May 22, 2023
@andrewbenington andrewbenington marked this pull request as ready for review May 22, 2023 20:56
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.

Small change for some backend code, ran out of time looking through the UI.

Since this PR contains UI changes, it would be helpful to add a screenshot or two to make sure everyone is on the same page for the design/layout

notification/webhook/sender.go Outdated Show resolved Hide resolved
@mastercactapus
Copy link
Member

Do you think we should mark this as a draft in the meantime? I think you mentioned this is blocked until the dropdown changes are made to the rule form.

No one is actively working on that for the moment, to my knowledge, though. I created a separate issue to track that specific change:

@andrewbenington
Copy link
Contributor Author

andrewbenington commented May 24, 2023

I mistakenly thought that the user groups PR was going to include the dropdown. This PR adds the dropdown, I was just worried it would conflict if there was another PR introducing one. Because the logic and components are already present here, I would say that this should come first and then a PR for #3045 would just separate user groups from channels. But it's up to you

@mastercactapus
Copy link
Member

@andrewbenington That does make sense, though that makes the scope of this PR bigger due to coupling the UI redesign with the new webhook notification option.

That said, I think it makes sense to do #3045 first because switching to the dropdown would best be done along with updating the user group behavior to match the new UI (since it is currently coupled with channels).

Doing so with this PR would only serve to make it even larger and harder to get through review.

@andrewbenington andrewbenington marked this pull request as draft May 24, 2023 16:21
@andrewbenington andrewbenington marked this pull request as ready for review June 5, 2023 21:14
@github-actions github-actions bot added size/m and removed size/xl labels Jun 5, 2023
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.

It looks like we're displaying the full URL, including sensitive parts:
image

In contact methods, we mask the password, strip query params, and truncate the path:
image
image

And escalation policies only show the hostname:
image

We should be consistent everywhere as a follow-up, but let's update this to follow one of the two existing methods.

@andrewbenington
Copy link
Contributor Author

It looks like we're displaying the full URL, including sensitive parts:

In contact methods, we mask the password, strip query params, and truncate the path:

And escalation policies only show the hostname:

We should be consistent everywhere as a follow-up, but let's update this to follow one of the two existing methods.

@mastercactapus I think it's good to have the full URL to differentiate between webhooks with the same hostname. The name is now set to the "FriendlyValue", but the full URL will still be visible on edit. Should this be hidden too?
Screenshot 2023-06-06 at 4 14 14 PM
The Webhook contact method disables editing the field, and masks the password (but not query params):
Screenshot 2023-06-06 at 4 19 46 PM

@mastercactapus
Copy link
Member

@andrewbenington It's mostly consistent with contact method behavior, and I'd consider changing that to be out-of-scope for this PR.

Mainly because the notification channel can be edited, and contact methods can not (since they also convey ownership). With it in experimental state, I'm okay with leaving it open for iteration in the future (e.g., maybe only show the FriendlyValue or allow replacing it entirely) but then consider how we can get EP and contact methods to all match

Copy link
Collaborator

@KatieMSB KatieMSB left a comment

Choose a reason for hiding this comment

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

lgtm! 👍

@andrewbenington andrewbenington merged commit 0809dee into master Jun 7, 2023
@andrewbenington andrewbenington deleted the webhook-on-call-notification branch June 7, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants