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

Integration Re-sync Bug Fix #6124

Merged
merged 23 commits into from Sep 10, 2019

Conversation

@saadmk11
Copy link
Member

commented Aug 29, 2019

closes: #6123
ref: #6122

Copy link
Member

left a comment

We're adding remove_secret a lot of places in this PR. It feels like it's almost certainly too complex. Why wasn't this needed before, but is needed now?

@saadmk11

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

@ericholscher This is done because previously we did not sync webhooks when we create an integration but now we do so. If for any reason the webhook fails to re-sync this tries to remove the secret so that the integration can be used manually by the user.

@ericholscher

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Ugh, I broke something trying to push to this branch. Silly github :/

@ericholscher ericholscher force-pushed the saadmk11:sync-bug-fix branch from 67cd06a to 79cdefb Sep 5, 2019
@ericholscher

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Alright, fixed up the commits, and cleaned up some of the logic. I think it makes more sense now -- I'm not 100% sold on the GitHub setup_webhook that will create a new webhook when we try and sync one we aren't managing. I don't know the best path forward on those -- perhaps we could query GitHub to get the webhook pointing at the URL?

@@ -203,7 +203,7 @@ def setup_webhook(self, project, integration=None):
"""
session = self.get_session()
owner, repo = build_utils.get_github_username_repo(url=project.repo)
if integration:
if integration and not integration.secret:

This comment has been minimized.

Copy link
@saadmk11

saadmk11 Sep 5, 2019

Author Member

@ericholscher When we pass the integration from update_webhook to setup_webhook on 404the integration already has a secret so this condition always returns False and creates a new integration. thats why some tests are failing.

saadmk11 added 5 commits Sep 5, 2019
Copy link
Member

left a comment

This is a much better approach. It will allow us to "reclaim" a webhook on GitHub if it already exists, and update it if it doesn't.

@@ -226,6 +226,19 @@ def get_paginated_results(self, response):
"""
raise NotImplementedError

def get_provider_data(self, project, integration):
"""
Gets provider data from GitHub Webhooks API.

This comment has been minimized.

Copy link
@ericholscher

ericholscher Sep 5, 2019

Member

This shouldn't mention GitHub, as it's on the base class.

saadmk11 added 3 commits Sep 5, 2019
@ericholscher ericholscher requested a review from readthedocs/core Sep 9, 2019
@ericholscher

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@stsewd Would love your take on this -- I think it's a better approach, but could use a bit more review.

@stsewd

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@ericholscher I'll try to review this today

@stsewd
stsewd approved these changes Sep 10, 2019
Copy link
Member

left a comment

LGTM 👍

I removed some dead code and updated the docs.

So, looks like we are not updating the secret on resync anymore. But looks like we found a better pattern to remove the secret if something failed p: If we go for not update the secret on resync, we need to update our docs https://docs.readthedocs.io/en/stable/webhooks.html#payload-validation

@saadmk11

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

@stsewd Thanks for the update looks good :)

@ericholscher

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

If we go for not update the secret on resync, we need to update our docs https://docs.readthedocs.io/en/stable/webhooks.html#payload-validation

Yea, we should update the doc in this PR then 👍

@stsewd

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Done, feel free to merge :)

@ericholscher ericholscher merged commit 72972b5 into readthedocs:master Sep 10, 2019
2 checks passed
2 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@saadmk11 saadmk11 deleted the saadmk11:sync-bug-fix branch Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.