From 50d97f7e7edf6f219f9bc683d6914f80301f8912 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 12 Dec 2018 19:44:18 -0500 Subject: [PATCH 1/3] Refactor remove_dir --- readthedocs/core/views/__init__.py | 4 +-- readthedocs/projects/admin.py | 8 +++-- readthedocs/projects/tasks.py | 29 +++++++++---------- readthedocs/projects/views/private.py | 6 +++- .../tests/projects/test_admin_actions.py | 4 +-- readthedocs/rtd_tests/tests/test_celery.py | 4 +-- .../rtd_tests/tests/test_project_views.py | 4 +-- 7 files changed, 32 insertions(+), 27 deletions(-) diff --git a/readthedocs/core/views/__init__.py b/readthedocs/core/views/__init__.py index 342eb530498..dee1cc96dc1 100644 --- a/readthedocs/core/views/__init__.py +++ b/readthedocs/core/views/__init__.py @@ -20,7 +20,7 @@ from readthedocs.builds.models import Version from readthedocs.core.utils import broadcast from readthedocs.projects.models import Project, ImportedFile -from readthedocs.projects.tasks import remove_dir +from readthedocs.projects.tasks import remove_dirs from readthedocs.redirects.utils import get_redirect_response log = logging.getLogger(__name__) @@ -91,7 +91,7 @@ def wipe_version(request, project_slug, version_slug): os.path.join(version.project.doc_path, 'conda', version.slug), ] for del_dir in del_dirs: - broadcast(type='build', task=remove_dir, args=[del_dir]) + broadcast(type='build', task=remove_dirs, args=[(del_dir,)]) return redirect('project_version_list', project_slug) return render( request, diff --git a/readthedocs/projects/admin.py b/readthedocs/projects/admin.py index d55954fdbcf..4a58933c436 100644 --- a/readthedocs/projects/admin.py +++ b/readthedocs/projects/admin.py @@ -31,7 +31,7 @@ WebHook, ) from .notifications import ResourceUsageNotification -from .tasks import remove_dir +from .tasks import remove_dirs class ProjectSendNotificationView(SendNotificationView): @@ -174,7 +174,11 @@ def delete_selected_and_artifacts(self, request, queryset): """ if request.POST.get('post'): for project in queryset: - broadcast(type='app', task=remove_dir, args=[project.doc_path]) + broadcast( + type='app', + task=remove_dirs, + args=[(project.doc_path,)], + ) return delete_selected(self, request, queryset) def get_actions(self, request): diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index be3e56e3a18..e88cdd6e027 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -875,19 +875,19 @@ def sync_files(project_pk, version_pk, hostname=None, html=False, # Clean up unused artifacts version = Version.objects.get(pk=version_pk) if not pdf: - remove_dir( + remove_dirs([ version.project.get_production_media_path( type_='pdf', version_slug=version.slug, ), - ) + ]) if not epub: - remove_dir( + remove_dirs([ version.project.get_production_media_path( type_='epub', version_slug=version.slug, ), - ) + ]) # Sync files to the web servers move_files( @@ -1322,27 +1322,24 @@ def update_static_metadata(project_pk, path=None): # Random Tasks @app.task() -def remove_dir(path): +def remove_dirs(paths): """ - Remove a directory on the build/celery server. + Remove artifacts from the web servers. This is mainly a wrapper around shutil.rmtree so that app servers can kill things on the build server. - """ - log.info('Removing %s', path) - shutil.rmtree(path, ignore_errors=True) - - -@app.task() -def clear_artifacts(paths): - """ - Remove artifacts from the web servers. :param paths: list containing PATHs where production media is on disk (usually ``Version.get_artifact_paths``) """ for path in paths: - remove_dir(path) + log.info('Removing %s', path) + shutil.rmtree(path, ignore_errors=True) + + +@app.task() +def clear_artifacts(paths): + remove_dirs(paths) @app.task(queue='web') diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 06791847dc0..5c1de195eb6 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -221,7 +221,11 @@ def project_delete(request, project_slug): ) if request.method == 'POST': - broadcast(type='app', task=tasks.remove_dir, args=[project.doc_path]) + broadcast( + type='app', + task=tasks.remove_dirs, + args=[(project.doc_path,)] + ) project.delete() messages.success(request, _('Project deleted')) project_dashboard = reverse('projects_dashboard') diff --git a/readthedocs/rtd_tests/tests/projects/test_admin_actions.py b/readthedocs/rtd_tests/tests/projects/test_admin_actions.py index 54111314cee..6898c5bf136 100644 --- a/readthedocs/rtd_tests/tests/projects/test_admin_actions.py +++ b/readthedocs/rtd_tests/tests/projects/test_admin_actions.py @@ -60,7 +60,7 @@ def test_project_ban_multiple_owners(self): @mock.patch('readthedocs.projects.admin.broadcast') def test_project_delete(self, broadcast): """Test project and artifacts are removed""" - from readthedocs.projects.tasks import remove_dir + from readthedocs.projects.tasks import remove_dirs action_data = { ACTION_CHECKBOX_NAME: [self.project.pk], 'action': 'delete_selected', @@ -74,6 +74,6 @@ def test_project_delete(self, broadcast): self.assertFalse(Project.objects.filter(pk=self.project.pk).exists()) broadcast.assert_has_calls([ mock.call( - type='app', task=remove_dir, args=[self.project.doc_path] + type='app', task=remove_dirs, args=[(self.project.doc_path,)] ), ]) diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index 83a5fdbe815..a52df435432 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -46,10 +46,10 @@ def tearDown(self): shutil.rmtree(self.repo) super(TestCeleryBuilding, self).tearDown() - def test_remove_dir(self): + def test_remove_dirs(self): directory = mkdtemp() self.assertTrue(exists(directory)) - result = tasks.remove_dir.delay(directory) + result = tasks.remove_dirs.delay((directory,)) self.assertTrue(result.successful()) self.assertFalse(exists(directory)) diff --git a/readthedocs/rtd_tests/tests/test_project_views.py b/readthedocs/rtd_tests/tests/test_project_views.py index c6e4a93f884..cf437f29d62 100644 --- a/readthedocs/rtd_tests/tests/test_project_views.py +++ b/readthedocs/rtd_tests/tests/test_project_views.py @@ -379,8 +379,8 @@ def test_delete_project(self): self.assertFalse(Project.objects.filter(slug='pip').exists()) broadcast.assert_called_with( type='app', - task=tasks.remove_dir, - args=[project.doc_path]) + task=tasks.remove_dirs, + args=[(project.doc_path,)]) def test_subproject_create(self): project = get(Project, slug='pip', users=[self.user]) From 7eac9d348c641047cb954dba345a2507d7a3959b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 12 Dec 2018 19:56:29 -0500 Subject: [PATCH 2/3] Remove clear_artifacts --- readthedocs/builds/models.py | 9 +++++++-- readthedocs/projects/tasks.py | 5 ----- readthedocs/projects/views/private.py | 4 ++-- readthedocs/rtd_tests/tests/test_celery.py | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index a4b272abe35..9015e65677c 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -213,9 +213,14 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ def delete(self, *args, **kwargs): # pylint: disable=arguments-differ from readthedocs.projects import tasks log.info('Removing files for version %s', self.slug) - broadcast(type='app', task=tasks.clear_artifacts, args=[self.get_artifact_paths()]) broadcast( - type='app', task=tasks.symlink_project, args=[self.project.pk]) + type='app', + task=tasks.remove_dirs, + args=[self.get_artifact_paths()], + ) + broadcast( + type='app', task=tasks.symlink_project, args=[self.project.pk] + ) super(Version, self).delete(*args, **kwargs) @property diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index e88cdd6e027..d2117efdd91 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1337,11 +1337,6 @@ def remove_dirs(paths): shutil.rmtree(path, ignore_errors=True) -@app.task() -def clear_artifacts(paths): - remove_dirs(paths) - - @app.task(queue='web') def sync_callback(_, version_pk, commit, *args, **kwargs): """ diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 5c1de195eb6..dbb50dc350c 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -192,7 +192,7 @@ def project_version_detail(request, project_slug, version_slug): log.info('Removing files for version %s', version.slug) broadcast( type='app', - task=tasks.clear_artifacts, + task=tasks.remove_dirs, args=[version.get_artifact_paths()], ) version.built = False @@ -705,7 +705,7 @@ def project_version_delete_html(request, project_slug, version_slug): version.save() broadcast( type='app', - task=tasks.clear_artifacts, + task=tasks.remove_dirs, args=[version.get_artifact_paths()], ) else: diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index a52df435432..0e1d123bc42 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -58,14 +58,14 @@ def test_clear_artifacts(self): directory = self.project.get_production_media_path(type_='pdf', version_slug=version.slug) os.makedirs(directory) self.assertTrue(exists(directory)) - result = tasks.clear_artifacts.delay(paths=version.get_artifact_paths()) + result = tasks.remove_dirs.delay(paths=version.get_artifact_paths()) self.assertTrue(result.successful()) self.assertFalse(exists(directory)) directory = version.project.rtd_build_path(version=version.slug) os.makedirs(directory) self.assertTrue(exists(directory)) - result = tasks.clear_artifacts.delay(paths=version.get_artifact_paths()) + result = tasks.remove_dirs.delay(paths=version.get_artifact_paths()) self.assertTrue(result.successful()) self.assertFalse(exists(directory)) From 48cb36e5d2a3ff32892f386665b8777f3617fe1d Mon Sep 17 00:00:00 2001 From: Eric Holscher <25510+ericholscher@users.noreply.github.com> Date: Thu, 10 Jan 2019 15:19:00 -0500 Subject: [PATCH 3/3] Update docstring --- readthedocs/projects/tasks.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index b77462cd9df..1df662b4f5d 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1317,13 +1317,12 @@ def update_static_metadata(project_pk, path=None): @app.task() def remove_dirs(paths): """ - Remove artifacts from the web servers. + Remove artifacts from servers. - This is mainly a wrapper around shutil.rmtree so that app servers can kill - things on the build server. + This is mainly a wrapper around shutil.rmtree so that we can remove things across + every instance of a type of server (eg. all builds or all webs). - :param paths: list containing PATHs where production media is on disk - (usually ``Version.get_artifact_paths``) + :param paths: list containing PATHs where file is on disk """ for path in paths: log.info('Removing %s', path)