Refactor tasks into decorators (#4666)

* Refactor SyncRepositoryTask to decorator

* Refactor UpdateDocsTask to decorator

* Ignore some checks on pylint

Pylint doesn't like bounded tasks

* Fix lint

Author: stsewd

readthedocs/core/management/commands/pull.py

@@ -20,6 +20,4 @@ def handle(self, *args, **options):
         if args:
             for slug in args:
                 version = utils.version_from_slug(slug, LATEST)
-                tasks.SyncRepositoryTask().run(
-                    version.pk,
-                )
+                tasks.sync_repository_task(version.pk)

readthedocs/core/management/commands/update_api.py

@@ -33,5 +33,5 @@ def handle(self, *args, **options):
             project_data = api.project(slug).get()
             p = APIProject(**project_data)
             log.info("Building %s", p)
-            update_docs = tasks.UpdateDocsTask()
-            update_docs.run(pk=p.pk, docker=docker)
+            # pylint: disable=no-value-for-parameter
+            tasks.update_docs_task(p.pk, docker=docker)

readthedocs/core/management/commands/update_repos.py

@@ -73,8 +73,9 @@ def handle(self, *args, **options):
                             state='triggered',
                         )
 
-                        tasks.UpdateDocsTask().run(
-                            pk=version.project_id,
+                        # pylint: disable=no-value-for-parameter
+                        tasks.update_docs_task(
+                            version.project_id,
                             build_pk=build.pk,
                             version_pk=version.pk,
                         )
@@ -89,15 +90,17 @@ def handle(self, *args, **options):
                         active=True,
                         uploaded=False,
                 ):
