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/deleting versions #4876

Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Nov 7, 2018

Closes #4450

Still wip, I need to write some tests, but I tested the GitHub integration with ngrok, and it works really good.

TODO

  • GitHub
  • GitLab
  • Bitbucket
'build_triggered': False,
'project': project.slug,
'versions': [version],
}
Copy link
Member Author

@stsewd stsewd Nov 7, 2018

Still not sure about what to return, I'm returning the version that was used to do the clone, I guess this isn't so important, I mean, no one uses this, it's just sent to the integration (GitHub)

@codecov
Copy link

@codecov codecov bot commented Nov 7, 2018

Codecov Report

Merging #4876 into master will increase coverage by 0.05%.
The diff coverage is 90.47%.

@@            Coverage Diff            @@
##           master   #4876      +/-   ##
=========================================
+ Coverage   76.65%   76.7%   +0.05%     
=========================================
  Files         158     158              
  Lines       10057   10098      +41     
  Branches     1269    1274       +5     
=========================================
+ Hits         7709    7746      +37     
- Misses       2007    2010       +3     
- Partials      341     342       +1
Impacted Files Coverage Δ
readthedocs/oauth/services/github.py 43.58% <ø> (ø) ⬆️
readthedocs/restapi/views/integrations.py 91.76% <100%> (+3.11%) ⬆️
readthedocs/core/views/hooks.py 80.21% <62.5%> (-2.14%) ⬇️

@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 7, 2018

I'd like to have an early review here (GitHub is done), I think we could just sync the versions without cloning the repo (not sure yet, I'll investigate tomorrow)

@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 7, 2018

I didn't found any cons in using the response from the provider's API to sync the versions, I think I'll go for that method. I'll leave the previous commits if we decide to go for cloning the repo.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 7, 2018

Actually, GitHub doesn't return the commit sha :/, and that's something we kind of rely on tags. So, back to do the cloning. We can change the way tags are represented I think, something for later anyway.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 7, 2018

Gitlab is done, but I'll need to test it locally.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 8, 2018

Just tested gitlab, it's working :D

@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 8, 2018

Bitbucket is tested too, but I had to setup the webhook manually because I wasn't able to do the auth integration. I think this is ready for review.

Copy link
Member

@humitos humitos left a comment

I like these changes. I think it's close to be merged.

The code takes some decisions depending on the VCS service we receive the webhook. Reading the code without the context on how each service works is hard to understand (I need to google by myself the docs for each service and compare it with your code). Because of this, I'd like to see more comments on each of these decisions with links to the documentation.

Also, I'm thinking if the repo_nonblockinglock could be a possible problem here. Depending on "how many webhooks we will receive" on different situations. For example, what happen if the user does:

git push --tags

which will make a push (sending new commits and new tags). Will this trigger one PUSH and one CREATE events? What if the user is pushing more than one tag? This could cause a problem when building because if the sync_versions is executed first by celery, then the update_docs_task will be locked.

This is something to be considered. I'm not sure this is a real problem, though

Due that `sync_repository_task` is bound to a version,
we always pass the latest version.
:returns: The version that was used to trigger the clone (usually latest).
Copy link
Member

@humitos humitos Nov 15, 2018

Returns the version slug.

You can use :rtype: string also here.

"""
try:
version_slug = LATEST
version = project.versions.get(slug=version_slug)
Copy link
Member

@humitos humitos Nov 15, 2018

Won't this fail for users that override the latest version?

I don't remember exactly, but I think there was also a way to disable latest, right?

Copy link
Member Author

@stsewd stsewd Nov 15, 2018

Yup, I was considering using the default branch here instead.

readthedocs/restapi/views/integrations.py Show resolved Hide resolved
readthedocs/restapi/views/integrations.py Show resolved Hide resolved
@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 15, 2018

Also, I'm thinking if the repo_nonblockinglock could be a possible problem here. Depending on "how many webhooks we will receive" on different situations. For example, what happen if the user does:
which will make a push (sending new commits and new tags). Will this trigger one PUSH and one CREATE events? What if the user is pushing more than one tag? This could cause a problem when building because if the sync_versions is executed first by celery, then the update_docs_task will be locked.

I have tested this locally, and works well becuase the push step doesn't trigger a new build (the new version doesn't exists!), so only the create event triggers a the sync. Here are some responses from my webhook when testing that

screenshot_2018-11-15 stsewd rtd-test

When pushing several versions, that would trigger several syncs, but the sync is really quick compared to the time that takes a full build, and I don't think we are going to experiment this very often, also, the current implementation has the same problem (when pushing several commits to master). And we are protected by the services, for example, from the github docs:

Additionally, webhooks will not receive this event for tags if more than three tags are pushed at once.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 15, 2018

I had added some docs and comments, and also I'm using the default_branch rather than using LATEST.

Copy link
Member

@humitos humitos left a comment

Thanks for the changes here. This looks great to me.

Copy link
Member

@ericholscher ericholscher left a comment

Looks good to me.

@ericholscher ericholscher merged commit 37f2ef4 into readthedocs:master Nov 29, 2018
3 checks passed
@stsewd stsewd deleted the sync-version-when-creating-branch-tag branch Nov 29, 2018
@stsewd stsewd mentioned this pull request Dec 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants