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

Handle GitHub Push events with deleted: true in the JSON #6465

Merged
merged 2 commits into from Dec 18, 2019

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 17, 2019

This fixes #6464

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)
Copy link
Member Author

@ericholscher ericholscher Dec 17, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

@stsewd stsewd left a comment

We are missing to sync the branches when it's created in a push event

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):
Copy link
Member

@stsewd stsewd Dec 17, 2019

Choose a reason for hiding this comment

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

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:
Copy link
Member

@stsewd stsewd Dec 17, 2019

Choose a reason for hiding this comment

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

If created=True this wouldn't trigger a new build, because the version doesn't exist yet in read the docs.

versions = project.versions_from_branch_name(branch)
for version in versions:

We should check that created is False here too.

Copy link
Member Author

@ericholscher ericholscher Dec 17, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Member

@stsewd stsewd Dec 17, 2019

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@ericholscher ericholscher Dec 17, 2019

Choose a reason for hiding this comment

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

Ah, I understand, so we need to check for created on the push as well 👍

stsewd
stsewd approved these changes Dec 17, 2019
@humitos humitos merged commit fac42fc into master Dec 18, 2019
3 checks passed
@humitos humitos deleted the dont-trigger-builds-on-push-delete branch Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants