Support for generic webhooks #8522

stsewd · 2021-09-22T21:33:52Z

Currently we support webhooks to notify external services when a build starts/fails. The problem with this is that most of the time users want a quick integration with slack or whatever, but those services require you to send a payload with a determined format (usually just a json body). So, in order to have that, users need to implement a proxy between our webhooks and the service they want to use. In order to provide more flexibility and easy to integrate webhooks I'm reworking our current implementation like so:

Users can define the format of the payload we are going to send on each event (we do this by providing some placeholders)
Users can select to what events their webhook is subscribed to.
We provide a signature of the payload for users to verify the precedence of the webhook.
Webhooks responses (aka http exchanges) are saved for each webhook, so is more easy to debug.

To make it more easy, we could document or suggest some "quick templates" for users to use, like a nice notification for slack or discord and some step by step tutorials https://api.slack.com/messaging/webhooks https://support.discord.com/hc/en-us/articles/228383668-Intro-to-Webhooks

Moving webhooks to their own page

Currently, we have webhooks under the "Notifications" tab,
there they are mixed with email notifications.

In order to have a better UI, I have moved the webhooks to its own page instead,
and renamed the other section as "Email notifications".

And to avoid having two pages to manage the webhooks,
the old and new webhooks can be accessed from the same page.

I have added some checks to see if the user has old webhooks and show a note
that those have been moved to the other page.

Old webhooks don't have a custom payload, but instead will be using the defined payload from https://docs.readthedocs.io/en/stable/guides/build-notifications.html#using-webhook.
Users can migrate to a custom payload by just editing the webhook,
we don't support creating webhooks from the old implementation.

Screenshots

Some confusing documentation...

So, in our docs we have a webhooks page https://docs.readthedocs.io/en/stable/webhooks.html,
but they are about the webhook we provide in order for vcs services to notify us when a new branch/tag is created or when new changes are pushed so we can build the docs for those. We actually refer to those as "integrations" rather than webhooks in our codebase/docs.

The actual webhooks docs are in a page called "build notifications"
https://docs.readthedocs.io/en/stable/guides/build-notifications.html.
Which make sense, but we could expand our webhooks to listen to more events outside builds.

Future improvements

Allow to mark a webhook as active/inactive, this is useful to test things
without having to delete the webhook in order to stop sending events to it.
Allow to hit the webhooks when building from an external version (aka from pull requests).
Right now we don't send notifications on builds from external versions,
doing this by default could be too spammy.
Allow filtering by version name/privacy-level before sending a webhook.
Currently, we send webhooks for any version on the subscribed events
(but I do see having people only want to be notified about changes on master for example).
Users could implement a proxy to filter those, or we could implement a filter in our side so they don't need to maintain that.
More events: when a version is created/deleted on rtd?

Closes #6942

humitos

Looks like a good first approach to me. I did a non-complete review for now and I left some comments.

readthedocs/projects/models.py

readthedocs/templates/projects/project_edit_base.html

readthedocs/builds/tasks.py

readthedocs/projects/migrations/0082_init_generic_webhooks.py

readthedocs/projects/models.py

humitos · 2021-09-23T08:56:33Z

readthedocs/projects/tasks.py

        if not self.version or self.version.type != EXTERNAL:
