Skip to content

Builds: restart build commands before a new build #7999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions readthedocs/api/v2/views/model_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,17 @@ def retrieve(self, *args, **kwargs):
)
return Response(data)

@decorators.action(
detail=True,
permission_classes=[permissions.IsAdminUser],
methods=['post'],
)
def reset(self, request, **kwargs):
Copy link
Member

@ericholscher ericholscher Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be a method on the model instead of the API? We can probably keep it here for now, but I could see other cases where it might be useful to have this functionality.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move it to the model 👍

"""Reset the build so it can be re-used when re-trying."""
instance = self.get_object()
instance.reset()
return Response(status=status.HTTP_204_NO_CONTENT)


class BuildCommandViewSet(UserSelectViewSet):
parser_classes = [JSONParser, MultiPartParser]
Expand Down
18 changes: 18 additions & 0 deletions readthedocs/builds/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,24 @@ def external_version_name(self):
def using_latest_config(self):
return int(self.config.get('version', '1')) == LATEST_CONFIGURATION_VERSION

def reset(self):
"""
Reset the build so it can be re-used when re-trying.

Dates and states are usually overriden by the build,
we care more about deleting the commands.
"""
self.state = BUILD_STATE_TRIGGERED
self.status = ''
self.success = True
self.output = ''
self.error = ''
self.exit_code = None
self.builder = ''
self.cold_storage = False
self.commands.all().delete()
self.save()


class BuildCommandResultMixin:

Expand Down
6 changes: 5 additions & 1 deletion readthedocs/projects/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
from readthedocs.projects.models import APIProject, Feature
from readthedocs.search.utils import index_new_files, remove_indexed_files
from readthedocs.sphinx_domains.models import SphinxDomain
from readthedocs.storage import build_media_storage, build_environment_storage
from readthedocs.storage import build_environment_storage, build_media_storage
from readthedocs.vcs_support import utils as vcs_support_utils
from readthedocs.worker import app

Expand Down Expand Up @@ -670,6 +670,10 @@ def run_setup(self, record=True):

Return True if successful.
"""
# Reset build only if it has some commands already.
if self.build.get('commands'):
api_v2.build(self.build['id']).reset.post()

if settings.DOCKER_ENABLE:
env_cls = DockerBuildEnvironment
else:
Expand Down
54 changes: 53 additions & 1 deletion readthedocs/rtd_tests/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@
GitLabWebhookView,
)
from readthedocs.api.v2.views.task_views import get_status_data
from readthedocs.builds.constants import EXTERNAL, LATEST
from readthedocs.builds.constants import (
BUILD_STATE_CLONING,
BUILD_STATE_TRIGGERED,
BUILD_STATUS_DUPLICATED,
EXTERNAL,
LATEST,
)
from readthedocs.builds.models import Build, BuildCommandResult, Version
from readthedocs.integrations.models import Integration
from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
Expand All @@ -55,6 +61,11 @@
class APIBuildTests(TestCase):
fixtures = ['eric.json', 'test_data.json']

def setUp(self):
self.user = User.objects.get(username='eric')
self.project = get(Project, users=[self.user])
self.version = self.project.versions.get(slug=LATEST)

def test_make_build(self):
"""Test that a superuser can use the API."""
client = APIClient()
Expand Down Expand Up @@ -82,6 +93,47 @@ def test_make_build(self):
self.assertEqual(build['output'], 'Test Output')
self.assertEqual(build['state_display'], 'Cloning')

def test_reset_build(self):
build = get(
Build,
project=self.project,
version=self.version,
state=BUILD_STATE_CLONING,
status=BUILD_STATUS_DUPLICATED,
success=False,
output='Output',
error='Error',
exit_code=9,
builder='Builder',
cold_storage=True,
)
command = get(
BuildCommandResult,
build=build,
)
build.commands.add(command)

self.assertEqual(build.commands.count(), 1)

client = APIClient()
client.force_login(self.user)
r = client.post(reverse('build-reset', args=(build.pk,)))

self.assertEqual(r.status_code, 204)
build.refresh_from_db()
self.assertEqual(build.project, self.project)
self.assertEqual(build.version, self.version)
self.assertEqual(build.state, BUILD_STATE_TRIGGERED)
self.assertEqual(build.status, '')
self.assertTrue(build.success)
self.assertEqual(build.output, '')
self.assertEqual(build.error, '')
self.assertIsNone(build.exit_code)
self.assertEqual(build.builder, '')
self.assertFalse(build.cold_storage)
self.assertEqual(build.commands.count(), 0)


def test_api_does_not_have_private_config_key_superuser(self):
client = APIClient()
client.login(username='super', password='test')
Expand Down
12 changes: 5 additions & 7 deletions readthedocs/rtd_tests/tests/test_builds.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
# -*- coding: utf-8 -*-
import datetime
import os

from unittest import mock

from allauth.socialaccount.models import SocialAccount
from django.contrib.auth.models import User
from django.test import TestCase
from django_dynamic_fixture import fixture, get
from django.utils import timezone

from allauth.socialaccount.models import SocialAccount
from django_dynamic_fixture import fixture, get

from readthedocs.builds.constants import (
BRANCH,
EXTERNAL,
GENERIC_EXTERNAL_VERSION_NAME,
GITHUB_EXTERNAL_VERSION_NAME,
GITLAB_EXTERNAL_VERSION_NAME,
GENERIC_EXTERNAL_VERSION_NAME
)
from readthedocs.builds.models import Build, Version
from readthedocs.core.utils import trigger_build
Expand Down Expand Up @@ -325,7 +323,7 @@ def test_save_config_in_build_model(self, load_config, api_v2):
task.run_setup()
build_config = task.build['config']
# For patch
api_v2.build.assert_called_once()
api_v2.build().patch.assert_called_once()
assert build_config['version'] == '1'
assert 'sphinx' in build_config
assert build_config['doctype'] == 'sphinx'
Expand Down
1 change: 1 addition & 0 deletions readthedocs/rtd_tests/tests/test_privacy_urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ def setUp(self):
}
self.response_data = {
'build-concurrent': {'status_code': 403},
'build-reset': {'status_code': 403},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is useful anymore. Might be owrth removing this automation if we feel we have good test coverage over this stuff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if they are that useful, but it doesn't hurt having more tests p:

'project-sync-versions': {'status_code': 403},
'project-token': {'status_code': 403},
'emailhook-list': {'status_code': 403},
Expand Down