Handle GitHub Push events with deleted: true in the JSON
#6465
Conversation
| try: | ||
| branches = [self._normalize_ref(self.data['ref'])] | ||
| return self.get_response_push(self.project, branches) | ||
| except KeyError: | ||
| raise ParseError('Parameter "ref" is required') | ||
| if event in (GITHUB_CREATE, GITHUB_DELETE): | ||
| if event in (GITHUB_CREATE, GITHUB_DELETE) or (event == GITHUB_PUSH and deleted): | ||
| return self.sync_versions(self.project) |
There was a problem hiding this comment.
This is probably not the most efficient thing we could be doing. We could grab the branch name from the Push and delete it explicitly, but this seemed like the easiest implementation.
| try: | ||
| branches = [self._normalize_ref(self.data['ref'])] | ||
| return self.get_response_push(self.project, branches) | ||
| except KeyError: | ||
| raise ParseError('Parameter "ref" is required') | ||
| if event in (GITHUB_CREATE, GITHUB_DELETE): | ||
| if event in (GITHUB_CREATE, GITHUB_DELETE) or (event == GITHUB_PUSH and deleted): |
There was a problem hiding this comment.
We already subscribe to create and delete events that does the same. But I think this is still useful for users with old webhooks that only have the push event. We should also build when there is a push event with created=True.
| if event == GITHUB_PUSH: | ||
| # Don't build a branch if it's a push that was actually a delete | ||
| # https://developer.github.com/v3/activity/events/types/#pushevent | ||
| if event == GITHUB_PUSH and not deleted: |
There was a problem hiding this comment.
If created=True this wouldn't trigger a new build, because the version doesn't exist yet in read the docs.
readthedocs.org/readthedocs/core/views/hooks.py
Lines 50 to 52 in 2e67b7d
We should check that created is False here too.
There was a problem hiding this comment.
That's an existing bug though, right? I don't know when we want to trigger a build when a user creates a new branch on a push?
There was a problem hiding this comment.
Yeah, a push event + deleted or push event + created should trigger a version sync, not a build (because it will fail when deteled=True and it wouldn't to anything if created=True)
There was a problem hiding this comment.
Ah, I understand, so we need to check for created on the push as well 👍
This fixes #6464