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

Safe symlink on version deletion #4937

Merged
merged 5 commits into from Jan 10, 2019

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Nov 29, 2018

Fix #3743

Also, I think we don't use this in code. I thought we were using it when syncing versions, but we call
https://github.com/rtfd/readthedocs.org/blob/7f2b831ac18ee839535254e9b14e595621ebf653/readthedocs/restapi/utils.py#L172-L172

there, when doing a deletion in that way, django doesn't call the delete method.

@codecov
Copy link

@codecov codecov bot commented Nov 29, 2018

Codecov Report

Merging #4937 into master will increase coverage by 0.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4937      +/-   ##
==========================================
+ Coverage   76.74%   76.97%   +0.22%     
==========================================
  Files         158      158              
  Lines        9951    10032      +81     
  Branches     1244     1259      +15     
==========================================
+ Hits         7637     7722      +85     
+ Misses       1980     1976       -4     
  Partials      334      334
Impacted Files Coverage Δ
readthedocs/builds/models.py 79.77% <ø> (+0.94%) ⬆️
readthedocs/restapi/views/model_views.py 94.15% <0%> (-0.97%) ⬇️
readthedocs/doc_builder/python_environments.py 82.97% <0%> (-0.47%) ⬇️
readthedocs/projects/models.py 85.44% <0%> (-0.12%) ⬇️
readthedocs/gold/views.py 66.17% <0%> (ø) ⬆️
readthedocs/projects/constants.py 100% <0%> (ø) ⬆️
readthedocs/projects/forms.py 79.66% <0%> (ø) ⬆️
readthedocs/restapi/serializers.py 97.43% <0%> (ø) ⬆️
readthedocs/projects/views/private.py 80.1% <0%> (+0.05%) ⬆️
... and 4 more

@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 29, 2018

Not really sure how to test this one.

And, Investigating more about this, we don't call delete if the version is active, so, if the user deletes the version on the repo and the version is active in rtd, the docs aren't deleted, which is good. If we deactivate the version, it deletes the docs.

That happens here

https://github.com/rtfd/readthedocs.org/blob/7f2b831ac18ee839535254e9b14e595621ebf653/readthedocs/builds/models.py#L203-L215

So, again, not sure if we need that code.

@humitos
Copy link
Member

@humitos humitos commented Dec 3, 2018

We may want to check the git log to understand why/when that .delete code was written.

I suppose that we don't need it since we don't delete versions calling manually Version.delete.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Dec 3, 2018

This was added here #1686, maybe this can be useful when deleting a version from the admin, not sure.

@humitos
Copy link
Member

@humitos humitos commented Dec 4, 2018

I think it's safe to remove this delete method completely. We do have a task called remove_orphan_symlinks and delete_media.

The later is not exactly the same than clear_artifacts but similar. Although, clear_artificts is being called where it has to be called: when disabling a version or cleaning a version.

humitos added a commit that referenced this issue Dec 4, 2018
We used to have a "Clean" button in the Inactive Versions listing to
remove built and disabled versions of the documentation.

This is not more possible, because when the version is disabled (mark
as non active) we call `clean_artifacts` and remove all the HTML files
for this version and mark it as `Version.build=False`. In the end,
that button does not appear anymore.

Related #4937
humitos added a commit that referenced this issue Dec 4, 2018
We used to have a "Clean" button in the Inactive Versions listing to
remove built and disabled versions of the documentation.

This is not more possible, because when the version is disabled (mark
as non active) we call `clean_artifacts` and remove all the HTML files
for this version and mark it as `Version.build=False`. In the end,
that button does not appear anymore.

Related #4937
@stsewd stsewd added this to the Cleanup milestone Dec 11, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Dec 13, 2018

OK, turns out we do use this in .com

https://github.com/rtfd/readthedocs-corporate/blob/0738490bb24d88d519db8212bf5cc65690e68073/readthedocsinc/organizations/signals.py#L104-L104

Actually, not sure, I'm not so familiar with that code, so not sure if it's dead code or what, if not, I can revert the latest commit and put the leave the initial fix :)

@humitos
Copy link
Member

@humitos humitos commented Dec 17, 2018

OK, turns out we do use this in .com

Good point!

We need to take a look into that before merging this PR and removing the delete method. You are right.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Dec 17, 2018

@humitos that was added for you actually readthedocs/readthedocs-corporate@3b335f3, I guess we need to keep that code?

@humitos
Copy link
Member

@humitos humitos commented Dec 18, 2018

Yes! I was thinking if we may want to migrate that code to .com, but I think it's correct to use the .delete method and keep it where it is.

@stsewd stsewd requested a review from Jan 7, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 10, 2019

It looks like this patch is just a small linting change, is there more to it?

Copy link
Member

@ericholscher ericholscher left a comment

Oh, I see. We're just moving the symlinking to after the delete. Makes sense.

@stsewd stsewd merged commit 8ad259a into readthedocs:master Jan 10, 2019
1 of 2 checks passed
@stsewd stsewd deleted the safe-symlink-on-version-deletion branch Jan 10, 2019
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