-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Separate update and checkout steps #4901
Conversation
Codecov Report
@@ 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
|
Diff coverage is falling because we don't have tests for bzr or svn. Otherwise, this is good to merge. |
There was a problem hiding this 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
@humitos you're probably most familiar with this code, marking you for another review of this |
There was a problem hiding this 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.
There was a problem hiding this 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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was moved to the function's docstring https://github.com/rtfd/readthedocs.org/pull/4901/files?utf8=%E2%9C%93&diff=unified#diff-b9399e1d3499066c5564f98a620e8881R162
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I grep the project and didn't find any additional checkout calls |
I'm merging this to start testing it more locally before the release. |
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 thedefault 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