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

Sync versions when creating a new branch/tag #4450

Closed
stsewd opened this issue Jul 30, 2018 · 14 comments
Closed

Sync versions when creating a new branch/tag #4450

stsewd opened this issue Jul 30, 2018 · 14 comments
Assignees
Labels
Accepted Accepted issue on our roadmap Feature New feature

Comments

@stsewd
Copy link
Member

stsewd commented Jul 30, 2018

Currently rtd doesn't detect if a branch/tag was deleted, we only detect if a commit was pushed to a branch or a tag was re-tagged.

Ref #3192 (comment)

Current status

GitHub

We only listen to push events for branch and tags in github https://developer.github.com/v3/activity/events/types/#pushevent. We can listen to Create and Delete events https://developer.github.com/v3/activity/events/types/#createevent https://developer.github.com/v3/activity/events/types/#deleteevent

GitLab

We only listen to push events which exclude tags https://docs.gitlab.com/ce/user/project/integrations/webhooks.html#push-events. We can listen to tag events with https://docs.gitlab.com/ce/user/project/integrations/webhooks.html#tag-events

GitLab doesn't have a separate event for creation/deletion, instead, it sets the before/after field to 0000000000000000000000000000000000000000

BitBucket

We currently listen to push events, it works for branches and tags https://confluence.atlassian.com/bitbucket/event-payloads-740262817.html#EventPayloads-Push.

BitBucket doesn't have a separate event for creation/deletion, instead, it sets the new attribute (null if it is a deletion) and the old attribute (null if it is a creation).

Solution

First, we need to listen to (or modify) the above events to have the information about the creations and deletions. Our sync step is bound to a specific version (and it should exists in the database)

https://github.com/rtfd/readthedocs.org/blob/b04b11298f5a45d6f6c6ec3f9bbbf368b942295b/readthedocs/projects/tasks.py#L223-L226

So, we can't call it directly, said that we have 2 options:

  1. Trigger a new build to the default branch

  2. Call to SyncRepositoryTask with the default branch

https://github.com/rtfd/readthedocs.org/blob/b04b11298f5a45d6f6c6ec3f9bbbf368b942295b/readthedocs/projects/tasks.py#L223-L226

It doesn't trigger a new build, but clone the repo to the default branch and update the version list.

Conclusion

The changes for getting the information about new/removed branches and tags is relatively easy, but we need to take a decision about how to sync the versions, with 1) the user can see the build process and that something is happening, but we will be wasting resources building the documentation, with 2) we save resources but the user doesn't see anything (it can be checked in the webhook response anyway)

@stsewd stsewd added the Needed: design decision A core team decision is required label Jul 30, 2018
@humitos
Copy link
Member

humitos commented Jul 31, 2018

First, we need to listen to (or modify) the above events to have the information about the creations and deletions

On Github, for example, we are only listening for push and pull_request events. Here is the code where this lives and should be updated to add more events:

https://github.com/rtfd/readthedocs.org/blob/b04b11298f5a45d6f6c6ec3f9bbbf368b942295b/readthedocs/oauth/services/github.py#L175

(the get_webhook_data on the other services classes have the same info for GitLab and Bitbucket)

  1. we save resources but the user doesn't see anything (it can be checked in the webhook response anyway)

I prefer this way. Why? Because I think the user shouldn't care about this at all. This process of syncying the VCS repo with our Versions should be transparent and automatic. I'd say that we can trigger the SyncRepositoryTask with the default_branch of the project, as you mentioned.

@humitos humitos added this to the New build features milestone Jul 31, 2018
@humitos humitos added the Feature New feature label Jul 31, 2018
@humitos
Copy link
Member

humitos commented Jul 31, 2018

I just realize that we have an inconsistency in our code. Even though that we are subscribed to push and pull_request in GitHub. Then, when we receive an event we are just considering push only:

https://github.com/rtfd/readthedocs.org/blob/b04b11298f5a45d6f6c6ec3f9bbbf368b942295b/readthedocs/restapi/views/integrations.py#L161

@stsewd
Copy link
Member Author

stsewd commented Jul 31, 2018

@humitos maybe for a future implementation #1340 , we don't need that at the moment

@humitos
Copy link
Member

humitos commented Jul 31, 2018

I meant that we are subscribing webhooks for two different events and we are processing only one.

@agjohnson agjohnson added the Accepted Accepted issue on our roadmap label Aug 2, 2018
collin-mcgrath added a commit to Invoca/developer-docs that referenced this issue Oct 18, 2018
…etwork: Describe Read the Docs build workaround for new branches

- Should be remove when readthedocs/readthedocs.org#4450 is completed.
@ericholscher
Copy link
Member

Call to SyncRepositoryTask with the default branch

Feels like the right implementation. That was the general idea behind that bit of code to behind with, so we're mostly just expanding when it gets called to include delete events, I think?

@astrojuanlu
Copy link
Contributor

Has this been deployed already?

@stsewd
Copy link
Member Author

stsewd commented Dec 27, 2018

Yes, it is

@astrojuanlu
Copy link
Contributor

I ask because I pushed a tag and it was not discovered in RTD, and it has happened more times this year.

Maybe the problem is that my repository is configured with a GitHub Service (which are being deprecated) instead of a webhook. However, the documentation says that "Just the push event" should be selected. Perhaps it needs an update as well?

@stsewd
Copy link
Member Author

stsewd commented Dec 27, 2018

Oh, yes, the docs should be updated, we need to listen to delete and create events on github too https://github.com/rtfd/readthedocs.org/pull/4876/files#diff-d4da38e842120f1e061ecec27ff050c4R175.

That's done automatically by rtd, you only need to do that if you created the webhook manually. If it's an old webhook created by rtd, you should resync the webhook.

@humitos
Copy link
Member

humitos commented Dec 27, 2018

I can confirm that this is working properly in production but, as @stsewd said, you need to "Resync" your Webhook from Admin > Integrations so we subscribe to the new events.

@stsewd is this manual step something that we can plan to do automatically for all of our users? Sounds like not too many people will use this feature because they imported the project months ago otherwise :/

@stsewd
Copy link
Member Author

stsewd commented Dec 27, 2018

@humitos I think that's a manual step, but we could show a message maybe? But not sure how to identify the old webhooks.

@humitos
Copy link
Member

humitos commented Dec 27, 2018

I think that's a manual step, but we could show a message maybe?

Why it can't be automated?

But not sure how to identify the old webhooks.

All the project created before the PR got deployed need to be re-synced.

@stsewd
Copy link
Member Author

stsewd commented Dec 27, 2018

All the project created before the PR got deployed need to be re-synced.

It may be projects that are already re-synced, but I suppose that shouldn't be a problem.

@stsewd
Copy link
Member Author

stsewd commented Jan 3, 2019

@humitos I opened #5062 to track this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Feature New feature
Projects
None yet
Development

No branches or pull requests

5 participants