diff --git a/.gitignore b/.gitignore index 4aa17ec608f..11f1bac4d81 100644 --- a/.gitignore +++ b/.gitignore @@ -39,6 +39,7 @@ media/pdf media/static /static node_modules +nodeenv readthedocs/rtd_tests/builds readthedocs/rtd_tests/tests/builds user_builds diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index 7b1e4dc5253..340568bf703 100644 --- a/readthedocs/api/v2/utils.py +++ b/readthedocs/api/v2/utils.py @@ -166,14 +166,16 @@ def _set_or_create_version(project, slug, version_id, verbose_name, type_): return version, False -def _get_deleted_versions_qs(project, version_data): +def _get_deleted_versions_qs(project, tags_data, branches_data): # We use verbose_name for tags # because several tags can point to the same identifier. versions_tags = [ - version['verbose_name'] for version in version_data.get('tags', []) + version['verbose_name'] + for version in tags_data ] versions_branches = [ - version['identifier'] for version in version_data.get('branches', []) + version['identifier'] + for version in branches_data ] to_delete_qs = ( @@ -193,14 +195,18 @@ def _get_deleted_versions_qs(project, version_data): return to_delete_qs -def delete_versions_from_db(project, version_data): +def delete_versions_from_db(project, tags_data, branches_data): """ Delete all versions not in the current repo. :returns: The slug of the deleted versions from the database. """ to_delete_qs = ( - _get_deleted_versions_qs(project, version_data) + _get_deleted_versions_qs( + project=project, + tags_data=tags_data, + branches_data=branches_data, + ) .exclude(active=True) ) deleted_versions = set(to_delete_qs.values_list('slug', flat=True)) @@ -214,10 +220,14 @@ def delete_versions_from_db(project, version_data): return deleted_versions -def get_deleted_active_versions(project, version_data): +def get_deleted_active_versions(project, tags_data, branches_data): """Return the slug of active versions that were deleted from the repository.""" to_delete_qs = ( - _get_deleted_versions_qs(project, version_data) + _get_deleted_versions_qs( + project=project, + tags_data=tags_data, + branches_data=branches_data, + ) .filter(active=True) ) return set(to_delete_qs.values_list('slug', flat=True)) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 639d9716df1..9c2b1014d0a 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -6,7 +6,6 @@ from allauth.socialaccount.models import SocialAccount from django.conf import settings from django.core.files.storage import get_storage_class -from django.db.models import Q from django.shortcuts import get_object_or_404 from django.template.loader import render_to_string from rest_framework import decorators, permissions, status, viewsets @@ -14,27 +13,14 @@ from rest_framework.renderers import BaseRenderer, JSONRenderer from rest_framework.response import Response -from readthedocs.builds.constants import ( - BRANCH, - BUILD_STATE_FINISHED, - BUILD_STATE_TRIGGERED, - INTERNAL, - TAG, -) +from readthedocs.builds.constants import INTERNAL from readthedocs.builds.models import Build, BuildCommandResult, Version -from readthedocs.core.utils import trigger_build -from readthedocs.core.utils.extend import SettingsOverrideObject +from readthedocs.builds.tasks import sync_versions_task from readthedocs.oauth.models import RemoteOrganization, RemoteRepository from readthedocs.oauth.services import GitHubService, registry -from readthedocs.projects.models import Domain, EmailHook, Project -from readthedocs.projects.version_handling import determine_stable_version - -from ..permissions import ( - APIPermission, - APIRestrictedPermission, - IsOwner, - RelatedProjectIsOwner, -) +from readthedocs.projects.models import Domain, Project + +from ..permissions import APIPermission, APIRestrictedPermission, IsOwner from ..serializers import ( BuildAdminSerializer, BuildCommandSerializer, @@ -52,10 +38,6 @@ ProjectPagination, RemoteOrganizationPagination, RemoteProjectPagination, - delete_versions_from_db, - get_deleted_active_versions, - run_automation_rules, - sync_versions_to_db, ) log = logging.getLogger(__name__) @@ -181,47 +163,34 @@ def canonical_url(self, request, **kwargs): permission_classes=[permissions.IsAdminUser], methods=['post'], ) - def sync_versions(self, request, **kwargs): # noqa: D205 + def sync_versions(self, request, **kwargs): # noqa """ Sync the version data in the repo (on the build server). Version data in the repo is synced with what we have in the database. :returns: the identifiers for the versions that have been deleted. + + .. note:: + + This endpoint is deprecated in favor of `sync_versions_task`. """ project = get_object_or_404( Project.objects.api(request.user), pk=kwargs['pk'], ) - # If the currently highest non-prerelease version is active, then make - # the new latest version active as well. - current_stable = project.get_original_stable_version() - if current_stable is not None: - activate_new_stable = current_stable.active - else: - activate_new_stable = False + added_versions = [] + deleted_versions = [] try: - # Update All Versions data = request.data - added_versions = set() - if 'tags' in data: - ret_set = sync_versions_to_db( - project=project, - versions=data['tags'], - type=TAG, - ) - added_versions.update(ret_set) - if 'branches' in data: - ret_set = sync_versions_to_db( - project=project, - versions=data['branches'], - type=BRANCH, - ) - added_versions.update(ret_set) - deleted_versions = delete_versions_from_db(project, data) - deleted_active_versions = get_deleted_active_versions(project, data) + # Calling the task synchronically to keep backward compatibility + added_versions, deleted_versions = sync_versions_task( + project_pk=project.pk, + tags_data=data.get('tags', []), + branches_data=data.get('branches', []), + ) except Exception as e: log.exception('Sync Versions Error') return Response( @@ -231,42 +200,6 @@ def sync_versions(self, request, **kwargs): # noqa: D205 status=status.HTTP_400_BAD_REQUEST, ) - try: - # The order of added_versions isn't deterministic. - # We don't track the commit time or any other metadata. - # We usually have one version added per webhook. - run_automation_rules(project, added_versions, deleted_active_versions) - except Exception: - # Don't interrupt the request if something goes wrong - # in the automation rules. - log.exception( - 'Failed to execute automation rules for [%s]: %s', - project.slug, added_versions - ) - - # TODO: move this to an automation rule - promoted_version = project.update_stable_version() - new_stable = project.get_stable_version() - if promoted_version and new_stable and new_stable.active: - log.info( - 'Triggering new stable build: %(project)s:%(version)s', - { - 'project': project.slug, - 'version': new_stable.identifier, - } - ) - trigger_build(project=project, version=new_stable) - - # Marking the tag that is considered the new stable version as - # active and building it if it was just added. - if ( - activate_new_stable and - promoted_version.slug in added_versions - ): - promoted_version.active = True - promoted_version.save() - trigger_build(project=project, version=promoted_version) - return Response({ 'added_versions': added_versions, 'deleted_versions': deleted_versions, diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index 27c2ba822a8..8c17e67fcd5 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -8,14 +8,24 @@ from django.core.files.storage import get_storage_class from readthedocs.api.v2.serializers import BuildSerializer +from readthedocs.api.v2.utils import ( + delete_versions_from_db, + get_deleted_active_versions, + run_automation_rules, + sync_versions_to_db, +) from readthedocs.builds.constants import ( + BRANCH, BUILD_STATUS_FAILURE, BUILD_STATUS_PENDING, BUILD_STATUS_SUCCESS, MAX_BUILD_COMMAND_SIZE, + TAG, ) from readthedocs.builds.models import Build, Version from readthedocs.builds.utils import memcache_lock +from readthedocs.core.utils import trigger_build +from readthedocs.projects.models import Project from readthedocs.projects.tasks import send_build_status from readthedocs.worker import app @@ -206,3 +216,99 @@ def delete_inactive_external_versions(limit=200, days=30 * 3): version.project.slug, version.slug, ) version.delete() + + +@app.task( + max_retries=1, + default_retry_delay=60, + queue='web' +) +def sync_versions_task(project_pk, tags_data, branches_data, **kwargs): + """ + Sync the version data in the repo (from build server) into our database. + + Creates new Version objects for tags/branches that aren't tracked in the database, + and deletes Version objects for tags/branches that don't exists in the repository. + + :param tags_data: List of dictionaries with ``verbose_name`` and ``identifier``. + :param branches_data: Same as ``tags_data`` but for branches. + :returns: the identifiers for the versions that have been deleted. + """ + project = Project.objects.get(pk=project_pk) + + # If the currently highest non-prerelease version is active, then make + # the new latest version active as well. + current_stable = project.get_original_stable_version() + if current_stable is not None: + activate_new_stable = current_stable.active + else: + activate_new_stable = False + + try: + # Update All Versions + added_versions = set() + result = sync_versions_to_db( + project=project, + versions=tags_data, + type=TAG, + ) + added_versions.update(result) + + result = sync_versions_to_db( + project=project, + versions=branches_data, + type=BRANCH, + ) + added_versions.update(result) + + deleted_versions = delete_versions_from_db( + project=project, + tags_data=tags_data, + branches_data=branches_data, + ) + deleted_active_versions = get_deleted_active_versions( + project=project, + tags_data=tags_data, + branches_data=branches_data, + ) + except Exception: + log.exception('Sync Versions Error') + return [], [] + + try: + # The order of added_versions isn't deterministic. + # We don't track the commit time or any other metadata. + # We usually have one version added per webhook. + run_automation_rules(project, added_versions, deleted_active_versions) + except Exception: + # Don't interrupt the request if something goes wrong + # in the automation rules. + log.exception( + 'Failed to execute automation rules for [%s]: %s', + project.slug, added_versions + ) + + # TODO: move this to an automation rule + promoted_version = project.update_stable_version() + new_stable = project.get_stable_version() + if promoted_version and new_stable and new_stable.active: + log.info( + 'Triggering new stable build: %(project)s:%(version)s', + { + 'project': project.slug, + 'version': new_stable.identifier, + } + ) + trigger_build(project=project, version=new_stable) + + # Marking the tag that is considered the new stable version as + # active and building it if it was just added. + if ( + activate_new_stable and + promoted_version.slug in added_versions + ): + promoted_version.active = True + promoted_version.save() + trigger_build(project=project, version=promoted_version) + + return list(added_versions), list(deleted_versions) diff --git a/readthedocs/core/management/commands/pull.py b/readthedocs/core/management/commands/pull.py index ae62f9da10c..8b2a95a2939 100644 --- a/readthedocs/core/management/commands/pull.py +++ b/readthedocs/core/management/commands/pull.py @@ -4,7 +4,7 @@ import logging -from django.core.management.base import BaseCommand +from django.core.management.base import LabelCommand from readthedocs.builds.constants import LATEST from readthedocs.projects import tasks, utils @@ -13,12 +13,10 @@ log = logging.getLogger(__name__) -class Command(BaseCommand): +class Command(LabelCommand): help = __doc__ - def handle(self, *args, **options): - if args: - for slug in args: - version = utils.version_from_slug(slug, LATEST) - tasks.sync_repository_task(version.pk) + def handle_label(self, label, **options): + version = utils.version_from_slug(label, LATEST) + tasks.sync_repository_task(version.pk) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index cad3ecc833d..f0986aedafe 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1580,11 +1580,14 @@ def add_features(sender, **kwargs): CONDA_APPEND_CORE_REQUIREMENTS = 'conda_append_core_requirements' CONDA_USES_MAMBA = 'conda_uses_mamba' ALL_VERSIONS_IN_HTML_CONTEXT = 'all_versions_in_html_context' + CACHED_ENVIRONMENT = 'cached_environment' + LIMIT_CONCURRENT_BUILDS = 'limit_concurrent_builds' + + # Versions sync related features SKIP_SYNC_TAGS = 'skip_sync_tags' SKIP_SYNC_BRANCHES = 'skip_sync_branches' SKIP_SYNC_VERSIONS = 'skip_sync_versions' - CACHED_ENVIRONMENT = 'cached_environment' - LIMIT_CONCURRENT_BUILDS = 'limit_concurrent_builds' + SYNC_VERSIONS_USING_A_TASK = 'sync_versions_using_a_task' # Search related features DISABLE_SERVER_SIDE_SEARCH = 'disable_server_side_search' @@ -1671,6 +1674,16 @@ def add_features(sender, **kwargs): 'when building with Sphinx' ), ), + ( + CACHED_ENVIRONMENT, + _('Cache the environment (virtualenv, conda, pip cache, repository) in storage'), + ), + ( + LIMIT_CONCURRENT_BUILDS, + _('Limit the amount of concurrent builds'), + ), + + # Versions sync related features ( SKIP_SYNC_BRANCHES, _('Skip syncing branches'), @@ -1683,14 +1696,6 @@ def add_features(sender, **kwargs): SKIP_SYNC_VERSIONS, _('Skip sync versions task'), ), - ( - CACHED_ENVIRONMENT, - _('Cache the environment (virtualenv, conda, pip cache, repository) in storage'), - ), - ( - LIMIT_CONCURRENT_BUILDS, - _('Limit the amount of concurrent builds'), - ), # Search related features. ( diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index db3a33580d4..5874c4e37a5 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -233,18 +233,18 @@ def sync_repo(self, environment): ) version_repo = self.get_vcs_repo(environment) version_repo.update() - self.sync_versions_api(version_repo) + self.sync_versions(version_repo) identifier = getattr(self, 'commit', None) or self.version.identifier version_repo.checkout(identifier) - def sync_versions_api(self, version_repo): + def sync_versions(self, version_repo): """ - Update tags/branches hitting the API. + Update tags/branches via a Celery task. - It may trigger a new build to the stable version when hitting the - ``sync_versions`` endpoint. + .. note:: + + It may trigger a new build to the stable version. """ - version_post_data = {'repo': version_repo.repo_url} tags = None branches = None if ( @@ -256,39 +256,69 @@ def sync_versions_api(self, version_repo): # have already cloned the repository locally. The latter happens # when triggering a normal build. branches, tags = version_repo.lsremote + log.info('Remote versions: branches=%s tags=%s', branches, tags) + + branches_data = [] + tags_data = [] - if all([ - version_repo.supports_tags, + if ( + version_repo.supports_tags and not self.project.has_feature(Feature.SKIP_SYNC_TAGS) - ]): - tags = tags or version_repo.tags - version_post_data['tags'] = [{ - 'identifier': v.identifier, - 'verbose_name': v.verbose_name, - } for v in tags] - - if all([ - version_repo.supports_branches, + ): + # Will be an empty list if we called lsremote and had no tags returned + if tags is None: + tags = version_repo.tags + tags_data = [ + { + 'identifier': v.identifier, + 'verbose_name': v.verbose_name, + } + for v in tags + ] + + if ( + version_repo.supports_branches and not self.project.has_feature(Feature.SKIP_SYNC_BRANCHES) - ]): - branches = branches or version_repo.branches - version_post_data['branches'] = [{ - 'identifier': v.identifier, - 'verbose_name': v.verbose_name, - } for v in branches] + ): + # Will be an empty list if we called lsremote and had no branches returned + if branches is None: + branches = version_repo.branches + branches_data = [ + { + 'identifier': v.identifier, + 'verbose_name': v.verbose_name, + } + for v in branches + ] - self.validate_duplicate_reserved_versions(version_post_data) + self.validate_duplicate_reserved_versions( + tags_data=tags_data, + branches_data=branches_data, + ) - try: - api_v2.project(self.project.pk).sync_versions.post( - version_post_data, + if self.project.has_feature(Feature.SYNC_VERSIONS_USING_A_TASK): + from readthedocs.builds import tasks as build_tasks + build_tasks.sync_versions_task.delay( + project_pk=self.project.pk, + tags_data=tags_data, + branches_data=branches_data, ) - except HttpClientError: - log.exception('Sync Versions Exception') - except Exception: - log.exception('Unknown Sync Versions Exception') + else: + try: + version_post_data = { + 'repo': version_repo.repo_url, + 'tags': tags_data, + 'branches': branches_data, + } + api_v2.project(self.project.pk).sync_versions.post( + version_post_data, + ) + except HttpClientError: + log.exception('Sync Versions Exception') + except Exception: + log.exception('Unknown Sync Versions Exception') - def validate_duplicate_reserved_versions(self, data): + def validate_duplicate_reserved_versions(self, tags_data, branches_data): """ Check if there are duplicated names of reserved versions. @@ -300,7 +330,7 @@ def validate_duplicate_reserved_versions(self, data): """ version_names = [ version['verbose_name'] - for version in data.get('tags', []) + data.get('branches', []) + for version in tags_data + branches_data ] counter = Counter(version_names) for reserved_name in [STABLE_VERBOSE_NAME, LATEST_VERBOSE_NAME]: @@ -430,7 +460,7 @@ def update_versions_from_repository(self, environment): self.sync_repo(environment) else: log.info('Syncing repository via remote listing. project=%s', self.project.slug) - self.sync_versions_api(version_repo) + self.sync_versions(version_repo) @app.task( diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index 2144354483c..b32558884c4 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -22,7 +22,7 @@ from readthedocs.oauth.models import RemoteRepository from readthedocs.projects import tasks from readthedocs.projects.exceptions import RepositoryError -from readthedocs.projects.models import Project +from readthedocs.projects.models import Feature, Project from readthedocs.rtd_tests.mocks.mock_api import mock_api from readthedocs.rtd_tests.utils import ( create_git_branch, @@ -54,6 +54,11 @@ def setUp(self): repo=repo, ) self.project.users.add(self.eric) + get( + Feature, + feature_id=Feature.SYNC_VERSIONS_USING_A_TASK, + projects=[self.project], + ) def get_update_docs_task(self, version): build_env = LocalBuildEnvironment( @@ -237,9 +242,8 @@ def test_clean_build_after_failure_in_sync_repository(self, clean_build, run_syn result = tasks.sync_repository_task.delay(version.pk) clean_build.assert_called_with(version.pk) - @patch('readthedocs.projects.tasks.api_v2') @patch('readthedocs.projects.models.Project.checkout_path') - def test_check_duplicate_reserved_version_latest(self, checkout_path, api_v2): + def test_check_duplicate_reserved_version_latest(self, checkout_path): create_git_branch(self.repo, 'latest') create_git_tag(self.repo, 'latest') @@ -259,7 +263,7 @@ def test_check_duplicate_reserved_version_latest(self, checkout_path, api_v2): delete_git_branch(self.repo, 'latest') sync_repository.sync_repo(sync_repository.build_env) - api_v2.project().sync_versions.post.assert_called() + self.assertTrue(self.project.versions.filter(slug=LATEST).exists()) @patch('readthedocs.projects.tasks.api_v2') @patch('readthedocs.projects.models.Project.checkout_path') @@ -284,8 +288,7 @@ def test_check_duplicate_reserved_version_stable(self, checkout_path, api_v2): # TODO: Check that we can build properly after # deleting the tag. - @patch('readthedocs.projects.tasks.api_v2') - def test_check_duplicate_no_reserved_version(self, api_v2): + def test_check_duplicate_no_reserved_version(self): create_git_branch(self.repo, 'no-reserved') create_git_tag(self.repo, 'no-reserved') @@ -293,8 +296,11 @@ def test_check_duplicate_no_reserved_version(self, api_v2): sync_repository = self.get_update_docs_task(version) + self.assertEqual(self.project.versions.filter(slug__startswith='no-reserved').count(), 0) + sync_repository.sync_repo(sync_repository.build_env) - api_v2.project().sync_versions.post.assert_called() + + self.assertEqual(self.project.versions.filter(slug__startswith='no-reserved').count(), 2) def test_public_task_exception(self): """ diff --git a/readthedocs/rtd_tests/tests/test_sync_versions.py b/readthedocs/rtd_tests/tests/test_sync_versions.py index d8b8d0ef43f..d94ade1a06b 100644 --- a/readthedocs/rtd_tests/tests/test_sync_versions.py +++ b/readthedocs/rtd_tests/tests/test_sync_versions.py @@ -19,7 +19,7 @@ @mock.patch('readthedocs.core.utils.trigger_build', mock.MagicMock()) -@mock.patch('readthedocs.api.v2.views.model_views.trigger_build', mock.MagicMock()) +@mock.patch('readthedocs.builds.tasks.trigger_build', mock.MagicMock()) class TestSyncVersions(TestCase): fixtures = ['eric', 'test_data'] @@ -773,7 +773,7 @@ def test_deletes_version_with_same_identifier(self): 1, ) - @mock.patch('readthedocs.api.v2.views.model_views.run_automation_rules') + @mock.patch('readthedocs.builds.tasks.run_automation_rules') def test_automation_rules_are_triggered_for_new_versions(self, run_automation_rules): Version.objects.create( project=self.pip, @@ -944,7 +944,7 @@ def test_automation_rule_dont_delete_default_version(self): self.assertTrue(self.pip.versions.filter(slug=version_slug).exists()) @mock.patch('readthedocs.core.utils.trigger_build', mock.MagicMock()) -@mock.patch('readthedocs.api.v2.views.model_views.trigger_build', mock.MagicMock()) +@mock.patch('readthedocs.builds.tasks.trigger_build', mock.MagicMock()) class TestStableVersion(TestCase): fixtures = ['eric', 'test_data'] @@ -1459,7 +1459,7 @@ def test_user_defined_stable_version_branch_with_tags(self): @mock.patch('readthedocs.core.utils.trigger_build', mock.MagicMock()) -@mock.patch('readthedocs.api.v2.views.model_views.trigger_build', mock.MagicMock()) +@mock.patch('readthedocs.builds.tasks.trigger_build', mock.MagicMock()) class TestLatestVersion(TestCase): fixtures = ['eric', 'test_data']