Add webhook modeling to projects #2764

agjohnson · 2017-03-30T19:30:50Z

This will add Integration models, which include just webhooks for now. This
extends the integrations page to list integrations, and allows for
per-integration syncing now as well. The brute force method of resyncing
webhooks will be replaced by this work.

See #2755 for more context on the the scope of this work. This also depends on #2745 and can be rebased after that is merged.

This work does not currently attempt to consolidate code, and there might be
some redundancy. The next phase of this work should be to consolidate the oauth
representations and remote modeling into a single place.

New features
Tests
Docs

agjohnson · 2017-04-21T18:43:57Z

There are a few places I noted required some more testing, and I owe docs here as well. This will require rebasing on master after #2745 is merged for better display.

ericholscher

Looks like a great change. Mostly just have some nits and questions.

ericholscher · 2017-04-28T22:02:34Z

docs/webhooks.rst

-updated within seconds is an awesome feeling.
+The primary method that Read the Docs uses to detect changes to your
+documentation is through the use of *webhooks*. Webhooks are configured with
+your repository provider and with each commit, merge, or other change to your


repository provider (like GitHub)?

ericholscher · 2017-04-28T22:03:12Z

docs/webhooks.rst

+The primary method that Read the Docs uses to detect changes to your
+documentation is through the use of *webhooks*. Webhooks are configured with
+your repository provider and with each commit, merge, or other change to your
+repository, Read the Docs is notified. When this happens, we determine if the


s/When this happens/When we recieve the webhook/

ericholscher · 2017-04-28T22:03:26Z

docs/webhooks.rst

+Webhook creation
+----------------
+
+If you import a project using a connected account, a webhook will be set up


connected account?

ericholscher · 2017-04-28T22:04:44Z

docs/webhooks.rst

+can click **Add integration** and select the integration type you'd like to add.
+In order to set up a new webhook with your provider, you'll need the *webhook
+URL*. You can get this URL by selecting the integration you just added on the
+list of integrations.


You can get this URL by selecting the integration you just added on the list of integrations. sounds confusing, and could be more explicit. Should we be returning them to the detail page, instead of the listing, when they add a new integration?

Perhaps, yeah. There's no good reason not to redirect the user there.

Added redirect in 8eb590d

ericholscher · 2017-04-28T22:05:52Z

docs/webhooks.rst

+integrations, this integration has a specific URL, found on the integration
+detail page. This integration can either be added manually or may be created
+automatically if you had previously set up a webhook with your provider
+manually.


This integration can either be added manually or may be created automatically if you had previously set up a webhook with your provider manually. Not sure what this is saying. It I previously set it up manually its now automatic?

If you previously created a webhook manually, it will automatically create an integration when we receive data. I can clarify here.

ericholscher · 2017-04-28T22:10:51Z

readthedocs/core/fields.py

+
+def default_token():
+    """Generate default value for token field"""
+    return ''.join([int_to_base36(randrange(sys.maxint)) for _ in range(2)])


Does this need to be cryptographically secure, or is it just a hash? I feel like Django has a way to generate these that can't be guessed.

Either way, we should document the thread profile of it.

This isn't a hash of any data in particular, so cryptographic security here isn't a concern. Randomization is the primary concern instead.

This was brought in from our auth token implementation, I'm not aware of a built-in token in Django. DRF uses this as a token generator:
https://github.com/encode/django-rest-framework/blob/master/rest_framework/authtoken/models.py#L37-L38

It might be best to replace this with the DRF version, as randrange(sys.maxint) has about 1:1000000000000000 chance of being 4 chars or less, or with concatenation of two of these strings, something like 1:1e-30 chance of being 8 characters or less.

ericholscher · 2017-04-28T22:14:36Z

readthedocs/integrations/models.py

+    GITHUB_WEBHOOK = 'github_webhook'
+    BITBUCKET_WEBHOOK = 'bitbucket_webhook'
+    GITLAB_WEBHOOK = 'gitlab_webhook'
+    API_WEBHOOK = 'api_webhook'


Should these be defined in a constants.py file, instead of on the class directly?