-                    tasks.UpdateDocsTask().run(
-                        pk=version.project_id,
+                    # pylint: disable=no-value-for-parameter
+                    tasks.update_docs_task(
+                        version.project_id,
                         force=force,
                         version_pk=version.pk,
                     )
             else:
                 log.info('Updating all docs')
                 for project in Project.objects.all():
-                    tasks.UpdateDocsTask().run(
-                        pk=project.pk,
+                    # pylint: disable=no-value-for-parameter
+                    tasks.update_docs_task(
+                        project.pk,
                         force=force,
                     )

readthedocs/core/management/commands/update_versions.py

@@ -4,7 +4,7 @@
 from django.core.management.base import BaseCommand
 
 from readthedocs.builds.models import Version
-from readthedocs.projects.tasks import UpdateDocsTask
+from readthedocs.projects.tasks import update_docs_task
 
 
 class Command(BaseCommand):
@@ -13,6 +13,9 @@ class Command(BaseCommand):
 
     def handle(self, *args, **options):
         for version in Version.objects.filter(active=True, built=False):
-            update_docs = UpdateDocsTask()
-            update_docs.run(version.project_id, record=False,
-                            version_pk=version.pk)
+            # pylint: disable=no-value-for-parameter
+            update_docs_task(
+                version.project_id,
+                record=False,
+                version_pk=version.pk
+            )

readthedocs/core/utils/__init__.py

@@ -89,10 +89,10 @@ def prepare_build(
     :param record: whether or not record the build in a new Build object
     :param force: build the HTML documentation even if the files haven't changed
     :param immutable: whether or not create an immutable Celery signature
-    :returns: Celery signature of UpdateDocsTask to be executed
+    :returns: Celery signature of update_docs_task to be executed
     """
     # Avoid circular import
-    from readthedocs.projects.tasks import UpdateDocsTask
+    from readthedocs.projects.tasks import update_docs_task
     from readthedocs.builds.models import Build
 
     if project.skip:
@@ -138,9 +138,8 @@ def prepare_build(
     options['soft_time_limit'] = time_limit
     options['time_limit'] = int(time_limit * 1.2)
 
-    update_docs_task = UpdateDocsTask()
     return update_docs_task.signature(
-        (project.pk,),
+        args=(project.pk,),
         kwargs=kwargs,
         options=options,
         immutable=True,

readthedocs/core/views/hooks.py

@@ -12,7 +12,7 @@
 from readthedocs.builds.constants import LATEST
 from readthedocs.projects import constants
 from readthedocs.projects.models import Project, Feature
-from readthedocs.projects.tasks import SyncRepositoryTask
+from readthedocs.projects.tasks import sync_repository_task
 
 import logging
 
@@ -128,12 +128,9 @@ def _build_url(url, projects, branches):
     for project in projects:
         (built, not_building) = build_branches(project, branches)
         if not built:
-            # Call SyncRepositoryTask to update tag/branch info
+            # Call sync_repository_task to update tag/branch info
             version = project.versions.get(slug=LATEST)
-            sync_repository = SyncRepositoryTask()
-            sync_repository.apply_async(
-                args=(version.pk,),
-            )
+            sync_repository_task.delay(version.pk)
             msg = '(URL Build) Syncing versions for %s' % project.slug
             log.info(msg)
         all_built[project.slug] = built

readthedocs/projects/apps.py

@@ -5,10 +5,3 @@
 
 class ProjectsConfig(AppConfig):
     name = 'readthedocs.projects'
-
-    def ready(self):
-        from readthedocs.projects import tasks
-        from readthedocs.worker import app
-
-        app.tasks.register(tasks.SyncRepositoryTask)
-        app.tasks.register(tasks.UpdateDocsTask)

readthedocs/projects/tasks.py

@@ -21,7 +21,6 @@
 
 import requests
 from builtins import str
-from celery import Task
 from celery.exceptions import SoftTimeLimitExceeded
 from django.conf import settings
 from django.core.urlresolvers import reverse
@@ -173,19 +172,11 @@ def validate_duplicate_reserved_versions(self, data):
                 )
 
 
-# TODO SyncRepositoryTask should be refactored into a standard celery task,
-# there is no more need to have this be a separate class
-class SyncRepositoryTask(Task):
-
+@app.task(max_retries=5, default_retry_delay=7 * 60)
+def sync_repository_task(version_pk):
     """Celery task to trigger VCS version sync."""
-
-    max_retries = 5
-    default_retry_delay = (7 * 60)
-    name = __name__ + '.sync_repository'
-
-    def run(self, *args, **kwargs):
-        step = SyncRepositoryTaskStep()
-        return step.run(*args, **kwargs)
+    step = SyncRepositoryTaskStep()
+    return step.run(version_pk)
 
 
 class SyncRepositoryTaskStep(SyncRepositoryMixin):
@@ -226,17 +217,10 @@ def run(self, version_pk):  # pylint: disable=arguments-differ
         return False
 
 
-# TODO UpdateDocsTask should be refactored into a standard celery task,
-# there is no more need to have this be a separate class
-class UpdateDocsTask(Task):
-
-    max_retries = 5
-    default_retry_delay = (7 * 60)
-    name = __name__ + '.update_docs'
-
-    def run(self, *args, **kwargs):
-        step = UpdateDocsTaskStep(task=self)
-        return step.run(*args, **kwargs)
+@app.task(bind=True, max_retries=5, default_retry_delay=7 * 60)
+def update_docs_task(self, project_id, *args, **kwargs):
+    step = UpdateDocsTaskStep(task=self)
+    return step.run(project_id, *args, **kwargs)
 
 
 class UpdateDocsTaskStep(SyncRepositoryMixin):

readthedocs/rtd_tests/tests/test_celery.py

@@ -76,8 +76,7 @@ def test_update_docs(self):
         build = get(Build, project=self.project,
                     version=self.project.versions.first())
         with mock_api(self.repo) as mapi:
-            update_docs = tasks.UpdateDocsTask()
-            result = update_docs.delay(
+            result = tasks.update_docs_task.delay(
                 self.project.pk,
                 build_pk=build.pk,
                 record=False,
@@ -94,8 +93,7 @@ def test_update_docs_unexpected_setup_exception(self, mock_setup_vcs):
         build = get(Build, project=self.project,
                     version=self.project.versions.first())
         with mock_api(self.repo) as mapi:
-            update_docs = tasks.UpdateDocsTask()
-            result = update_docs.delay(
+            result = tasks.update_docs_task.delay(
                 self.project.pk,
                 build_pk=build.pk,
                 record=False,
@@ -112,8 +110,7 @@ def test_update_docs_unexpected_build_exception(self, mock_build_docs):
         build = get(Build, project=self.project,
                     version=self.project.versions.first())
         with mock_api(self.repo) as mapi:
-            update_docs = tasks.UpdateDocsTask()
-            result = update_docs.delay(
+            result = tasks.update_docs_task.delay(
                 self.project.pk,
                 build_pk=build.pk,
                 record=False,
@@ -123,10 +120,7 @@ def test_update_docs_unexpected_build_exception(self, mock_build_docs):
     def test_sync_repository(self):
         version = self.project.versions.get(slug=LATEST)
         with mock_api(self.repo):
-            sync_repository = tasks.SyncRepositoryTask()
-            result = sync_repository.apply_async(
-                args=(version.pk,),
-            )
+            result = tasks.sync_repository_task.delay(version.pk)
         self.assertTrue(result.successful())
 
     @patch('readthedocs.projects.tasks.api_v2')

readthedocs/rtd_tests/tests/test_core_utils.py

@@ -18,7 +18,7 @@ def setUp(self):
         self.project = get(Project, container_time_limit=None)
         self.version = get(Version, project=self.project)
 
-    @mock.patch('readthedocs.projects.tasks.UpdateDocsTask')
+    @mock.patch('readthedocs.projects.tasks.update_docs_task')
     def test_trigger_custom_queue(self, update_docs):
         """Use a custom queue when routing the task"""
         self.project.build_queue = 'build03'
@@ -34,17 +34,17 @@ def test_trigger_custom_queue(self, update_docs):
             'time_limit': 720,
             'soft_time_limit': 600,
         }
-        update_docs().signature.assert_has_calls([
+        update_docs.signature.assert_has_calls([
             mock.call(
-                (self.project.pk,),
+                args=(self.project.pk,),
                 kwargs=kwargs,
                 options=options,
                 immutable=True,
             ),
         ])
-        update_docs().signature().apply_async.assert_called()
+        update_docs.signature().apply_async.assert_called()
 
-    @mock.patch('readthedocs.projects.tasks.UpdateDocsTask')
+    @mock.patch('readthedocs.projects.tasks.update_docs_task')
     def test_trigger_build_time_limit(self, update_docs):
         """Pass of time limit"""
         trigger_build(project=self.project, version=self.version)
@@ -59,17 +59,17 @@ def test_trigger_build_time_limit(self, update_docs):
             'time_limit': 720,
             'soft_time_limit': 600,
         }
-        update_docs().signature.assert_has_calls([
+        update_docs.signature.assert_has_calls([
             mock.call(
-                (self.project.pk,),
+                args=(self.project.pk,),
                 kwargs=kwargs,
                 options=options,
                 immutable=True,
             ),
         ])
-        update_docs().signature().apply_async.assert_called()
+        update_docs.signature().apply_async.assert_called()
 
-    @mock.patch('readthedocs.projects.tasks.UpdateDocsTask')
+    @mock.patch('readthedocs.projects.tasks.update_docs_task')
     def test_trigger_build_invalid_time_limit(self, update_docs):
         """Time limit as string"""
         self.project.container_time_limit = '200s'
@@ -85,17 +85,17 @@ def test_trigger_build_invalid_time_limit(self, update_docs):
             'time_limit': 720,
             'soft_time_limit': 600,
         }
-        update_docs().signature.assert_has_calls([
+        update_docs.signature.assert_has_calls([
             mock.call(
-                (self.project.pk,),
+                args=(self.project.pk,),
                 kwargs=kwargs,
                 options=options,
                 immutable=True,
             ),
         ])
-        update_docs().signature().apply_async.assert_called()
+        update_docs.signature().apply_async.assert_called()
 
-    @mock.patch('readthedocs.projects.tasks.UpdateDocsTask')
+    @mock.patch('readthedocs.projects.tasks.update_docs_task')
     def test_trigger_build_rounded_time_limit(self, update_docs):
         """Time limit should round down"""
         self.project.container_time_limit = 3
@@ -111,15 +111,15 @@ def test_trigger_build_rounded_time_limit(self, update_docs):
             'time_limit': 3,
             'soft_time_limit': 3,
         }
-        update_docs().signature.assert_has_calls([
+        update_docs.signature.assert_has_calls([
             mock.call(
-                (self.project.pk,),
+                args=(self.project.pk,),
                 kwargs=kwargs,
                 options=options,
                 immutable=True,
             ),
         ])
-        update_docs().signature().apply_async.assert_called()
+        update_docs.signature().apply_async.assert_called()
 
     def test_slugify(self):
         """Test additional slugify"""

readthedocs/rtd_tests/tests/test_privacy.py

@@ -27,7 +27,7 @@ def setUp(self):
         self.tester.set_password('test')
         self.tester.save()
 
-        tasks.UpdateDocsTask.delay = mock.Mock()
+        tasks.update_docs_task.delay = mock.Mock()
 
     def _create_kong(self, privacy_level='private',
                      version_privacy_level='private'):