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 2 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
27 changes: 26 additions & 1 deletion readthedocs/api/v2/views/model_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from rest_framework.renderers import BaseRenderer, JSONRenderer
from rest_framework.response import Response

from readthedocs.builds.constants import INTERNAL
from readthedocs.builds.constants import BUILD_STATE_TRIGGERED, INTERNAL
from readthedocs.builds.models import Build, BuildCommandResult, Version
from readthedocs.builds.tasks import sync_versions_task
from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
Expand Down Expand Up @@ -272,6 +272,31 @@ 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.

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


class BuildCommandViewSet(UserSelectViewSet):
parser_classes = [JSONParser, MultiPartParser]
Expand Down
4 changes: 3 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,8 @@ def run_setup(self, record=True):

Return True if successful.
"""
api_v2.build(self.build['id']).reset.post()
Copy link
Member

Choose a reason for hiding this comment

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

Seems we're calling this on every build? It would be best to run it only in the exception handler, but I'm not 100% sure that code will run properly, so this probably makes sense 👍

Copy link
Member

Choose a reason for hiding this comment

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

Thinking more, it's probably fine to have it here. There are likely other cases where we want to clear out old build commands (eg. retrying a build if we implement that), so this is probably the most explicit way of handling it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also it would be weird for all commands to disappear till the build is re-triggered. But maybe I can check if the build has any commands build['commands'] before calling the API, let me know if that's better

Copy link
Member

Choose a reason for hiding this comment

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

If we have that data, it would be a good protection. I just worry about us reseting builds that already have important data in them in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I'm not sure if a build with no commands can have other meta-data that could confuse the build


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