Don't trigger a sync twice on creation/deletion for GitHub #6614
Conversation
We are listening for creation/deletion on push events for old webhooks.
| # GitHub will send push and create/delete events on a creation/deletion. | ||
| # If we receive a push event we need to check if the webhook doesn't | ||
| # already have the create/delete events. So we don't trigger the sync twice. | ||
| # We listen to push events for creation/deletion for old webhooks only. |
There was a problem hiding this comment.
I don't follow this code 😢
We are already handling GITHUB_PUSH in the previous if with deleted or created (line 382), why don't we just remove the GITHUB_PUSH from this elif (line 389) instead? That seems to do the same without extra code.
Like this on line 389:
elif event in (GITHUB_CREATE, GITHUB_DELETE):
return self.sync_versions(self.project)
That way, we are only syncing versions on CREATE and DELETE, but not on regular PUSH.
There was a problem hiding this comment.
I think what we want is:
GITHUB_PUSH and (created or deleted)-> sync versionsGITHUB_CREATE or GITHUB_DELETE-> sync versions- all the other possibilities -> do not sync versions
If this is correct, I do see this logic easier to understand than our current implementation.
There was a problem hiding this comment.
We are already handling GITHUB_PUSH in the previous if with deleted or created
We are triggering a build in that case, and we check for not (deleted or created)
why don't we just remove the GITHUB_PUSH from this elif (line 389) instead?
We want to listen to PUSH events for sync, but only trigger a sync if the webhook isn't subscribed to creation/deletion events.
There was a problem hiding this comment.
GitHub will send two events when a branch/tag is created/deleted (if the webhook is listening to creation/deletion events), that's why we need to discard the push event in that case.
GITHUB_PUSH and (created or deleted) -> sync versions
That will still need to check if the webhook isn't subscribed to creation/deletion. Otherwise it'll trigger two syncs.
GITHUB_CREATE or GITHUB_DELETE -> sync versions
This will trigger a second sync for users that have webhooks listening to creation/deletion events.
| # GitHub will send push and create/delete events on a creation/deletion. | ||
| # If we receive a push event we need to check if the webhook doesn't | ||
| # already have the create/delete events. So we don't trigger the sync twice. | ||
| # We listen to push events for creation/deletion for old webhooks only. |
There was a problem hiding this comment.
I don't follow this code 😢
We are already handling GITHUB_PUSH in the previous if with deleted or created (line 382), why don't we just remove the GITHUB_PUSH from this elif (line 389) instead? That seems to do the same without extra code.
Like this on line 389:
elif event in (GITHUB_CREATE, GITHUB_DELETE):
return self.sync_versions(self.project)
That way, we are only syncing versions on CREATE and DELETE, but not on regular PUSH.
| # GitHub will send push and create/delete events on a creation/deletion. | ||
| # If we receive a push event we need to check if the webhook doesn't | ||
| # already have the create/delete events. So we don't trigger the sync twice. | ||
| # We listen to push events for creation/deletion for old webhooks only. |
There was a problem hiding this comment.
I think what we want is:
GITHUB_PUSH and (created or deleted)-> sync versionsGITHUB_CREATE or GITHUB_DELETE-> sync versions- all the other possibilities -> do not sync versions
If this is correct, I do see this logic easier to understand than our current implementation.
|
@stsewd Thoughts on Manuel's comments? Need to address those before merging. |
|
@ericholscher I added the missing test and put the default value on the model. I also replied to #6614 (comment) |
Weird, it doesn't show it later in the PR thread, really confusing. |
|
Alright, I'm gonna go ahead and merge this, since I think it makes sense. We will see if it blows up in prod :) |
We are listening for creation/deletion on push events for old webhooks.
Another solution is to don't listen to push events for creation/deletion and force users to update those events in their old webhooks. Listening for creations/deletion on push events in old webhoooks was introduced in #6465