From 547cfb00453c767269a4e2a1986e0cecd6df263f Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Wed, 8 Nov 2017 12:47:43 -0700 Subject: [PATCH 01/16] Clean up logging around exceptions --- readthedocs/builds/models.py | 4 ++-- .../core/management/commands/reindex_elasticsearch.py | 2 +- readthedocs/core/management/commands/set_metadata.py | 2 +- readthedocs/core/views/hooks.py | 10 +++++----- readthedocs/doc_builder/environments.py | 2 +- readthedocs/oauth/services/github.py | 2 +- readthedocs/projects/models.py | 10 +++++----- readthedocs/projects/utils.py | 4 ++-- readthedocs/search/utils.py | 2 +- readthedocs/vcs_support/utils.py | 3 +-- 10 files changed, 20 insertions(+), 21 deletions(-) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index f3927178987..10d8a9e5aa6 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -155,7 +155,7 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ try: self.project.sync_supported_versions() except Exception: - log.error('failed to sync supported versions', exc_info=True) + log.exception('failed to sync supported versions') broadcast(type='app', task=tasks.symlink_project, args=[self.project.pk]) return obj @@ -224,7 +224,7 @@ def clean_build_path(self): path, self)) rmtree(path) except OSError: - log.error('Build path cleanup failed', exc_info=True) + log.exception('Build path cleanup failed') def get_github_url(self, docroot, filename, source_suffix='.rst', action='view'): """ diff --git a/readthedocs/core/management/commands/reindex_elasticsearch.py b/readthedocs/core/management/commands/reindex_elasticsearch.py index 731dc861af6..e1094068b3e 100644 --- a/readthedocs/core/management/commands/reindex_elasticsearch.py +++ b/readthedocs/core/management/commands/reindex_elasticsearch.py @@ -53,4 +53,4 @@ def handle(self, *args, **options): update_search(version.pk, commit, delete_non_commit_files=False) except Exception: - log.error('Reindex failed for %s', version, exc_info=True) + log.exception('Reindex failed for %s'.format(version)) diff --git a/readthedocs/core/management/commands/set_metadata.py b/readthedocs/core/management/commands/set_metadata.py index 2f8014cf828..dbefcddbdd0 100644 --- a/readthedocs/core/management/commands/set_metadata.py +++ b/readthedocs/core/management/commands/set_metadata.py @@ -23,4 +23,4 @@ def handle(self, *args, **options): try: broadcast(type='app', task=tasks.update_static_metadata, args=[p.pk]) except Exception: - log.error('Build failed for %s', p, exc_info=True) + log.exception('Build failed for %s', p) diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index 3525fb88177..2a438f27da8 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -178,7 +178,7 @@ def github_build(request): # noqa: D205 ssh_search_url = ssh_url.replace('git@', '').replace('.git', '') branches = [data['ref'].replace('refs/heads/', '')] except (ValueError, TypeError, KeyError): - log.error('Invalid GitHub webhook payload', exc_info=True) + log.exception('Invalid GitHub webhook payload') return HttpResponse('Invalid request', status=400) try: repo_projects = get_project_from_url(http_search_url) @@ -198,7 +198,7 @@ def github_build(request): # noqa: D205 projects = repo_projects | ssh_projects return _build_url(http_search_url, projects, branches) except NoProjectException: - log.error('Project match not found: url=%s', http_search_url) + log.exception('Project match not found: url=%s', http_search_url) return HttpResponseNotFound('Project not found') else: return HttpResponse('Method not allowed, POST is required', status=405) @@ -222,7 +222,7 @@ def gitlab_build(request): # noqa: D205 search_url = re.sub(r'^https?://(.*?)(?:\.git|)$', '\\1', url) branches = [data['ref'].replace('refs/heads/', '')] except (ValueError, TypeError, KeyError): - log.error('Invalid GitLab webhook payload', exc_info=True) + log.exception('Invalid GitLab webhook payload') return HttpResponse('Invalid request', status=400) log.info( 'GitLab webhook search: url=%s branches=%s', @@ -281,7 +281,7 @@ def bitbucket_build(request): data['repository']['full_name'] ) except (TypeError, ValueError, KeyError): - log.error('Invalid Bitbucket webhook payload', exc_info=True) + log.exception('Invalid Bitbucket webhook payload') return HttpResponse('Invalid request', status=400) log.info( @@ -320,7 +320,7 @@ def generic_build(request, project_id_or_slug=None): try: project = Project.objects.get(slug=project_id_or_slug) except (Project.DoesNotExist, ValueError): - log.error( + log.exception( "(Incoming Generic Build) Repo not found: %s", project_id_or_slug) return HttpResponseNotFound( diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 3fa97b0d036..7fb8f7e592e 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -433,7 +433,7 @@ def update_build(self, state=None): except HttpClientError as e: log.error("Unable to post a new build: %s", e.content) except Exception: - log.error("Unknown build exception", exc_info=True) + log.exception("Unknown build exception") class LocalEnvironment(BuildEnvironment): diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index a0c630b1a3a..9472f20825f 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -289,5 +289,5 @@ def get_token_for_project(cls, project, force_local=False): if tokens.exists(): token = tokens[0].token except Exception: - log.error('Failed to get token for project', exc_info=True) + log.exception('Failed to get token for project') return token diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index f7c3adb32d5..9dc5a682367 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -315,29 +315,29 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ latest.identifier = self.default_branch latest.save() except Exception: - log.error('Failed to update latest identifier', exc_info=True) + log.exception('Failed to update latest identifier') # Add exceptions here for safety try: self.sync_supported_versions() except Exception: - log.error('failed to sync supported versions', exc_info=True) + log.exception('failed to sync supported versions') try: if not first_save: broadcast(type='app', task=tasks.symlink_project, args=[self.pk]) except Exception: - log.error('failed to symlink project', exc_info=True) + log.exception('failed to symlink project') try: if not first_save: broadcast(type='app', task=tasks.update_static_metadata, args=[self.pk]) except Exception: - log.error('failed to update static metadata', exc_info=True) + log.exception('failed to update static metadata') try: branch = self.default_branch or self.vcs_repo().fallback_branch if not self.versions.filter(slug=LATEST).exists(): self.versions.create_latest(identifier=branch) except Exception: - log.error('Error creating default branches', exc_info=True) + log.exception('Error creating default branches') def get_absolute_url(self): return reverse('projects_detail', args=[self.slug]) diff --git a/readthedocs/projects/utils.py b/readthedocs/projects/utils.py index 86e2040d03c..2683ec59cfb 100644 --- a/readthedocs/projects/utils.py +++ b/readthedocs/projects/utils.py @@ -84,7 +84,7 @@ def run(*commands): command = ' '.join(command) except TypeError: run_command = command - log.info('Running command: cwd=%s command=%s', cwd, command) + log.debug('Running command: cwd=%s command=%s', cwd, command) try: p = subprocess.Popen( run_command, @@ -100,7 +100,7 @@ def run(*commands): out = '' err = traceback.format_exc() ret = -1 - log.error("Command failed", exc_info=True) + log.exception("Command failed") return (ret, out, err) diff --git a/readthedocs/search/utils.py b/readthedocs/search/utils.py index 07eb38cd1f8..385e1110804 100644 --- a/readthedocs/search/utils.py +++ b/readthedocs/search/utils.py @@ -266,7 +266,7 @@ def parse_mkdocs_sections(content): # we're unsure which exceptions can be raised # pylint: disable=bare-except except: - log.error('Failed indexing', exc_info=True) + log.exception('Failed indexing') def parse_sections(documentation_type, content): diff --git a/readthedocs/vcs_support/utils.py b/readthedocs/vcs_support/utils.py index f4e2018a010..5dd59c80f1b 100644 --- a/readthedocs/vcs_support/utils.py +++ b/readthedocs/vcs_support/utils.py @@ -57,8 +57,7 @@ def __exit__(self, exc, value, tb): log.info("Lock (%s): Releasing", self.name) os.remove(self.fpath) except OSError: - log.error("Lock (%s): Failed to release, ignoring...", self.name, - exc_info=True) + log.exception("Lock (%s): Failed to release, ignoring...", self.name) class NonBlockingLock(object): From 94df0ccef9dafcbef8e05159b5943a28a08360b3 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Wed, 8 Nov 2017 16:19:42 -0700 Subject: [PATCH 02/16] Another one --- readthedocs/projects/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index a52fa80c3af..d197520e203 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -624,7 +624,7 @@ def move_files(version_pk, hostname, html=False, localmedia=False, search=False, """ version = Version.objects.get(pk=version_pk) log.debug(LOG_TEMPLATE.format(project=version.project.slug, version=version.slug, - msg='Moving files: {}'.format(locals()))) + msg='Moving files')) if html: from_path = version.project.artifact_path( From fc8728798a22a225d65728aa8b0e02aee9989741 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Wed, 8 Nov 2017 17:00:45 -0700 Subject: [PATCH 03/16] Fix lint --- readthedocs/core/management/commands/reindex_elasticsearch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/core/management/commands/reindex_elasticsearch.py b/readthedocs/core/management/commands/reindex_elasticsearch.py index e1094068b3e..a2bce6df840 100644 --- a/readthedocs/core/management/commands/reindex_elasticsearch.py +++ b/readthedocs/core/management/commands/reindex_elasticsearch.py @@ -53,4 +53,4 @@ def handle(self, *args, **options): update_search(version.pk, commit, delete_non_commit_files=False) except Exception: - log.exception('Reindex failed for %s'.format(version)) + log.exception('Reindex failed for {}'.format(version)) From b5af4b619136f0b47ac155c8e002218a461f8c25 Mon Sep 17 00:00:00 2001 From: anatoly techtonik Date: Fri, 10 Nov 2017 15:52:11 +0300 Subject: [PATCH 04/16] Rearrange development docs ToC This is to make architecture the top link after changelog and installation instructions. Also moves API to the bottom, because it stands out of the reading flow, which is: - see if the development thing you think about is already done - launch installation if you're going to fix anything - (or) read about archtecture - about testing, probably installation is ready - how to sync docs - standards, etc. - components (should be linked from archtecture) - everything else - leave API pages open --- docs/index.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/index.rst b/docs/index.rst index 10675f8321f..24ead577682 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -92,18 +92,18 @@ Information about development is also available: :maxdepth: 2 :caption: Developer Documentation - install - api/index changelog + install + architecture tests docs - architecture development/standards development/buildenvironments symlinks settings i18n issue-labels + api/index .. _business-docs: From 96b23c891581b5f135589fec031230ba7e7a1899 Mon Sep 17 00:00:00 2001 From: Anthony Date: Fri, 10 Nov 2017 17:17:36 -0700 Subject: [PATCH 05/16] Use new Celery, use new application pattern (#3237) * Use new Celery, use new application pattern * Use modern celery * Drop djcelery * New pattern for starting celery * Bump redis to 2.10.6 to avoid startup bug, change autodiscover call * Update docs mentioning Celery * Goof on package name * Missed djcelery import * Fix rebased task that was missed * Handle change to Celery group calls in 4.x * Fix tests * Fix up some linting issues and problems starting the application * Fix celery task registration * We don't need shared_task anymore, swap for readthedocs.worker.app.task * Fix call to chord --- docs/faq.rst | 4 +- readthedocs/__init__.py | 4 ++ .../core/management/commands/update_api.py | 3 +- .../core/management/commands/update_repos.py | 25 +++++++---- .../management/commands/update_versions.py | 3 +- readthedocs/core/tasks.py | 5 ++- readthedocs/core/utils/__init__.py | 13 ++++-- readthedocs/core/utils/tasks/retrieve.py | 5 ++- readthedocs/oauth/tasks.py | 3 +- readthedocs/projects/__init__.py | 1 + readthedocs/projects/apps.py | 12 ++++++ readthedocs/projects/tasks.py | 43 +++++++++---------- .../tests/projects/test_admin_actions.py | 9 ++-- readthedocs/rtd_tests/tests/test_celery.py | 9 ++-- .../rtd_tests/tests/test_core_utils.py | 18 ++++---- .../rtd_tests/tests/test_privacy_urls.py | 13 +++--- readthedocs/settings/base.py | 7 --- readthedocs/worker.py | 19 ++++++++ requirements/pip.txt | 5 +-- 19 files changed, 125 insertions(+), 76 deletions(-) create mode 100644 readthedocs/projects/apps.py create mode 100644 readthedocs/worker.py diff --git a/docs/faq.rst b/docs/faq.rst index c6ad95f4690..0410cc65a13 100644 --- a/docs/faq.rst +++ b/docs/faq.rst @@ -104,7 +104,7 @@ If you add a subproject to a project, that documentation will also be served under the parent project's subdomain. For example, -Kombu is a subproject of celery, +Kombu is a subproject of Celery, so you can access it on the `celery.readthedocs.io` domain: http://celery.readthedocs.io/projects/kombu/en/latest/ @@ -204,4 +204,4 @@ file* field. What commit of Read the Docs is in production? ---------------------------------------------- -We deploy readthedocs.org from the `rel` branch in our GitHub repository. You can see the latest commits that have been deployed by looking on GitHub: https://github.com/rtfd/readthedocs.org/commits/rel \ No newline at end of file +We deploy readthedocs.org from the `rel` branch in our GitHub repository. You can see the latest commits that have been deployed by looking on GitHub: https://github.com/rtfd/readthedocs.org/commits/rel diff --git a/readthedocs/__init__.py b/readthedocs/__init__.py index e69de29bb2d..1343a18d341 100644 --- a/readthedocs/__init__.py +++ b/readthedocs/__init__.py @@ -0,0 +1,4 @@ +"""Read the Docs""" + +# Import the Celery application before anything else happens +from readthedocs.worker import app # noqa diff --git a/readthedocs/core/management/commands/update_api.py b/readthedocs/core/management/commands/update_api.py index 27d176f5ebb..e8433513bdd 100644 --- a/readthedocs/core/management/commands/update_api.py +++ b/readthedocs/core/management/commands/update_api.py @@ -33,4 +33,5 @@ def handle(self, *args, **options): project_data = api.project(slug).get() p = APIProject(**project_data) log.info("Building %s", p) - tasks.update_docs.run(pk=p.pk, docker=docker) + update_docs = tasks.UpdateDocsTask() + update_docs.run(pk=p.pk, docker=docker) diff --git a/readthedocs/core/management/commands/update_repos.py b/readthedocs/core/management/commands/update_repos.py index 142ad19e5e2..536e4c018e2 100644 --- a/readthedocs/core/management/commands/update_repos.py +++ b/readthedocs/core/management/commands/update_repos.py @@ -54,9 +54,11 @@ def handle(self, *args, **options): for version in Version.objects.filter(project__slug=slug, active=True, uploaded=False): - tasks.update_docs.run(pk=version.project_id, - record=False, - version_pk=version.pk) + tasks.UpdateDocsTask().run( + pk=version.project_id, + record=False, + version_pk=version.pk + ) else: p = Project.all_objects.get(slug=slug) log.info("Building %s", p) @@ -66,12 +68,17 @@ def handle(self, *args, **options): log.info("Updating all versions") for version in Version.objects.filter(active=True, uploaded=False): - tasks.update_docs.run(pk=version.project_id, - record=record, - force=force, - version_pk=version.pk) + tasks.UpdateDocsTask().run( + pk=version.project_id, + record=record, + force=force, + version_pk=version.pk + ) else: log.info("Updating all docs") for project in Project.objects.all(): - tasks.update_docs.run(pk=project.pk, record=record, - force=force) + tasks.UpdateDocsTask().run( + pk=project.pk, + record=record, + force=force + ) diff --git a/readthedocs/core/management/commands/update_versions.py b/readthedocs/core/management/commands/update_versions.py index 88e8d7c410c..c3ae6fa8e1b 100644 --- a/readthedocs/core/management/commands/update_versions.py +++ b/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 update_docs +from readthedocs.projects.tasks import UpdateDocsTask class Command(BaseCommand): @@ -13,5 +13,6 @@ 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) diff --git a/readthedocs/core/tasks.py b/readthedocs/core/tasks.py index 7c57cd90a4f..e5b67355e3f 100644 --- a/readthedocs/core/tasks.py +++ b/readthedocs/core/tasks.py @@ -3,19 +3,20 @@ from __future__ import absolute_import import logging -from celery import task from django.conf import settings from django.core.mail import EmailMultiAlternatives from django.template.loader import get_template from django.template import TemplateDoesNotExist +from readthedocs.worker import app + log = logging.getLogger(__name__) EMAIL_TIME_LIMIT = 30 -@task(queue='web', time_limit=EMAIL_TIME_LIMIT) +@app.task(queue='web', time_limit=EMAIL_TIME_LIMIT) def send_email_task(recipient, subject, template, template_html, context=None): """Send multipart email diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index fca8e13f96f..80e5d90631b 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -49,9 +49,15 @@ def broadcast(type, task, args, kwargs=None, callback=None): # pylint: disable= task_sig = task.s(*args, **kwargs).set(queue=server) tasks.append(task_sig) if callback: - task_promise = chord(tasks)(callback).get() + task_promise = chord(tasks, callback).apply_async() else: - task_promise = group(*tasks).apply_async() + # Celery's Group class does some special handling when an iterable with + # len() == 1 is passed in. This will be hit if there is only one server + # defined in the above queue lists + if len(tasks) > 1: + task_promise = group(*tasks).apply_async() + else: + task_promise = group(tasks).apply_async() return task_promise @@ -77,7 +83,7 @@ def trigger_build(project, version=None, record=True, force=False, basic=False): will be prefixed with ``build-`` to unify build queue names. """ # Avoid circular import - from readthedocs.projects.tasks import update_docs + from readthedocs.projects.tasks import UpdateDocsTask from readthedocs.builds.models import Build if project.skip: @@ -121,6 +127,7 @@ def trigger_build(project, version=None, record=True, force=False, basic=False): options['soft_time_limit'] = time_limit options['time_limit'] = int(time_limit * 1.2) + update_docs = UpdateDocsTask() update_docs.apply_async(kwargs=kwargs, **options) return build diff --git a/readthedocs/core/utils/tasks/retrieve.py b/readthedocs/core/utils/tasks/retrieve.py index f33afab8094..9da3c581601 100644 --- a/readthedocs/core/utils/tasks/retrieve.py +++ b/readthedocs/core/utils/tasks/retrieve.py @@ -1,7 +1,6 @@ """Utilities for retrieving task data.""" from __future__ import absolute_import -from djcelery import celery as celery_app from celery.result import AsyncResult @@ -20,6 +19,8 @@ def get_task_data(task_id): meta data has no ``'task_name'`` key set. """ + from readthedocs.worker import app + result = AsyncResult(task_id) state, info = result.state, result.info if state == 'PENDING': @@ -27,7 +28,7 @@ def get_task_data(task_id): if 'task_name' not in info: raise TaskNotFound(task_id) try: - task = celery_app.tasks[info['task_name']] + task = app.tasks[info['task_name']] except KeyError: raise TaskNotFound(task_id) return task, state, info diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index b345d443046..7f95f9610ea 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -2,7 +2,6 @@ from __future__ import absolute_import from django.contrib.auth.models import User -from djcelery import celery as celery_app from readthedocs.core.utils.tasks import PublicTask from readthedocs.core.utils.tasks import permission_check @@ -22,4 +21,4 @@ def run_public(self, user_id): service.sync() -sync_remote_repositories = celery_app.tasks[SyncRemoteRepositories.name] +sync_remote_repositories = SyncRemoteRepositories() diff --git a/readthedocs/projects/__init__.py b/readthedocs/projects/__init__.py index e69de29bb2d..ff5ded49b17 100644 --- a/readthedocs/projects/__init__.py +++ b/readthedocs/projects/__init__.py @@ -0,0 +1 @@ +default_app_config = 'readthedocs.projects.apps.ProjectsConfig' diff --git a/readthedocs/projects/apps.py b/readthedocs/projects/apps.py new file mode 100644 index 00000000000..71111f4e8d0 --- /dev/null +++ b/readthedocs/projects/apps.py @@ -0,0 +1,12 @@ +"""Project app config""" + +from django.apps import AppConfig + + +class ProjectsConfig(AppConfig): + name = 'readthedocs.projects' + + def ready(self): + from readthedocs.projects.tasks import UpdateDocsTask + from readthedocs.worker import app + app.tasks.register(UpdateDocsTask) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index d197520e203..f5052b34697 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -16,12 +16,11 @@ import requests from builtins import str -from celery import task, Task +from celery import Task from celery.exceptions import SoftTimeLimitExceeded from django.conf import settings from django.core.urlresolvers import reverse from django.utils.translation import ugettext_lazy as _ -from djcelery import celery as celery_app from readthedocs_build.config import ConfigError from slumber.exceptions import HttpClientError @@ -53,6 +52,7 @@ from readthedocs.search.parse_json import process_all_json_files from readthedocs.search.utils import process_mkdocs_json from readthedocs.vcs_support import utils as vcs_support_utils +from readthedocs.worker import app log = logging.getLogger(__name__) @@ -80,7 +80,7 @@ class UpdateDocsTask(Task): max_retries = 5 default_retry_delay = (7 * 60) - name = 'update_docs' + name = __name__ + '.update_docs' def __init__(self, build_env=None, python_env=None, config=None, force=False, search=True, localmedia=True, @@ -490,10 +490,7 @@ def send_notifications(self): send_notifications.delay(self.version.pk, build_pk=self.build['id']) -update_docs = celery_app.tasks[UpdateDocsTask.name] - - -@task() +@app.task() def update_imported_docs(version_pk): """ Check out or update the given project's repository @@ -572,7 +569,7 @@ def update_imported_docs(version_pk): # Web tasks -@task(queue='web') +@app.task(queue='web') def sync_files(project_pk, version_pk, hostname=None, html=False, localmedia=False, search=False, pdf=False, epub=False): """Sync build artifacts to application instances @@ -604,7 +601,7 @@ def sync_files(project_pk, version_pk, hostname=None, html=False, update_static_metadata(project_pk) -@task(queue='web') +@app.task(queue='web') def move_files(version_pk, hostname, html=False, localmedia=False, search=False, pdf=False, epub=False): """Task to move built documentation to web servers @@ -670,7 +667,7 @@ def move_files(version_pk, hostname, html=False, localmedia=False, search=False, Syncer.copy(from_path, to_path, host=hostname) -@task(queue='web') +@app.task(queue='web') def update_search(version_pk, commit, delete_non_commit_files=True): """Task to update search indexes @@ -705,7 +702,7 @@ def update_search(version_pk, commit, delete_non_commit_files=True): ) -@task(queue='web') +@app.task(queue='web') def symlink_project(project_pk): project = Project.objects.get(pk=project_pk) for symlink in [PublicSymlink, PrivateSymlink]: @@ -713,7 +710,7 @@ def symlink_project(project_pk): sym.run() -@task(queue='web') +@app.task(queue='web') def symlink_domain(project_pk, domain_pk, delete=False): project = Project.objects.get(pk=project_pk) domain = Domain.objects.get(pk=domain_pk) @@ -725,7 +722,7 @@ def symlink_domain(project_pk, domain_pk, delete=False): sym.symlink_cnames(domain) -@task(queue='web') +@app.task(queue='web') def symlink_subproject(project_pk): project = Project.objects.get(pk=project_pk) for symlink in [PublicSymlink, PrivateSymlink]: @@ -733,7 +730,7 @@ def symlink_subproject(project_pk): sym.symlink_subprojects() -@task(queue='web') +@app.task(queue='web') def fileify(version_pk, commit): """ Create ImportedFile objects for all of a version's files. @@ -809,7 +806,7 @@ def _manage_imported_files(version, path, commit): purge(cdn_ids[version.project.slug], changed_files) -@task(queue='web') +@app.task(queue='web') def send_notifications(version_pk, build_pk): version = Version.objects.get(pk=version_pk) build = Build.objects.get(pk=build_pk) @@ -878,7 +875,7 @@ def webhook_notification(version, build, hook_url): requests.post(hook_url, data=data) -@task(queue='web') +@app.task(queue='web') def update_static_metadata(project_pk, path=None): """Update static metadata JSON file @@ -924,7 +921,7 @@ def update_static_metadata(project_pk, path=None): # Random Tasks -@task() +@app.task() def remove_dir(path): """ Remove a directory on the build/celery server. @@ -936,7 +933,7 @@ def remove_dir(path): shutil.rmtree(path, ignore_errors=True) -@task() +@app.task() def clear_artifacts(version_pk): """Remove artifacts from the web servers""" version = Version.objects.get(pk=version_pk) @@ -946,7 +943,7 @@ def clear_artifacts(version_pk): clear_html_artifacts(version) -@task() +@app.task() def clear_pdf_artifacts(version): if isinstance(version, int): version = Version.objects.get(pk=version) @@ -954,7 +951,7 @@ def clear_pdf_artifacts(version): type_='pdf', version_slug=version.slug)) -@task() +@app.task() def clear_epub_artifacts(version): if isinstance(version, int): version = Version.objects.get(pk=version) @@ -962,7 +959,7 @@ def clear_epub_artifacts(version): type_='epub', version_slug=version.slug)) -@task() +@app.task() def clear_htmlzip_artifacts(version): if isinstance(version, int): version = Version.objects.get(pk=version) @@ -970,14 +967,14 @@ def clear_htmlzip_artifacts(version): type_='htmlzip', version_slug=version.slug)) -@task() +@app.task() def clear_html_artifacts(version): if isinstance(version, int): version = Version.objects.get(pk=version) remove_dir(version.project.rtd_build_path(version=version.slug)) -@task(queue='web') +@app.task(queue='web') def sync_callback(_, version_pk, commit, *args, **kwargs): """ This will be called once the sync_files tasks are done. diff --git a/readthedocs/rtd_tests/tests/projects/test_admin_actions.py b/readthedocs/rtd_tests/tests/projects/test_admin_actions.py index b0f5526b331..7bda4501bf7 100644 --- a/readthedocs/rtd_tests/tests/projects/test_admin_actions.py +++ b/readthedocs/rtd_tests/tests/projects/test_admin_actions.py @@ -57,9 +57,10 @@ def test_project_ban_multiple_owners(self): self.assertFalse(self.project.users.filter(profile__banned=True).exists()) self.assertEqual(self.project.users.filter(profile__banned=False).count(), 2) - @mock.patch('readthedocs.projects.admin.remove_dir') - def test_project_delete(self, remove_dir): + @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 action_data = { ACTION_CHECKBOX_NAME: [self.project.pk], 'action': 'delete_selected', @@ -71,8 +72,8 @@ def test_project_delete(self, remove_dir): action_data ) self.assertFalse(Project.objects.filter(pk=self.project.pk).exists()) - remove_dir.s.assert_has_calls([ + broadcast.assert_has_calls([ mock.call( - self.project.doc_path, + type='app', task=remove_dir, args=[self.project.doc_path] ), ]) diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index d73c2c58fca..b4ac4530152 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -72,7 +72,8 @@ def test_update_docs(self): build = get(Build, project=self.project, version=self.project.versions.first()) with mock_api(self.repo) as mapi: - result = tasks.update_docs.delay( + update_docs = tasks.UpdateDocsTask() + result = update_docs.delay( self.project.pk, build_pk=build.pk, record=False, @@ -89,7 +90,8 @@ def test_update_docs_unexpected_setup_exception(self, mock_update_build, mock_se build = get(Build, project=self.project, version=self.project.versions.first()) with mock_api(self.repo) as mapi: - result = tasks.update_docs.delay( + update_docs = tasks.UpdateDocsTask() + result = update_docs.delay( self.project.pk, build_pk=build.pk, record=False, @@ -107,7 +109,8 @@ def test_update_docs_unexpected_build_exception(self, mock_update_build, mock_bu build = get(Build, project=self.project, version=self.project.versions.first()) with mock_api(self.repo) as mapi: - result = tasks.update_docs.delay( + update_docs = tasks.UpdateDocsTask() + result = update_docs.delay( self.project.pk, build_pk=build.pk, record=False, diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index b48795e8807..dc3424b93e8 100644 --- a/readthedocs/rtd_tests/tests/test_core_utils.py +++ b/readthedocs/rtd_tests/tests/test_core_utils.py @@ -18,12 +18,12 @@ def setUp(self): self.project = get(Project, container_time_limit=None) self.version = get(Version, project=self.project) - @mock.patch('readthedocs.projects.tasks.update_docs') + @mock.patch('readthedocs.projects.tasks.UpdateDocsTask') def test_trigger_build_time_limit(self, update_docs): """Pass of time limit""" trigger_build(project=self.project, version=self.version) - update_docs.assert_has_calls([ - mock.call.apply_async( + update_docs().apply_async.assert_has_calls([ + mock.call( time_limit=720, soft_time_limit=600, queue=mock.ANY, @@ -38,13 +38,13 @@ def test_trigger_build_time_limit(self, update_docs): ) ]) - @mock.patch('readthedocs.projects.tasks.update_docs') + @mock.patch('readthedocs.projects.tasks.UpdateDocsTask') def test_trigger_build_invalid_time_limit(self, update_docs): """Time limit as string""" self.project.container_time_limit = '200s' trigger_build(project=self.project, version=self.version) - update_docs.assert_has_calls([ - mock.call.apply_async( + update_docs().apply_async.assert_has_calls([ + mock.call( time_limit=720, soft_time_limit=600, queue=mock.ANY, @@ -59,13 +59,13 @@ def test_trigger_build_invalid_time_limit(self, update_docs): ) ]) - @mock.patch('readthedocs.projects.tasks.update_docs') + @mock.patch('readthedocs.projects.tasks.UpdateDocsTask') def test_trigger_build_rounded_time_limit(self, update_docs): """Time limit should round down""" self.project.container_time_limit = 3 trigger_build(project=self.project, version=self.version) - update_docs.assert_has_calls([ - mock.call.apply_async( + update_docs().apply_async.assert_has_calls([ + mock.call( time_limit=3, soft_time_limit=3, queue=mock.ANY, diff --git a/readthedocs/rtd_tests/tests/test_privacy_urls.py b/readthedocs/rtd_tests/tests/test_privacy_urls.py index b9fa7c0eaeb..e992a55ae1c 100644 --- a/readthedocs/rtd_tests/tests/test_privacy_urls.py +++ b/readthedocs/rtd_tests/tests/test_privacy_urls.py @@ -1,22 +1,23 @@ from __future__ import absolute_import from __future__ import print_function -from builtins import object import re +from builtins import object from django.contrib.admindocs.views import extract_views_from_urlpatterns from django.test import TestCase from django.core.urlresolvers import reverse +from django_dynamic_fixture import get +import mock +from taggit.models import Tag from readthedocs.builds.models import Build, VersionAlias, BuildCommandResult from readthedocs.comments.models import DocumentComment, NodeSnapshot +from readthedocs.core.utils.tasks import TaskNoPermission from readthedocs.integrations.models import HttpExchange, Integration from readthedocs.projects.models import Project, Domain from readthedocs.oauth.models import RemoteRepository, RemoteOrganization from readthedocs.rtd_tests.utils import create_user -from django_dynamic_fixture import get -from taggit.models import Tag - class URLAccessMixin(object): @@ -341,8 +342,10 @@ def setUp(self): class APIUnauthAccessTest(APIMixin, TestCase): - def test_api_urls(self): + @mock.patch('readthedocs.restapi.views.task_views.get_public_task_data') + def test_api_urls(self, get_public_task_data): from readthedocs.restapi.urls import urlpatterns + get_public_task_data.side_effect = TaskNoPermission('Nope') self._test_url(urlpatterns) def login(self): diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index d595f49e430..a46ccaefa75 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -4,8 +4,6 @@ from __future__ import absolute_import import os -import djcelery - from readthedocs.core.settings import Settings try: @@ -15,8 +13,6 @@ donate = False -djcelery.setup_loader() - _ = gettext = lambda s: s @@ -85,9 +81,6 @@ def INSTALLED_APPS(self): # noqa 'django_extensions', 'messages_extends', - # Celery bits - 'djcelery', - # daniellindsleyrocksdahouse 'haystack', 'tastypie', diff --git a/readthedocs/worker.py b/readthedocs/worker.py new file mode 100644 index 00000000000..e04d30d6dbb --- /dev/null +++ b/readthedocs/worker.py @@ -0,0 +1,19 @@ +"""Celery worker application instantiation""" + +from __future__ import absolute_import, unicode_literals + +import os + +from celery import Celery + + +def create_application(): + """Create a Celery application using Django settings""" + os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'readthedocs.settings.dev') + application = Celery('readthedocs') + application.config_from_object('django.conf:settings') + application.autodiscover_tasks(None) + return application + + +app = create_application() # pylint: disable=invalid-name diff --git a/requirements/pip.txt b/requirements/pip.txt index caa6b917067..2a56390d8ae 100644 --- a/requirements/pip.txt +++ b/requirements/pip.txt @@ -26,9 +26,8 @@ lxml==3.3.5 defusedxml==0.5.0 # Basic tools -redis==2.10.3 -celery==3.1.23 -django-celery==3.2.1 +redis==2.10.6 +celery==4.1.0 django-allauth==0.32.0 dnspython==1.15.0 From faa2308b6b6819ab5426bcf645bbb38cb4d880d8 Mon Sep 17 00:00:00 2001 From: anatoly techtonik Date: Sat, 11 Nov 2017 02:31:22 +0200 Subject: [PATCH 06/16] Rename test.py to tests.py (#3239) Because it is not discovered - https://travis-ci.org/rtfd/readthedocs.org/jobs/300125610#L793 --- readthedocs/vcs_support/{test.py => tests.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename readthedocs/vcs_support/{test.py => tests.py} (100%) diff --git a/readthedocs/vcs_support/test.py b/readthedocs/vcs_support/tests.py similarity index 100% rename from readthedocs/vcs_support/test.py rename to readthedocs/vcs_support/tests.py From 7b5dd2b11679e9bef56800b0ec3f8f113e43b785 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 15:07:28 -0700 Subject: [PATCH 07/16] Improve error reporting to users --- readthedocs/doc_builder/environments.py | 11 ++++-- readthedocs/projects/tasks.py | 46 ++++++++++++++++++------- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 7fb8f7e592e..e2f8ed6707c 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -418,10 +418,17 @@ def update_build(self, state=None): # BuildEnvironmentException or BuildEnvironmentWarning if isinstance(self.failure, (BuildEnvironmentException, BuildEnvironmentWarning)): - self.build['error'] = str(self.failure) + self.build['error'] = ugettext_noop( + "A failure in building the documentation as occured: {}".format( + str(self.failure) + ) + ) else: self.build['error'] = ugettext_noop( - "An unexpected error occurred") + "A failure in our code has occured. The failure is: {}".format( + str(self.failure) + ) + ) # Attempt to stop unicode errors on build reporting for key, val in list(self.build.items()): diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index f5052b34697..51142ad6a5a 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -110,18 +110,40 @@ def _log(self, msg): def run(self, pk, version_pk=None, build_pk=None, record=True, docker=False, search=True, force=False, localmedia=True, **__): - # pylint: disable=arguments-differ - self.project = self.get_project(pk) - self.version = self.get_version(self.project, version_pk) - self.build = self.get_build(build_pk) - self.build_search = search - self.build_localmedia = localmedia - self.build_force = force - self.config = None + """ + Run a documentation build. - setup_successful = self.run_setup(record=record) - if setup_successful: - self.run_build(record=record, docker=docker) + This is fully wrapped in exception handling to account for a number of failure cases. + """ + + # pylint: disable=arguments-differ + try: + self.project = self.get_project(pk) + self.version = self.get_version(self.project, version_pk) + self.build = self.get_build(build_pk) + self.build_search = search + self.build_localmedia = localmedia + self.build_force = force + self.config = None + + setup_successful = self.run_setup(record=record) + if setup_successful: + self.run_build(record=record, docker=docker) + failure = self.setup_env.failure or self.build_env.failure + except Exception as e: # noqa + log.exception('Top-level build exception has been raised') + failure = str(e) + + # **Always** report build status. + # This can still fail if the API Is totally down, but should catch more failures + build_updates = {'state': BUILD_STATE_FINISHED, + 'success': False, + 'error': 'Unknown setup failure: {}'.format(failure)} + if hasattr(self, 'build'): + self.build.update(build_updates) + api_v2.build(self.build['id']).put(self.build) + else: + api_v2.build(build_pk).patch(build_updates) def run_setup(self, record=True): """Run setup in the local environment. @@ -230,8 +252,6 @@ def run_build(self, docker=False, record=True): self.send_notifications() build_complete.send(sender=Build, build=self.build_env.build) - self.build_env.update_build(state=BUILD_STATE_FINISHED) - @staticmethod def get_project(project_pk): """Get project from API""" From b801ad256bfdc999ac400089536a68e379e65eb4 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 15:16:03 -0700 Subject: [PATCH 08/16] Retry API calls up to 3 times. This should make us a lot more resilient to failures, specifically when *one* web server is acting up. --- readthedocs/restapi/client.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/readthedocs/restapi/client.py b/readthedocs/restapi/client.py index 3ebb6b2a82e..c9cb90307ce 100644 --- a/readthedocs/restapi/client.py +++ b/readthedocs/restapi/client.py @@ -4,7 +4,7 @@ import logging from slumber import API, serialize -from requests import Session +import requests from django.conf import settings from rest_framework.renderers import JSONRenderer from rest_framework.parsers import JSONParser @@ -32,8 +32,10 @@ def dumps(self, data): def setup_api(): - session = Session() + session = requests.Session() session.headers.update({'Host': PRODUCTION_DOMAIN}) + retry_adapter = requests.adapters.HTTPAdapter(max_retries=3) + session.mount(API_HOST, retry_adapter) api_config = { 'base_url': '%s/api/v2/' % API_HOST, 'serializer': serialize.Serializer( From 7a4e4da2f230db436c30e88005f716efa7cc808c Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 15:27:22 -0700 Subject: [PATCH 09/16] Handle a few more cases --- readthedocs/projects/tasks.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 51142ad6a5a..65f4ee26f5c 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -136,14 +136,18 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, # **Always** report build status. # This can still fail if the API Is totally down, but should catch more failures - build_updates = {'state': BUILD_STATE_FINISHED, - 'success': False, - 'error': 'Unknown setup failure: {}'.format(failure)} if hasattr(self, 'build'): - self.build.update(build_updates) - api_v2.build(self.build['id']).put(self.build) + self.build['state'] = BUILD_STATE_FINISHED + if failure: + self.build['error'] += 'Unknown setup failure: {}'.format(failure) + self.build['success'] = False + result = api_v2.build(self.build['id']).patch(self.build) else: - api_v2.build(build_pk).patch(build_updates) + build_updates = {'state': BUILD_STATE_FINISHED, + 'success': False, + 'error': 'Unknown setup failure: {}'.format(failure)} + result = api_v2.build(build_pk).patch(build_updates) + print(result) def run_setup(self, record=True): """Run setup in the local environment. @@ -257,7 +261,6 @@ def get_project(project_pk): """Get project from API""" project_data = api_v2.project(project_pk).get() project = APIProject(**project_data) - return project @staticmethod def get_version(project, version_pk): From 6ed4eef572124c9117fe0876b6d1f2d83c75c05f Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 15:28:23 -0700 Subject: [PATCH 10/16] Fix error I was testing :D --- readthedocs/projects/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 65f4ee26f5c..b5117601f7a 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -260,7 +260,7 @@ def run_build(self, docker=False, record=True): def get_project(project_pk): """Get project from API""" project_data = api_v2.project(project_pk).get() - project = APIProject(**project_data) + return APIProject(**project_data) @staticmethod def get_version(project, version_pk): From aaa918a6c80657e678265fd94636ff7cbdcddd4b Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 15:57:51 -0700 Subject: [PATCH 11/16] Clean up task reporting --- readthedocs/projects/tasks.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index b5117601f7a..73057b0078f 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -139,15 +139,14 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, if hasattr(self, 'build'): self.build['state'] = BUILD_STATE_FINISHED if failure: - self.build['error'] += 'Unknown setup failure: {}'.format(failure) + self.build['error'] = 'Unknown setup failure: {}'.format(failure) self.build['success'] = False - result = api_v2.build(self.build['id']).patch(self.build) + result = api_v2.build(build_pk).patch(self.build) else: build_updates = {'state': BUILD_STATE_FINISHED, 'success': False, 'error': 'Unknown setup failure: {}'.format(failure)} result = api_v2.build(build_pk).patch(build_updates) - print(result) def run_setup(self, record=True): """Run setup in the local environment. From d942cddac6554e65147794891700679113d0bf48 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 15:58:18 -0700 Subject: [PATCH 12/16] Fix test --- readthedocs/rtd_tests/tests/test_celery.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index b4ac4530152..3bf4a429cc2 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -116,7 +116,6 @@ def test_update_docs_unexpected_build_exception(self, mock_update_build, mock_bu record=False, intersphinx=False) self.assertTrue(result.successful()) - mock_update_build.assert_called_with(state=BUILD_STATE_FINISHED) def test_update_imported_doc(self): with mock_api(self.repo): From 3d61762b4246ae980198b36feb433c1d90e93f1d Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 15:59:55 -0700 Subject: [PATCH 13/16] Speed up tests! --- readthedocs/rtd_tests/tests/test_celery.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index 3bf4a429cc2..0ecc9840b11 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -64,10 +64,9 @@ def test_clear_artifacts(self): self.assertTrue(result.successful()) self.assertFalse(exists(directory)) - @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', - new=MagicMock) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', - new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', new=MagicMock) def test_update_docs(self): build = get(Build, project=self.project, version=self.project.versions.first()) @@ -80,8 +79,8 @@ def test_update_docs(self): intersphinx=False) self.assertTrue(result.successful()) - @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', - new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs') @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build') def test_update_docs_unexpected_setup_exception(self, mock_update_build, mock_setup_vcs): @@ -100,8 +99,8 @@ def test_update_docs_unexpected_setup_exception(self, mock_update_build, mock_se mock_update_build.assert_called_once_with(state=BUILD_STATE_FINISHED) @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs') - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', - new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', new=MagicMock) @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build') def test_update_docs_unexpected_build_exception(self, mock_update_build, mock_build_docs): exc = Exception() From f27196c3d83350ccbf2ac8d8a8408d98765f24d1 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 16:09:47 -0700 Subject: [PATCH 14/16] More cleaning of tests --- readthedocs/projects/tasks.py | 1 - readthedocs/rtd_tests/tests/test_celery.py | 11 +++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 73057b0078f..b9efaa22f6e 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -191,7 +191,6 @@ def run_setup(self, record=True): if not isinstance(self.setup_env.failure, vcs_support_utils.LockTimeout): self.send_notifications() - self.setup_env.update_build(state=BUILD_STATE_FINISHED) return False if self.setup_env.successful and not self.project.has_valid_clone: diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index 0ecc9840b11..49db6b96dd0 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -81,9 +81,9 @@ def test_update_docs(self): @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_environment', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', new=MagicMock) + @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs') - @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build') - def test_update_docs_unexpected_setup_exception(self, mock_update_build, mock_setup_vcs): + def test_update_docs_unexpected_setup_exception(self, mock_setup_vcs): exc = Exception() mock_setup_vcs.side_effect = exc build = get(Build, project=self.project, @@ -96,13 +96,12 @@ def test_update_docs_unexpected_setup_exception(self, mock_update_build, mock_se record=False, intersphinx=False) self.assertTrue(result.successful()) - mock_update_build.assert_called_once_with(state=BUILD_STATE_FINISHED) - @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs') @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_environment', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', new=MagicMock) - @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build') - def test_update_docs_unexpected_build_exception(self, mock_update_build, mock_build_docs): + @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs') + def test_update_docs_unexpected_build_exception(self, mock_build_docs): exc = Exception() mock_build_docs.side_effect = exc build = get(Build, project=self.project, From fcc12de02fbe66e33459055fc0eae709d0734542 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 16:10:05 -0700 Subject: [PATCH 15/16] Return API call from task --- readthedocs/projects/tasks.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index b9efaa22f6e..a8600f35ce0 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -136,6 +136,7 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, # **Always** report build status. # This can still fail if the API Is totally down, but should catch more failures + result = {} if hasattr(self, 'build'): self.build['state'] = BUILD_STATE_FINISHED if failure: @@ -147,6 +148,7 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, 'success': False, 'error': 'Unknown setup failure: {}'.format(failure)} result = api_v2.build(build_pk).patch(build_updates) + return result def run_setup(self, record=True): """Run setup in the local environment. From 3da4cc0ade793b12e8eedaa43408277143c12a74 Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Fri, 10 Nov 2017 17:29:11 -0700 Subject: [PATCH 16/16] Fix lint --- readthedocs/projects/tasks.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index a8600f35ce0..1a75ad8d07c 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -108,6 +108,7 @@ def _log(self, msg): version=self.version.slug, msg=msg)) + # pylint: disable=arguments-differ def run(self, pk, version_pk=None, build_pk=None, record=True, docker=False, search=True, force=False, localmedia=True, **__): """ @@ -115,8 +116,6 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, This is fully wrapped in exception handling to account for a number of failure cases. """ - - # pylint: disable=arguments-differ try: self.project = self.get_project(pk) self.version = self.get_version(self.project, version_pk)