diff --git a/readthedocs/builds/constants.py b/readthedocs/builds/constants.py index 3847289c509..f5bf898bce0 100644 --- a/readthedocs/builds/constants.py +++ b/readthedocs/builds/constants.py @@ -140,10 +140,8 @@ } BUILD_STATUS_NORMAL = 'normal' -BUILD_STATUS_DUPLICATED = 'duplicated' BUILD_STATUS_CHOICES = ( (BUILD_STATUS_NORMAL, 'Normal'), - (BUILD_STATUS_DUPLICATED, 'Duplicated'), ) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 6a30f4ac0d0..3829c94acee 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -1,12 +1,10 @@ """Common utility functions.""" -import datetime import re import signal import structlog from django.conf import settings -from django.utils import timezone from django.utils.functional import keep_lazy from django.utils.safestring import SafeText, mark_safe from django.utils.text import slugify as slugify_base @@ -19,11 +17,7 @@ BUILD_STATUS_PENDING, EXTERNAL, ) -from readthedocs.doc_builder.exceptions import ( - BuildCancelled, - BuildMaxConcurrencyError, - DuplicatedBuildError, -) +from readthedocs.doc_builder.exceptions import BuildCancelled, BuildMaxConcurrencyError from readthedocs.worker import app log = structlog.get_logger(__name__) @@ -147,63 +141,6 @@ def prepare_build( # we cancel all of them and trigger a new one for the latest commit received. for running_build in running_builds: cancel_build(running_build) - else: - # NOTE: de-duplicate builds won't be required if we enable `CANCEL_OLD_BUILDS`, - # since canceling a build is more effective. - # However, we are keepting `DEDUPLICATE_BUILDS` code around while we test - # `CANCEL_OLD_BUILDS` with a few projects and we are happy with the results. - # After that, we can remove `DEDUPLICATE_BUILDS` code - # and make `CANCEL_OLD_BUILDS` the default behavior. - if commit: - skip_build = ( - Build.objects.filter( - project=project, - version=version, - commit=commit, - ) - .exclude( - state__in=BUILD_FINAL_STATES, - ) - .exclude( - pk=build.pk, - ) - .exists() - ) - else: - skip_build = ( - Build.objects.filter( - project=project, - version=version, - state=BUILD_STATE_TRIGGERED, - # By filtering for builds triggered in the previous 5 minutes we - # avoid false positives for builds that failed for any reason and - # didn't update their state, ending up on blocked builds for that - # version (all the builds are marked as DUPLICATED in that case). - # Adding this date condition, we reduce the risk of hitting this - # problem to 5 minutes only. - date__gte=timezone.now() - datetime.timedelta(minutes=5), - ).count() - > 1 - ) - - if not project.has_feature(Feature.DEDUPLICATE_BUILDS): - log.debug( - "Skipping deduplication of builds. Feature not enabled.", - ) - skip_build = False - - if skip_build: - # TODO: we could mark the old build as duplicated, however we reset our - # position in the queue and go back to the end of it --penalization - log.warning( - "Marking build to be skipped by builder.", - ) - build.error = DuplicatedBuildError.message - build.status = DuplicatedBuildError.status - build.exit_code = DuplicatedBuildError.exit_code - build.success = False - build.state = BUILD_STATE_CANCELLED - build.save() # Start the build in X minutes and mark it as limited if not skip_build and project.has_feature(Feature.LIMIT_CONCURRENT_BUILDS): diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index a75735ee42b..8a7efab50a9 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -2,7 +2,7 @@ from django.utils.translation import gettext_noop -from readthedocs.builds.constants import BUILD_STATE_CANCELLED, BUILD_STATUS_DUPLICATED +from readthedocs.builds.constants import BUILD_STATE_CANCELLED from readthedocs.projects.constants import BUILD_COMMANDS_OUTPUT_PATH_HTML @@ -56,13 +56,6 @@ class BuildMaxConcurrencyError(BuildUserError): message = gettext_noop('Concurrency limit reached ({limit}), retrying in 5 minutes.') -class DuplicatedBuildError(BuildUserError): - message = gettext_noop('Duplicated build.') - exit_code = 1 - status = BUILD_STATUS_DUPLICATED - state = BUILD_STATE_CANCELLED - - class BuildCancelled(BuildUserError): message = gettext_noop('Build cancelled by user.') state = BUILD_STATE_CANCELLED diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index c0b66831561..9e00eb02c01 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1863,7 +1863,6 @@ def add_features(sender, **kwargs): VCS_REMOTE_LISTING = "vcs_remote_listing" SPHINX_PARALLEL = "sphinx_parallel" USE_SPHINX_BUILDERS = "use_sphinx_builders" - DEDUPLICATE_BUILDS = "deduplicate_builds" CANCEL_OLD_BUILDS = "cancel_old_builds" DONT_CREATE_INDEX = "dont_create_index" @@ -2012,10 +2011,6 @@ def add_features(sender, **kwargs): USE_SPHINX_BUILDERS, _('Use regular sphinx builders instead of custom RTD builders'), ), - ( - DEDUPLICATE_BUILDS, - _('Mark duplicated builds as NOOP to be skipped by builders'), - ), ( CANCEL_OLD_BUILDS, _( diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index d0d066afa83..6553599bbbe 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -44,7 +44,6 @@ BuildCancelled, BuildMaxConcurrencyError, BuildUserError, - DuplicatedBuildError, MkDocsYAMLParseError, ProjectBuildsSkippedError, YAMLParseError, @@ -276,7 +275,6 @@ class UpdateDocsTask(SyncRepositoryMixin, Task): # All exceptions generated by a user miss-configuration should be listed # here. Actually, every subclass of ``BuildUserError``. throws = ( - DuplicatedBuildError, ProjectBuildsSkippedError, ConfigError, YAMLParseError, @@ -291,14 +289,12 @@ class UpdateDocsTask(SyncRepositoryMixin, Task): exceptions_without_notifications = ( BuildCancelled, BuildMaxConcurrencyError, - DuplicatedBuildError, ProjectBuildsSkippedError, ) # Do not send external build status on failure builds for these exceptions. exceptions_without_external_build_status = ( BuildMaxConcurrencyError, - DuplicatedBuildError, ) acks_late = True @@ -354,11 +350,6 @@ def _check_concurrency_limit(self): ) ) - def _check_duplicated_build(self): - if self.data.build.get('status') == DuplicatedBuildError.status: - log.warning('NOOP: build is marked as duplicated.') - raise DuplicatedBuildError - def _check_project_disabled(self): if self.data.project.skip: log.warning('Project build skipped.') @@ -412,7 +403,6 @@ def before_start(self, task_id, args, kwargs): self._setup_sigterm() self._check_project_disabled() - self._check_duplicated_build() self._check_concurrency_limit() self._reset_build() diff --git a/readthedocs/projects/views/public.py b/readthedocs/projects/views/public.py index 01da30f57a9..e2ab3b9810f 100644 --- a/readthedocs/projects/views/public.py +++ b/readthedocs/projects/views/public.py @@ -1,12 +1,12 @@ """Public project views.""" import hashlib -import structlog import mimetypes import os from collections import OrderedDict from urllib.parse import urlparse +import structlog from django.conf import settings from django.contrib import messages from django.db.models import prefetch_related_objects @@ -22,7 +22,7 @@ from readthedocs.analytics.tasks import analytics_event from readthedocs.analytics.utils import get_client_ip -from readthedocs.builds.constants import BUILD_STATUS_DUPLICATED, LATEST +from readthedocs.builds.constants import LATEST from readthedocs.builds.models import Version from readthedocs.builds.views import BuildTriggerMixin from readthedocs.core.permissions import AdminPermission @@ -184,20 +184,6 @@ def get(self, request, project_slug, *args, **kwargs): slug=version_slug, ).first() - if version: - last_build = ( - version.builds - .filter(type='html', state='finished') - .exclude(status=BUILD_STATUS_DUPLICATED) - .order_by('-date') - .first() - ) - if last_build: - if last_build.success: - status = self.STATUS_PASSING - else: - status = self.STATUS_FAILING - return self.serve_badge(request, status) def get_style(self, request): diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index fd42de5189d..8ee068b7d5e 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -40,7 +40,6 @@ from readthedocs.builds.constants import ( BUILD_STATE_CLONING, BUILD_STATE_TRIGGERED, - BUILD_STATUS_DUPLICATED, EXTERNAL, EXTERNAL_VERSION_STATE_CLOSED, LATEST, @@ -105,7 +104,6 @@ def test_reset_build(self): project=self.project, version=self.version, state=BUILD_STATE_CLONING, - status=BUILD_STATUS_DUPLICATED, success=False, output='Output', error='Error', diff --git a/readthedocs/rtd_tests/tests/test_builds.py b/readthedocs/rtd_tests/tests/test_builds.py index 41adf92de2f..2a4da4e1092 100644 --- a/readthedocs/rtd_tests/tests/test_builds.py +++ b/readthedocs/rtd_tests/tests/test_builds.py @@ -1,5 +1,4 @@ import datetime -from unittest import mock from django.contrib.auth.models import User from django.test import TestCase @@ -14,9 +13,7 @@ GITLAB_EXTERNAL_VERSION_NAME, ) from readthedocs.builds.models import Build, Version -from readthedocs.core.utils import trigger_build -from readthedocs.doc_builder.exceptions import DuplicatedBuildError -from readthedocs.projects.models import Feature, Project +from readthedocs.projects.models import Project class BuildModelTests(TestCase): @@ -482,112 +479,3 @@ def test_can_rebuild_with_old_build(self): self.assertFalse(old_external_build.can_rebuild) self.assertTrue(latest_external_build.can_rebuild) - - -@mock.patch('readthedocs.projects.tasks.builds.update_docs_task') -class DeDuplicateBuildTests(TestCase): - - def setUp(self): - self.project = get(Project) - self.version = get( - Version, - project=self.project, - type=BRANCH, - ) - - get( - Feature, - feature_id=Feature.DEDUPLICATE_BUILDS, - projects=[self.project], - ) - - def test_trigger_duplicated_build_by_commit(self, update_docs_task): - """ - Trigger a build for the same commit twice. - - The second build should be marked as NOOP. - """ - self.assertEqual(Build.objects.count(), 0) - trigger_build(self.project, self.version, commit='a1b2c3') - self.assertEqual(Build.objects.count(), 1) - build = Build.objects.first() - self.assertEqual(build.state, 'triggered') - - trigger_build(self.project, self.version, commit='a1b2c3') - self.assertEqual(Build.objects.count(), 2) - build = Build.objects.first() - self.assertEqual(build.error, DuplicatedBuildError.message) - self.assertEqual(build.success, False) - self.assertEqual(build.exit_code, DuplicatedBuildError.exit_code) - self.assertEqual(build.status, DuplicatedBuildError.status) - self.assertEqual(build.state, "cancelled") - - def test_trigger_duplicated_finshed_build_by_commit(self, update_docs_task): - """ - Trigger a build for the same commit twice. - - The second build should not be marked as NOOP if the previous - duplicated builds are in 'finished' state. - """ - self.assertEqual(Build.objects.count(), 0) - trigger_build(self.project, self.version, commit='a1b2c3') - self.assertEqual(Build.objects.count(), 1) - - # Mark the build as finished - build = Build.objects.first() - build.state = 'finished' - build.save() - build.refresh_from_db() - - trigger_build(self.project, self.version, commit='a1b2c3') - self.assertEqual(Build.objects.count(), 2) - build = Build.objects.first() - self.assertEqual(build.state, 'triggered') - self.assertIsNone(build.status) - - def test_trigger_duplicated_build_by_version(self, update_docs_task): - """ - Trigger a build for the same version. - - The second build should be marked as NOOP if there is already a build - for the same project and version on 'triggered' state. - """ - self.assertEqual(Build.objects.count(), 0) - trigger_build(self.project, self.version, commit=None) - self.assertEqual(Build.objects.count(), 1) - build = Build.objects.first() - self.assertEqual(build.state, 'triggered') - - trigger_build(self.project, self.version, commit=None) - self.assertEqual(Build.objects.count(), 2) - build = Build.objects.first() - self.assertEqual(build.error, DuplicatedBuildError.message) - self.assertEqual(build.success, False) - self.assertEqual(build.exit_code, DuplicatedBuildError.exit_code) - self.assertEqual(build.status, DuplicatedBuildError.status) - self.assertEqual(build.state, "cancelled") - - def test_trigger_duplicated_non_triggered_build_by_version(self, update_docs_task): - """ - Trigger a build for the same version. - - The second build should not be marked as NOOP because the previous build - for the same project and version is on 'building' state (any non 'triggered') - """ - self.assertEqual(Build.objects.count(), 0) - trigger_build(self.project, self.version, commit=None) - self.assertEqual(Build.objects.count(), 1) - build = Build.objects.first() - self.assertEqual(build.state, 'triggered') - - # Mark the build as building - build = Build.objects.first() - build.state = 'building' - build.save() - build.refresh_from_db() - - trigger_build(self.project, self.version, commit=None) - self.assertEqual(Build.objects.count(), 2) - build = Build.objects.first() - self.assertEqual(build.state, 'triggered') - self.assertIsNone(build.status) diff --git a/readthedocs/rtd_tests/tests/test_project_views.py b/readthedocs/rtd_tests/tests/test_project_views.py index fef3f434904..2ec080e8a42 100644 --- a/readthedocs/rtd_tests/tests/test_project_views.py +++ b/readthedocs/rtd_tests/tests/test_project_views.py @@ -8,7 +8,7 @@ from django.views.generic.base import ContextMixin from django_dynamic_fixture import get, new -from readthedocs.builds.constants import BUILD_STATUS_DUPLICATED, EXTERNAL +from readthedocs.builds.constants import EXTERNAL from readthedocs.builds.models import Build, Version from readthedocs.integrations.models import GenericAPIWebhook, GitHubWebhook from readthedocs.oauth.models import RemoteRepository, RemoteRepositoryRelation @@ -522,25 +522,6 @@ def test_passing_badge(self): self.assertContains(res, 'passing') self.assertEqual(res['Content-Type'], 'image/svg+xml') - def test_ignore_duplicated_build(self): - """Ignore builds marked as duplicate from the badge status.""" - get( - Build, - project=self.project, - version=self.version, - success=True, - ) - get( - Build, - project=self.project, - version=self.version, - success=False, - status=BUILD_STATUS_DUPLICATED, - ) - res = self.client.get(self.badge_url, {'version': self.version.slug}) - self.assertContains(res, 'passing') - self.assertEqual(res['Content-Type'], 'image/svg+xml') - def test_failing_badge(self): get(Build, project=self.project, version=self.version, success=False) res = self.client.get(self.badge_url, {'version': self.version.slug})