readthedocs.org
readthedocs.org copied to clipboard
readthedocs
Resync all github webhooks to expose new features
After https://github.com/rtfd/readthedocs.org/pull/4876 now we sync branches and tags when there are created, but it requires listening to new events in GitHub (BitBucket and GitLab are fine).
New webhooks are created with the new events, but old webhooks are not listening to these events, users need to resync the webhook.
It was suggested that we could resync this automatically https://github.com/rtfd/readthedocs.org/issues/4450#issuecomment-450150930. This is a one time operation I guess (or maybe it can be useful for https://github.com/rtfd/readthedocs.org/pull/4940 too).
So, maybe we can add this as a management command? Also, we are going to hit the github API a lot, we could reach a limit I guess. Have we done something like this before?
I think that "manually re-syncing" relying on the user is not a solution. We will end up with a few users with the new features and the one that do not re-sync keep opening issues for this inconsistency problem between versions and branches.
So, maybe we can add this as a management command? Also, we are going to hit the github API a lot, we could reach a limit I guess. Have we done something like this before?
Considering that "we have the ability to do it automatically" I'd go this way.
We have around ~90k projects. I guess that around 70% of them are from GitHub. If we hit the API every 1s, in 16 hours we are going to already migrate them all in just one shot.
Yesterday I had a crazy idea, what about doing this lazily? We can add a field to the webhook model or project indicating that the webhook needs a resync. When the webhook is hit we check if it needs a resync. That way we can just do a data migration and solve the problem.
It's not an API rate limitation, we can't take action on the webhook unless the user has a GitHub user with permission to change the project attached to the RTD project. This is not going to be 100% of the GitHub projects, so I don't think we can really enforce this in an automated way.
Our code should support both methods of operation based on what permissions were grants to us. Projects without these permissions should keep building.
Projects without these permissions are building, they just don't have the new features, they can have them by granting more permissions like is mentioned in https://docs.readthedocs.io/en/stable/webhooks.html
Closing as looks like we don't want to put new permissions without user intervention.
we can't take action on the webhook unless the user has a GitHub user with permission to change the project attached to the RTD project
@agjohnson I don't think we need extra permissions here. We do have permissions to add webhooks on behalf of the user. So, if it's not possible to upgrade the current webhook, we can delete it and create a new one that subscribes to the new events that we need. Don't you think this is possible?
they just don't have the new features, they can have them by granting more permissions like is mentioned in docs.readthedocs.io/en/stable/webhooks.html
I don't think many users will do this, and they will complain anyway saying that their versions are out of sync.
Docker did what I'm proposing here. See https://success.docker.com/article/i-have-issues-triggering-autobuilds
During that time, we sent out emails to users whose credentials we had stored did not have sufficient permissions for us to re-create webhooks on Github and Bitbucket using the new integrations
Now that we re-sync all the RemoteRepository for all the users and auto-connect all the Project to their RemoteRepository if it exists (see #8229), I want to revive this issue and do the same for the webhooks.
I want to do the same here and "update all the webhooks for all (non spam) projects". This way, the webhooks will have all the events we need [^1] and we will give users a better UX when enabling PR builder on their projects, avoiding them having to deal with the Troubleshooting section of our documentation, checking logs, updating webhooks manually, etc. Note that we keep receiving support request about this and users are usually pretty confused about why PR builder doesn't work on their projects.
I can write a similar script than the one used in #8229 and run it once from web-extra. / cc @ericholscher
[^1]: this is valid for PR and also to re-sync tags/branches immediately when they are deleted/created
@humitos I think that sounds like a great improvement. The more we can do to automatically get things into a working state for users, the better the product is going to feel and work.
@ericholscher I just created this script to re-sync the webhooks for GitLab and GitHub (https://github.com/readthedocs/r/blob/main/scripts/production/resync_webhooks.py). I ran a test with the first 10 projects matching the query and it worked. These are the logs: https://onenr.io/0Bj3BmrexRX. Some were updated, some other didn't and some projects were skipped because they looked like spam.
Can you please take a quick look at that script? If you don't find anything terrible on it, I'm happy to run it for all the projects matching the query (~50k)
@humitos the script looks good. A sleep of .1s is probably not doing much since I'm sure the queries take longer than that, but seems reasonable to run 👍 I wonder if this should be a management command in the application, instead of in a random script somewhere we'll never find again?
I wonder if this should be a management command in the application, instead of in a random script somewhere we'll never find again?
There is a trade-off on this. Having a management command that we plan to execute just once makes us to keep maintaining it and adapting each time we upgrade Django or similar. On the other hand, having it there and being broken is also useless and confusing.
I remember that we talked about this sometime ago and I found https://github.com/readthedocs/readthedocs.org/issues/1526
Those were my arguments to publish it into this separate repository, instead of just keeping it locally and private. I can easily share it with the rest of the team, but it does not interfere with application's code.
I triggered the resync for all these webhooks. I will close this issue once the script has finished. I guess it will take 1 or 2hs more to finish.
Yea, I imagine in the future we'll need to run this if we change the webhook again, but I agree it probably doesn't need to go in the codebase.