Refactor webhooks, add modern webhooks, and fix issues with Bitbucket #2433

agjohnson · 2016-10-02T08:01:05Z

Adds new API endpoint for webhook consumption
Adds webhook signals for future use
Cleans up old webhook implementations
Fixes old Bitbucket webhook implementation
Sets new webhooks to point to API endpoint
Removes GitHub service creation by naming the webhook 'web' -- 'readthedocs' created
an RTD service on GitHub
Updates Gitlab fixture data with example from API
Drops request.POST['payload'] from all old webhooks, not sure what it was
Made tests actually send JSON to webhook views

ericholscher

This is a great change, not having to map incoming pushes to projects. It will definitely make stuff easier going forward.

We should also update the "webhook URL" thats output on the project page, and update the documentation here as well.

ericholscher · 2016-10-04T21:36:21Z

readthedocs/core/views/hooks.py

    if request.method == 'POST':
        try:
-            # GitHub RTD integration
-            obj = json.loads(request.POST['payload'])


This is required for old GitHub hooks that POST'd with form data, instead of a straight JSON post. I believe we need to keep it around.

Roger, I suppose it's possible we have some old webhook configurations lingering then.

Actually, that would mean GitHub was sending the payload as JSON encoded in the payload field? I don't see any signs that this was the case.

Ah! nevermind, found it and you are correct on the usage here:
https://developer.github.com/webhooks/creating/#content-type

ericholscher · 2016-10-04T21:38:07Z

readthedocs/core/views/hooks.py

    else:
-        return HttpResponse("You must POST to this resource.")
+        return HttpResponse('Method not allowed', status=405)


Seems less explicit than telling folks they need to POST.

ericholscher · 2016-10-04T21:40:01Z

readthedocs/core/views/hooks.py

    if request.method == 'POST':
-        payload = request.POST.get('payload')


Think this is similarly required for old BB API's no? We're depending on request.body being JSON here, but presumably this code was working previously, so some BB requests were using this method?

This is where I'm more confused. I think that may have been the case with old POST services from bitbucket, but webhooks don't deliver x-www-form-urlencoded bodies?

Most I found was here:
https://confluence.atlassian.com/bitbucket/post-service-management-223216518.html

You're probably right in that we need to keep this here too.

ericholscher · 2016-10-04T21:42:32Z

readthedocs/restapi/views/integrations.py

+            branches = [request.data['ref'].replace('refs/heads/', '')]
+        except KeyError:
+            raise ParseError('Parameter "ref" is required')
+        if event == GITHUB_PUSH:


We should be logging when we get other events.

ericholscher · 2016-10-04T21:43:45Z

readthedocs/restapi/views/integrations.py

+        except KeyError:
+            raise ParseError('Parameter "ref" is required')
+        if event == GITHUB_PUSH:
+            to_build, not_building = build_branches(project, branches)


We should probably not throw away the not_building list here -- would be useful to log if nothing else.

ericholscher · 2016-10-04T21:44:21Z

readthedocs/restapi/views/integrations.py

+            return {'build_triggered': triggered,
+                    'project': project.slug,
+                    'versions': to_build}
+        return {'build_triggered': False}


This code feels like it could be abstracted, and it's the same for GH/BB except for a couple lines.

Yeah, I debated this, it felt odd both ways. I'll clean this up somehow.

agjohnson · 2016-10-05T07:00:00Z

Addressed this feedback in latest push, needs quick re-review.

I didn't add info to the project detail page -- this doesn't exist on that page and is the wrong page to add information like this anyways. I feel like what we want is a webhook admin dashboard page instead. This page should push users to connect their account to set up a webhook, it should contain the hidden resync webhook form, and a fallback to manual set up could list the custom webhook URLs for manual set up. I'll create an issue to address this later.

I didn't update the docs because of this, it's hard to communicate the webhook changes in docs. If we had a webhook admin page, it would supersede these docs anyways. The current docs aren't incorrect, so there's that at least.

