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

Don't activate version on build #4810



Copy link

@stsewd stsewd commented Oct 25, 2018

Close #4793

I was trying to write a test for this, but is so complicated or I don't know how
to do it, as it requires to hit the api from the SLUMBER setting.

I was trying using live server, but it didn't work as expected... If codecov fails,
at least I could test that we are calling to the patch with built=True

Copy link

@codecov codecov bot commented Oct 25, 2018

Codecov Report

Merging #4810 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4810   +/-   ##
  Coverage   76.26%   76.26%           
  Files         158      158           
  Lines       10034    10034           
  Branches     1267     1267           
  Hits         7652     7652           
  Misses       2039     2039           
  Partials      343      343
Impacted Files Coverage Δ
readthedocs/projects/ 69.51% <ø> (ø) ⬆️

Copy link
Member Author

@stsewd stsewd commented Oct 25, 2018

Also, this isn't a replacement of #4733, as that PR is needed to not build inactive versions via webhooks

@ericholscher ericholscher requested a review from Oct 31, 2018
humitos approved these changes Nov 1, 2018
Copy link

@humitos humitos left a comment

I know there are different opinions here, but I'm fine by merging this PR.

Although that building a version without making it active doesn't make too much sense for the service, I think that making it automatically makes things to be confusing (as we were confused when working on #4733 and #4810

Also, this could allow us to run management commands to build versions without publishing them, etc.

I only see "positive" things with this change and no negative ones.

Copy link

@ericholscher ericholscher left a comment

I'm willing to test this, but it feels like we're solving a symptom instead of a root cause. I'm also guessing we're going to hit issues with people building docs, and not having them become active, but those will also be bugs, and should be simple to solve.

@ericholscher ericholscher merged commit a2eee02 into readthedocs:master Nov 1, 2018
3 checks passed
@stsewd stsewd deleted the dont-activate-version-on-build branch Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants