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

Separate update and checkout steps #4901

Merged
merged 10 commits into from
Nov 20, 2018

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 14, 2018

So, I can see that the update commands of the other VCS' do the same that the first part of checkout. And we don't use update in any part of the code.

Closes #4259

And half fixes (#4890), right now users without a master branch are blocked and can't do anything to build their docs (previously users could put any text on the default branch, but now it's a select field).

With this change, the first build will fail, but now they can select another version as default branch.

Still wip, a lot tests will break probably and I neeed to do the same with the other VCS'

TODO

  • git
  • svn
  • hg
  • bzr

@stsewd stsewd added the PR: work in progress Pull request is not ready for full review label Nov 14, 2018
@codecov
Copy link

codecov bot commented Nov 14, 2018

Codecov Report

Merging #4901 into master will increase coverage by 0.1%.
The diff coverage is 76.31%.

@@            Coverage Diff            @@
##           master    #4901     +/-   ##
=========================================
+ Coverage   76.65%   76.76%   +0.1%     
=========================================
  Files         158      158             
  Lines       10057    10052      -5     
  Branches     1269     1267      -2     
=========================================
+ Hits         7709     7716      +7     
+ Misses       2007     1995     -12     
  Partials      341      341
Impacted Files Coverage Δ
readthedocs/vcs_support/backends/git.py 80.83% <100%> (+0.48%) ⬆️
readthedocs/vcs_support/backends/hg.py 64.51% <100%> (+5.42%) ⬆️
readthedocs/vcs_support/backends/svn.py 29.5% <25%> (+1.81%) ⬆️
readthedocs/vcs_support/backends/bzr.py 32.72% <50%> (+0.58%) ⬆️
readthedocs/projects/tasks.py 71.2% <77.77%> (+0.17%) ⬆️
readthedocs/restapi/client.py 87.87% <0%> (+0.78%) ⬆️
readthedocs/vcs_support/base.py 90.56% <0%> (+1.88%) ⬆️

@stsewd
Copy link
Member Author

stsewd commented Nov 14, 2018

Diff coverage is falling because we don't have tests for bzr or svn. Otherwise, this is good to merge.

@stsewd stsewd removed the PR: work in progress Pull request is not ready for full review label Nov 14, 2018
@stsewd stsewd requested a review from a team November 14, 2018 18:05
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code here looks good! I'm still not 100% on the potential fallout from this though, so it might be good to wait on a 2nd review

@agjohnson
Copy link
Contributor

@humitos you're probably most familiar with this code, marking you for another review of this

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed this makes sense.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me.

Now, it's cleaner than a checkout only means checkout instead of "update and checkout" or similar.

I don't see any potential problem with this on this flow, but we should check if there are other places where the VCS classes are used and confirm that we don't need to adapt those flows.

except Exception:
log.exception('Unknown Sync Versions Exception')
try:
api_v2.project(self.project.pk).sync_versions.post(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment about hitting this endpoint could trigger a new build for the stable is still relevant here, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You lost some info when moving the docstring. It talks about the stable version.

In fact, it may worth to expand it a little more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@stsewd
Copy link
Member Author

stsewd commented Nov 19, 2018

but we should check if there are other places where the VCS classes are used and confirm that we don't need to adapt those flows.

I grep the project and didn't find any additional checkout calls

@stsewd
Copy link
Member Author

stsewd commented Nov 20, 2018

I'm merging this to start testing it more locally before the release.

@stsewd stsewd merged commit 4c24cad into readthedocs:master Nov 20, 2018
@stsewd stsewd deleted the separate-checkout-and-update-steps branch November 20, 2018 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better UX when git checkout to a not existing branch/tag
4 participants