ericholscher

Looks good. I imagine it wasn't really possible to test the Post-commit changes, so I'm a bit weary about that, so we should make sure that doesn't blow up on deploy.

agjohnson · 2016-10-05T21:36:38Z

I did do some testing with ngrok and fresh webhooks -- but yeah, can't feasibly test old webhooks or service pushes. Hopefully our test data is good enough coverage here. We'll need to plan to watch this deploy.

I'll rebase and merge this.

* Adds new API endpoint for webhook consumption * Adds webhook signals for future use * Cleans up old webhook implementations * Fixes old Bitbucket webhook implementation * Sets new webhooks to point to API endpoint * Removes GitHub service creation by naming the webhook 'web' -- 'readthedocs' created an RTD service on GitHub * Updates Gitlab fixture data with example from API * Drops `request.POST['payload']` from all old webhooks, not sure what it was * Made tests actually send JSON to webhook views

agjohnson added the PR: ready for review label Oct 2, 2016

agjohnson mentioned this pull request Oct 2, 2016

Bitbucket webook trigger fails on private projects #2424

Closed

ericholscher requested changes Oct 4, 2016

View reviewed changes

ericholscher added Needed: documentation Documentation is required Needed: patch A pull request is required Status: reviewed and removed PR: ready for review labels Oct 4, 2016

agjohnson added PR: ready for review and removed Needed: documentation Documentation is required Needed: patch A pull request is required Status: reviewed labels Oct 5, 2016

agjohnson mentioned this pull request Oct 5, 2016

Project webhook admin dashboard view #2436

Closed

ericholscher approved these changes Oct 5, 2016

View reviewed changes

ericholscher added Status: reviewed and removed PR: ready for review labels Oct 5, 2016

agjohnson added 3 commits October 5, 2016 16:04

Lint fixes on API integrations

6ae03ef

Fix incoming payload parsing, cleanup, and add logging

d4d3322

agjohnson force-pushed the webhook-refactor branch from 54ced28 to d4d3322 Compare October 5, 2016 23:04

agjohnson merged commit 90249e3 into master Oct 5, 2016

agjohnson deleted the webhook-refactor branch October 5, 2016 23:09

ghost mentioned this pull request Oct 19, 2016

Add GitLab repo sync and webhook support #1870

Closed

5 tasks

browniebroke mentioned this pull request May 13, 2024

Documentation on readthedocs.io site is outdated jazzband/django-hosts#135

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor webhooks, add modern webhooks, and fix issues with Bitbucket #2433

Refactor webhooks, add modern webhooks, and fix issues with Bitbucket #2433

agjohnson commented Oct 2, 2016 •

edited

Loading

ericholscher left a comment

ericholscher Oct 4, 2016 •

edited

Loading

agjohnson Oct 4, 2016

agjohnson Oct 4, 2016

agjohnson Oct 4, 2016

ericholscher Oct 4, 2016

ericholscher Oct 4, 2016

agjohnson Oct 5, 2016

ericholscher Oct 4, 2016

ericholscher Oct 4, 2016

ericholscher Oct 4, 2016

agjohnson Oct 4, 2016

agjohnson commented Oct 5, 2016

ericholscher left a comment

agjohnson commented Oct 5, 2016

		if request.method == 'POST':
		payload = request.POST.get('payload')

Refactor webhooks, add modern webhooks, and fix issues with Bitbucket #2433

Refactor webhooks, add modern webhooks, and fix issues with Bitbucket #2433

Conversation

agjohnson commented Oct 2, 2016 • edited Loading

ericholscher left a comment

Choose a reason for hiding this comment

ericholscher Oct 4, 2016 • edited Loading

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 Oct 5, 2016

ericholscher left a comment

Choose a reason for hiding this comment

agjohnson commented Oct 5, 2016

agjohnson commented Oct 2, 2016 •

edited

Loading

ericholscher Oct 4, 2016 •

edited

Loading