Merge pull request #4994 from stsewd/refactor-remove-dir

Refactor remove_dir

readthedocs/builds/models.py

@@ -215,7 +215,7 @@ def delete(self, *args, **kwargs):  # pylint: disable=arguments-differ
         log.info('Removing files for version %s', self.slug)
         broadcast(
             type='app',
-            task=tasks.clear_artifacts,
+            task=tasks.remove_dirs,
             args=[self.get_artifact_paths()],
         )
         project_pk = self.project.pk

readthedocs/core/views/__init__.py

@@ -19,7 +19,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__)
@@ -89,7 +89,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,

readthedocs/projects/admin.py

@@ -31,11 +31,11 @@
     WebHook,
 )
 from .notifications import (
-    ResourceUsageNotification,
     DeprecatedBuildWebhookNotification,
     DeprecatedGitHubWebhookNotification,
+    ResourceUsageNotification,
 )
-from .tasks import remove_dir
+from .tasks import remove_dirs
 
 
 class ProjectSendNotificationView(SendNotificationView):
@@ -182,7 +182,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):

readthedocs/projects/tasks.py

@@ -880,19 +880,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(
@@ -1327,27 +1327,18 @@ def update_static_metadata(project_pk, path=None):
 
 # Random Tasks
 @app.task()
-def remove_dir(path):
-    """
-    Remove a directory on the build/celery server.
-
-    This is mainly a wrapper around shutil.rmtree so that app servers can kill
-    things on the build server.
+def remove_dirs(paths):
     """
-    log.info('Removing %s', path)
-    shutil.rmtree(path, ignore_errors=True)
+    Remove artifacts from servers.
 
+    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).
 
-@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``)
+    :param paths: list containing PATHs where file is on disk
     """
     for path in paths:
-        remove_dir(path)
+        log.info('Removing %s', path)
+        shutil.rmtree(path, ignore_errors=True)
 
 
 @app.task(queue='web')

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
@@ -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')
@@ -704,7 +708,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:

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,)]
             ),
         ])

readthedocs/rtd_tests/tests/test_celery.py

@@ -47,10 +47,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))
 
@@ -59,14 +59,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))
 

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])