Skip to content

Safe symlink on version deletion #4937

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 10, 2019
Merged

Safe symlink on version deletion #4937

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 bot commented Nov 29, 2018 •
edited
Loading

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 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 humitos added the PR: work in progress Pull request is not ready for full review label Dec 3, 2018
@humitos
Copy link
Member

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 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 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 pull request Dec 4, 2018
@humitos
Remove project_version_delete_html view
d362a11 
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 humitos mentioned this pull request Dec 4, 2018
Closed
humitos added a commit that referenced this pull request Dec 4, 2018
@humitos
Remove project_version_delete_html view
7982459 
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 removed the PR: work in progress Pull request is not ready for full review label Dec 4, 2018
@stsewd stsewd added this to the Cleanup milestone Dec 11, 2018
@stsewd
Copy link
Member Author

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 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.

@humitos humitos added the PR: work in progress Pull request is not ready for full review label Dec 17, 2018
@stsewd
Copy link
Member Author

stsewd commented Dec 17, 2018

@humitos that was added for you actually https://github.com/rtfd/readthedocs-corporate/commit/3b335f359efd4c5045c7b93d0e6f47bf159360ef, I guess we need to keep that code?

@humitos
Copy link
Member

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.

@humitos humitos removed the PR: work in progress Pull request is not ready for full review label Dec 18, 2018
@stsewd stsewd requested a review from a team January 7, 2019 17:16
@ericholscher
Copy link
Member

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

ericholscher
ericholscher approved these changes Jan 10, 2019
View reviewed changes
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.

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
@stsewd stsewd deleted the safe-symlink-on-version-deletion branch January 10, 2019 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericholscher ericholscher ericholscher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Cleanup
Development

Successfully merging this pull request may close these issues.
3 participants
@stsewd @humitos @ericholscher