-            send_notifications.delay(version_pk, build_pk=build_pk, email=email)
+            build_tasks.send_build_notifications.delay(


Actually, we may want to add this as a config for the Webhook. Like a checkbox for Send notifications on PRs so the user can decide whether or not to send them instead of us deciding by them.

readthedocs/builds/tasks.py

astrojuanlu · 2021-09-28T08:24:24Z

Does this solve #8137 and #8364?

stsewd · 2021-09-28T15:34:15Z

Does this solve #8137 and #8364?

Nope, those are vcs integrations. Integrations send a payload to our webhook (rtd.org/webhook/), meantime in webhooks, we send a payload to the given url.

stsewd · 2021-09-28T20:04:43Z

readthedocs/builds/tasks.py

+            HttpExchange.objects.from_requests_exchange(
+                response=response,
+                related_object=webhook,
+            )


We could create this object in two steps, before sending the request and updating it after the request was made. This is so we save the exchange even if the request failed (like timeout).

stsewd · 2021-09-28T20:08:27Z

readthedocs/projects/forms.py

+        model = WebHook
+        fields = ['project', 'url', 'events', 'payload', 'secret']
+        widgets = {
+            'events': forms.CheckboxSelectMultiple,


I think this is better than the list, and we don't have that many events. I wanted to show something similar to what github does (the name and the description)

but looks like we would need to just manually do that, there isn't a widget that does it for us :/

readthedocs/projects/models.py

stsewd · 2021-09-28T20:10:37Z

readthedocs/projects/models.py

+            '${event}': event,
+            '${build.id}': build.id,
+            '${build.commit}': build.commit or '',
+            '${organization.name}': organization_name,
+            '${organization.slug}': organization_slug,
+            '${project.slug}': project.slug,
+            '${project.name}': project.name,
+            '${version.slug}': version.slug,
+            '${version.name}': version.verbose_name,


don't know what others substitutions would be useful

URLs: project, version, build on Read the Docs and, maybe on VCS

added some urls, but most of that information can be built using the pk or slug of the models.

readthedocs/templates/projects/project_edit_base.html

stsewd · 2021-09-28T20:14:54Z

readthedocs/templates/projects/project_notifications.html

  </p>

+  {% if has_old_webhooks %}
+  <p class="help_text">


These could have a "warning" class or something more prominent, but don't think we are using a class like that in somewhere else.

stsewd · 2021-09-28T20:15:38Z

readthedocs/templates/projects/webhook_exchange_detail.html

@@ -0,0 +1,39 @@
+{% extends "projects/project_edit_base.html" %}


this is mostly copy/paste from https://github.com/readthedocs/readthedocs.org/blob/webhooks/readthedocs/templates/projects/integration_exchange_detail.html

Is there a way to share the same file instead of copy/paste?

The body could be moved to another file. But I do see this having more options (like retry the webhook), so don't know.

stsewd · 2021-09-28T20:16:05Z

readthedocs/templates/projects/webhook_form.html

+
+  {# If the webhook was created from the old implementation, it doesn't have a custom payload. #}
+  {% if object.pk and not object.payload %}
+  <p class="help_text">


same here, could be a warning class or something

stsewd · 2021-09-28T21:10:24Z

Think this should be ready for review, only docs are missing.

readthedocs/templates/projects/webhook_list.html

stsewd · 2021-09-29T18:32:33Z

readthedocs/projects/models.py

+        substitutions = {
+            '${event}': event,
+            '${build.id}': build.id,
+            '${build.commit}': build.commit or '',


was thinking we could have ${build.commit.short} to display only the 7/8 first chars of the commit.

humitos · 2021-10-04T08:47:58Z

readthedocs/builds/tasks.py

+    def __init__(self, version, build, event):
+        self.version = version
+        self.build = build
+        self.project = version.project


Why this requires version and build? It should be enough just with the build.

The old code was sending the build and version pk separately, don't know if there is a case where a build doesn't have the version attached.

Is it still required with the new code after the refactor? Can we remove it?

I have no idea why the old code required it, so not sure about changing that on this PR.

readthedocs/builds/tasks.py

readthedocs/projects/models.py

humitos

First of all, the PR's description looks awesome! 💯 --It helped me a lot to understand all the changes.

I'd say that all the future improvements could be an issue/issues. Besides, together with those improvements, I think we will also want a way to send extra HTTP headers together with a custom body.

The PR looks great! The only thing I'd like to change before merging, is the substitution syntax (${variable}) to use the Django Template syntax instead ({{ variable }}) which is more known.

humitos · 2021-10-04T09:14:00Z

readthedocs/templates/projects/webhook_exchange_detail.html

@@ -0,0 +1,39 @@
+{% extends "projects/project_edit_base.html" %}


Is there a way to share the same file instead of copy/paste?

readthedocs/templates/projects/webhook_list.html

readthedocs/projects/forms.py

readthedocs/projects/migrations/0084_create_webhook_events.py

readthedocs/projects/models.py

humitos · 2021-10-04T09:26:04Z

readthedocs/projects/models.py

+            '${event}': event,
+            '${build.id}': build.id,
+            '${build.commit}': build.commit or '',
+            '${organization.name}': organization_name,
+            '${organization.slug}': organization_slug,
+            '${project.slug}': project.slug,
+            '${project.name}': project.name,
+            '${version.slug}': version.slug,
+            '${version.name}': version.verbose_name,


URLs: project, version, build on Read the Docs and, maybe on VCS

readthedocs/projects/urls/private.py

stsewd · 2021-10-26T15:42:47Z

Yep, I struggled with GitHub notifications on this one. In any case, wdyt of #8522 (comment) ?

that can be done with some duplication, this is creating a webhook for each event just changing the payload. But, I think the most common case for slack notifications is failed builds.

ericholscher · 2021-10-27T20:32:45Z

I feel like we've been talking about this for a long time. I'd like to propose the following path forward:

Use {{}} for the variables, so we can close that conversation. It's still replacement only, and we move on from that conversation
We ship this with the slack & discord examples in docs
We work on another PR that adds better support for common extensions (probably Slack to start), which we control the format of

I feel like this is the path forward, but this PR has gotten overwhelming, and we should just get this baseline feature merged.

stsewd · 2021-10-28T14:45:23Z

Use {{}} for the variables, so we can close that conversation. It's still replacement only, and we move on from that conversation

Is that with one space around the name? Or is it using the template engine from django to support all variations?

ericholscher · 2021-10-28T19:46:18Z

Is that with one space around the name? Or is it using the template engine from django to support all variations?

1 or 0 spaces. Should be simple enough to implement (eg. just loop over the list of replacements twice).

$foo -> {{ foo }}

stsewd · 2021-11-02T16:19:39Z

Changed to allow {{ foo }} or {{foo}} (I still think if we go for {{ foo }} we should support just one variation), start_date is still iso-formated without microseconds and tzinfo (2021-10-25T16:02:25)

ericholscher

Let's move this forward 👍

readthedocs/projects/models.py

Co-authored-by: Eric Holscher <[email protected]>

Co-authored-by: Santos Gallegos <[email protected]>

Document generic webhooks

stsewd force-pushed the webhooks branch from 9813c74 to 57858b5 Compare September 22, 2021 23:49

humitos reviewed Sep 23, 2021

View reviewed changes

stsewd force-pushed the webhooks branch 6 times, most recently from c93065a to fd27634 Compare September 28, 2021 01:24

stsewd commented Sep 28, 2021

View reviewed changes

readthedocs/builds/tasks.py Outdated Show resolved Hide resolved

stsewd force-pushed the webhooks branch from fd27634 to b8561dd Compare September 28, 2021 01:34

stsewd force-pushed the webhooks branch 3 times, most recently from 2d05db6 to 9843252 Compare September 28, 2021 19:59

stsewd commented Sep 28, 2021

View reviewed changes

stsewd force-pushed the webhooks branch from 9843252 to 25f8543 Compare September 28, 2021 20:16

Support for generic webhooks

12cd38b

stsewd force-pushed the webhooks branch from 25f8543 to 12cd38b Compare September 28, 2021 21:01

stsewd marked this pull request as ready for review September 28, 2021 21:10

stsewd commented Sep 28, 2021

View reviewed changes

readthedocs/templates/projects/webhook_list.html Outdated Show resolved Hide resolved

stsewd commented Sep 29, 2021

View reviewed changes

stsewd added 4 commits September 29, 2021 13:59

Rename unsub_url -> unsubscribe_url

ed703fd

Merge branch 'master' into webhooks

2584afd

Update migrations

224622e

Fix merge conflict

c531286

stsewd requested a review from a team September 29, 2021 20:54

humitos reviewed Oct 4, 2021

View reviewed changes

stsewd added 2 commits November 2, 2021 10:19

Merge branch 'master' into webhooks

e4ea22e

Change substitutions format again

2b16a86

$foo -> {{ foo }}

astrojuanlu added 5 commits November 8, 2021 16:08

Move payload validation down

33f9557

Update template syntax

1077d61

Merge branch 'webhooks' into webhooks-documentation

481380d

Complete webhook example payload

19eec46

Add screenshot for custom payload

088e629

ericholscher approved these changes Nov 8, 2021

View reviewed changes

readthedocs/projects/models.py Outdated Show resolved Hide resolved

astrojuanlu and others added 10 commits November 8, 2021 19:17

Style improvements

2a0a031

Co-authored-by: Eric Holscher <[email protected]>

Comment

a26bd54

Merge branch 'master' into webhooks

a2b6a2a

Switch to one space for template substitutions

ec9b4fc

Add examples for URLs

b049047

Fix parameter name

94f9fc1

Complete remaining variables

a928097

Fix title length

8f20472

Fix JSON

04e2953

Co-authored-by: Santos Gallegos <[email protected]>

Merge pull request #8609 from readthedocs/webhooks-documentation

d1a21f0

Document generic webhooks

astrojuanlu requested a review from a team as a code owner November 8, 2021 22:23

astrojuanlu removed the request for review from a team November 8, 2021 22:25

stsewd added 3 commits November 8, 2021 17:33

Small updates to docs

b6dd115

Merge branch 'master' into webhooks

732ccdd

Explicitly test with/out organizations

d2611b4

stsewd merged commit d4f6873 into master Nov 9, 2021

stsewd deleted the webhooks branch November 9, 2021 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for generic webhooks #8522

Support for generic webhooks #8522

stsewd commented Sep 22, 2021 •

edited

Loading

humitos left a comment

humitos Sep 23, 2021

astrojuanlu commented Sep 28, 2021

stsewd commented Sep 28, 2021

stsewd Sep 28, 2021

stsewd Sep 28, 2021

stsewd Sep 28, 2021

humitos Oct 4, 2021

stsewd Oct 4, 2021

stsewd Sep 28, 2021

stsewd Sep 28, 2021

humitos Oct 4, 2021

stsewd Oct 4, 2021

stsewd Sep 28, 2021

stsewd commented Sep 28, 2021

stsewd Sep 29, 2021

humitos Oct 4, 2021

stsewd Oct 4, 2021

humitos Oct 5, 2021

stsewd Oct 5, 2021

humitos left a comment

humitos Oct 4, 2021

humitos Oct 4, 2021

stsewd commented Oct 26, 2021

ericholscher commented Oct 27, 2021

stsewd commented Oct 28, 2021

ericholscher commented Oct 28, 2021

stsewd commented Nov 2, 2021

ericholscher left a comment

		@@ -0,0 +1,39 @@
		{% extends "projects/project_edit_base.html" %}

Support for generic webhooks #8522

Support for generic webhooks #8522

Conversation

stsewd commented Sep 22, 2021 • edited Loading

Moving webhooks to their own page

Screenshots

Some confusing documentation...

Future improvements

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrojuanlu commented Sep 28, 2021

stsewd commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Oct 26, 2021

ericholscher commented Oct 27, 2021

stsewd commented Oct 28, 2021

ericholscher commented Oct 28, 2021

stsewd commented Nov 2, 2021

ericholscher left a comment

Choose a reason for hiding this comment

stsewd commented Sep 22, 2021 •

edited

Loading