Django docs have recommended on the model class for these constants:
https://docs.djangoproject.com/en/1.11/ref/models/fields/#django.db.models.Field.choices

ericholscher · 2017-04-28T22:17:41Z

readthedocs/oauth/services/bitbucket.py

+                      project)
+            try:
+                log.debug('Bitbucket webhook update failure response: %s',
+                          resp.json())


Pretty sure I had a patch that removed the json() call because it blows up on non-JSON data. We should comment here if we don't want to capture non-JSON return data.

Yeah, i commented on the last PR that I was fixing this way.

ericholscher · 2017-04-28T22:23:30Z

readthedocs/restapi/views/integrations.py

+        that a webhook has ever been created on our side. Most providers don't
+        pass the webhook ID in either, so we default to just finding *any*
+        integration from the provider. This is not ideal, but the
+        :py:class:`WebhookView` view solves this by perfomring a lookup on the


ericholscher · 2017-04-28T22:26:01Z

readthedocs/restapi/views/integrations.py

+        token = self.request.data.get('token')
+        if token:
+            integration = self.get_integration()
+            obj = Project.objects.get(**kwargs)


Do we want to catch a DoesNotExist here?

No, this is currently bubbling up to be caught in the calling function

…e permissive

agjohnson · 2017-05-09T01:53:52Z

This should be about set now. If the new changes look okay, we should get this merged and out this week.

agjohnson added the PR: work in progress Pull request is not ready for full review label Mar 30, 2017

agjohnson force-pushed the project-integration-webhook branch 2 times, most recently from 24f800f to 841c4d5 Compare April 21, 2017 18:42

agjohnson added 2 commits April 25, 2017 13:32

Add start of work to add webhook modeling for each integration

1783145

Lint cleanup

5359231

agjohnson force-pushed the project-integration-webhook branch from 77fbfe6 to 5359231 Compare April 25, 2017 20:32

agjohnson added 2 commits April 26, 2017 13:49

Add authentication to generic webhook

9a7abf5

Add tests, docs, and lint fixes

12a244b

agjohnson requested a review from ericholscher April 27, 2017 06:19

agjohnson added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Apr 27, 2017

agjohnson added this to the Webhook admin dashboard milestone Apr 27, 2017

Fix bitbucket operations for webhook create/update

dcf9fe0

ericholscher reviewed Apr 28, 2017

View reviewed changes

agjohnson added 4 commits May 3, 2017 17:32

Doc updates

978c2c3

Update integration token generator and make service debug logging mor…

e376b2f

…e permissive

Redirect user to integration detail page on create

8eb590d

Add information for details page, drop automatic webhook mention

b7c03b7

agjohnson merged commit a66bd56 into master May 10, 2017

agjohnson deleted the project-integration-webhook branch May 10, 2017 21:01

agjohnson removed the PR: ready for review label May 10, 2017

safwanrahman mentioned this pull request Feb 15, 2018

Add missing migrations #3608

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add webhook modeling to projects #2764

Add webhook modeling to projects #2764

agjohnson commented Mar 30, 2017 •

edited

Loading

agjohnson commented Apr 21, 2017

ericholscher left a comment

ericholscher Apr 28, 2017

ericholscher Apr 28, 2017

ericholscher Apr 28, 2017

ericholscher Apr 28, 2017

agjohnson May 3, 2017

agjohnson May 4, 2017

ericholscher Apr 28, 2017

agjohnson May 4, 2017

ericholscher Apr 28, 2017

agjohnson May 2, 2017

ericholscher Apr 28, 2017

agjohnson May 2, 2017

ericholscher Apr 28, 2017

agjohnson May 2, 2017

ericholscher Apr 28, 2017

ericholscher Apr 28, 2017

agjohnson May 2, 2017

agjohnson commented May 9, 2017

Add webhook modeling to projects #2764

Add webhook modeling to projects #2764

Conversation

agjohnson commented Mar 30, 2017 • edited Loading

agjohnson commented Apr 21, 2017

ericholscher left a 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

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

agjohnson commented May 9, 2017

agjohnson commented Mar 30, 2017 •

edited

Loading