EbookFoundation
/
readthedocs.org
mirror of https://github.com/EbookFoundation/readthedocs.org.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Update build status even on exception (#2573) 

* Never call update_build from a BuildEnvironment

* Fix broken celery test

This is really a design decision thing. Either projects should be marked
as having valid clones even if record is off, or they shouldn't.

Personally, I believe the former behavior is better, since we will have
actually cloned the repo at that point (in the setup_env), but this does
mean that record will record *some* data in the database, which might
not be desirable.

* Catch all exceptions when exiting BuildEnvironment

This removes the special handling of BuildEnvironmentError, and
logs and catches all exceptions which aren't subclasses of
BuildEnvironmentWarning.

This simplifies exception handling for users of BuildEnvironments,
but may have some unexpected effects if, for instance,
SystemExit, StopIteration or KeyboardInterrupt is raised while
inside a BuildEnvironment.

* Don't surface the exception message to the end user

* Raise BuildEnvironmentError on error parsing YAML
hotfix-virtualenv-no-downlaod
Luke GB 2017-01-06 23:12:01 +00:00 committed by Anthony
parent 21e1f341b3
commit 3de27742cb
5 changed files with 93 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
readthedocs/doc_builder/backends/mkdocs.py
View File

@ -7,6 +7,7 @@ from django.conf import settings
from django.template import Context, loader as template_loader
from readthedocs.doc_builder.base import BaseBuilder
from readthedocs.doc_builder.exceptions import BuildEnvironmentError
log = logging.getLogger(__name__)
@ -42,6 +43,15 @@ class BaseMkdocs(BaseBuilder):
user_config = {
'site_name': self.version.project.name,
}
except yaml.YAMLError as exc:
note = ''
if hasattr(exc, 'problem_mark'):
mark = exc.problem_mark
note = ' (line %d, column %d)' % (mark.line+1, mark.column+1)
raise BuildEnvironmentError(
"Your mkdocs.yml could not be loaded, "
"possibly due to a syntax error%s" % (
note,))
# Handle custom docs dirs

30
readthedocs/doc_builder/environments.py
View File

@ -12,7 +12,7 @@ import socket
from datetime import datetime
from django.utils.text import slugify
from django.utils.translation import ugettext_lazy as _
from django.utils.translation import ugettext_lazy as _, ugettext_noop
from docker import Client
from docker.utils import create_host_config
from docker.errors import APIError as DockerAPIError, DockerException
@ -281,7 +281,7 @@ class BuildEnvironment(object):
def __exit__(self, exc_type, exc_value, tb):
ret = self.handle_exception(exc_type, exc_value, tb)
self.update_build(state=BUILD_STATE_FINISHED)
self.build['state'] = BUILD_STATE_FINISHED
log.info(LOG_TEMPLATE
.format(project=self.project.slug,
version=self.version.slug,
@ -294,10 +294,9 @@ class BuildEnvironment(object):
This reports on the exception we're handling and special cases
subclasses of BuildEnvironmentException. For
:py:class:`BuildEnvironmentWarning`, exit this context gracefully, but
don't mark the build as a failure. For :py:class:`BuildEnvironmentError`,
exit gracefully, but mark the build as a failure. For all other
exception classes, the build will be marked as a failure and an
exception will bubble up.
don't mark the build as a failure. For all other exception classes,
including :py:class:`BuildEnvironmentError`, the build will be marked as
a failure and the context will be gracefully exited.
"""
if exc_type is not None:
log.error(LOG_TEMPLATE
@ -305,13 +304,9 @@ class BuildEnvironment(object):
version=self.version.slug,
msg=exc_value),
exc_info=True)
if issubclass(exc_type, BuildEnvironmentWarning):
return True
else:
if not issubclass(exc_type, BuildEnvironmentWarning):
self.failure = exc_value
if issubclass(exc_type, BuildEnvironmentError):
return True
return False
return True
def run(self, *cmd, **kwargs):
'''Shortcut to run command from environment'''
@ -409,7 +404,14 @@ class BuildEnvironment(object):
self.build['length'] = build_length.total_seconds()
if self.failure is not None:
self.build['error'] = str(self.failure)
# Only surface the error message if it was a
# BuildEnvironmentException or BuildEnvironmentWarning
if isinstance(self.failure,
(BuildEnvironmentException, BuildEnvironmentWarning)):
self.build['error'] = str(self.failure)
else:
self.build['error'] = ugettext_noop(
"An unexpected error occurred")
# Attempt to stop unicode errors on build reporting
for key, val in self.build.items():
@ -480,7 +482,7 @@ class DockerEnvironment(BuildEnvironment):
_('A build environment is currently '
'running for this version'))
self.failure = exc
self.update_build(state=BUILD_STATE_FINISHED)
self.build['state'] = BUILD_STATE_FINISHED
raise exc
else:
log.warn(LOG_TEMPLATE

5
readthedocs/rtd_tests/mocks/mock_api.py
View File

@ -9,6 +9,9 @@ class ProjectData(object):
def get(self):
return dict()
def put(self, x=None):
return x
def mock_version(repo):
class MockVersion(object):
@ -77,7 +80,7 @@ class MockApi(object):
return ProjectData()
def build(self, x):
return mock.Mock(**{'get.return_value': {'state': 'triggered'}})
return mock.Mock(**{'get.return_value': {'id': 123, 'state': 'triggered'}})
def command(self, x):
return mock.Mock(**{'get.return_value': {}})

37
readthedocs/rtd_tests/tests/test_celery.py
View File

@ -8,6 +8,7 @@ from django.contrib.auth.models import User
from django_dynamic_fixture import get
from mock import patch, MagicMock
from readthedocs.builds.constants import BUILD_STATE_INSTALLING, BUILD_STATE_FINISHED
from readthedocs.builds.models import Build
from readthedocs.projects.models import Project
from readthedocs.projects import tasks
@ -78,6 +79,42 @@ class TestCeleryBuilding(RTDTestCase):
intersphinx=False)
self.assertTrue(result.successful())
@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):
exc = Exception()
mock_setup_vcs.side_effect = exc
build = get(Build, project=self.project,
version=self.project.versions.first())
with mock_api(self.repo) as mapi:
result = tasks.update_docs.delay(
self.project.pk,
build_pk=build.pk,
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_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()
mock_build_docs.side_effect = exc
build = get(Build, project=self.project,
version=self.project.versions.first())
with mock_api(self.repo) as mapi:
result = tasks.update_docs.delay(
self.project.pk,
build_pk=build.pk,
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):
result = tasks.update_imported_docs.delay(self.project.pk)

42
readthedocs/rtd_tests/tests/test_doc_building.py
View File

@ -22,6 +22,8 @@ from readthedocs.rtd_tests.utils import make_test_git
from readthedocs.rtd_tests.base import RTDTestCase
from readthedocs.rtd_tests.mocks.environment import EnvironmentMockGroup
DUMMY_BUILD_ID = 123
class TestLocalEnvironment(TestCase):
'''Test execution and exception handling in environment'''
@ -44,7 +46,7 @@ class TestLocalEnvironment(TestCase):
type(self.mocks.process).returncode = PropertyMock(return_value=0)
build_env = LocalEnvironment(version=self.version, project=self.project,
build={})
build={'id': DUMMY_BUILD_ID})
with build_env:
build_env.run('echo', 'test')
self.assertTrue(self.mocks.process.communicate.called)
@ -52,6 +54,7 @@ class TestLocalEnvironment(TestCase):
self.assertTrue(build_env.successful)
self.assertEqual(len(build_env.commands), 1)
self.assertEqual(build_env.commands[0].output, u'This is okay')
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
def test_failing_execution(self):
'''Build in failing state'''
@ -60,7 +63,7 @@ class TestLocalEnvironment(TestCase):
type(self.mocks.process).returncode = PropertyMock(return_value=1)
build_env = LocalEnvironment(version=self.version, project=self.project,
build={})
build={'id': DUMMY_BUILD_ID})
with build_env:
build_env.run('echo', 'test')
self.fail('This should be unreachable')
@ -69,11 +72,12 @@ class TestLocalEnvironment(TestCase):
self.assertTrue(build_env.failed)
self.assertEqual(len(build_env.commands), 1)
self.assertEqual(build_env.commands[0].output, u'This is not okay')
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
def test_failing_execution_with_caught_exception(self):
'''Build in failing state with BuildEnvironmentError exception'''
build_env = LocalEnvironment(version=self.version, project=self.project,
build={})
build={'id': DUMMY_BUILD_ID})
with build_env:
raise BuildEnvironmentError('Foobar')
@ -82,20 +86,20 @@ class TestLocalEnvironment(TestCase):
self.assertEqual(len(build_env.commands), 0)
self.assertTrue(build_env.done)
self.assertTrue(build_env.failed)
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
def test_failing_execution_with_uncaught_exception(self):
def test_failing_execution_with_unexpected_exception(self):
'''Build in failing state with exception from code'''
build_env = LocalEnvironment(version=self.version, project=self.project,
build={})
build={'id': DUMMY_BUILD_ID})
def _inner():
with build_env:
raise Exception()
with build_env:
raise ValueError('uncaught')
self.assertRaises(Exception, _inner)
self.assertFalse(self.mocks.process.communicate.called)
self.assertTrue(build_env.done)
self.assertTrue(build_env.failed)
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
class TestDockerEnvironment(TestCase):
@ -116,7 +120,7 @@ class TestDockerEnvironment(TestCase):
def test_container_id(self):
'''Test docker build command'''
docker = DockerEnvironment(version=self.version, project=self.project,
build={'id': 123})
build={'id': DUMMY_BUILD_ID})
self.assertEqual(docker.container_id,
'build-123-project-6-pip')
@ -126,13 +130,14 @@ class TestDockerEnvironment(TestCase):
'side_effect': DockerException
})
build_env = DockerEnvironment(version=self.version, project=self.project,
build={})
build={'id': DUMMY_BUILD_ID})
def _inner():
with build_env:
self.fail('Should not hit this')
self.assertRaises(BuildEnvironmentError, _inner)
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
def test_api_failure(self):
'''Failing API error response from docker should raise exception'''
@ -146,13 +151,14 @@ class TestDockerEnvironment(TestCase):
})
build_env = DockerEnvironment(version=self.version, project=self.project,
build={})
build={'id': DUMMY_BUILD_ID})
def _inner():
with build_env:
self.fail('Should not hit this')
self.assertRaises(BuildEnvironmentError, _inner)
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
def test_command_execution(self):
'''Command execution through Docker'''
@ -163,7 +169,7 @@ class TestDockerEnvironment(TestCase):
})
build_env = DockerEnvironment(version=self.version, project=self.project,
build={'id': 123})
build={'id': DUMMY_BUILD_ID})
with build_env:
build_env.run('echo test', cwd='/tmp')
@ -177,6 +183,7 @@ class TestDockerEnvironment(TestCase):
self.assertEqual(build_env.commands[0].output, 'This is the return')
self.assertEqual(build_env.commands[0].error, None)
self.assertTrue(build_env.failed)
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
def test_command_execution_cleanup_exception(self):
'''Command execution through Docker, catch exception during cleanup'''
@ -193,13 +200,14 @@ class TestDockerEnvironment(TestCase):
})
build_env = DockerEnvironment(version=self.version, project=self.project,
build={'id': 123})
build={'id': DUMMY_BUILD_ID})
with build_env:
build_env.run('echo', 'test', cwd='/tmp')
self.mocks.docker_client.kill.assert_called_with(
'build-123-project-6-pip')
self.assertTrue(build_env.successful)
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
def test_container_already_exists(self):
'''Docker container already exists'''
@ -211,7 +219,7 @@ class TestDockerEnvironment(TestCase):
})
build_env = DockerEnvironment(version=self.version, project=self.project,
build={})
build={'id': DUMMY_BUILD_ID})
def _inner():
with build_env:
build_env.run('echo', 'test', cwd='/tmp')
@ -222,6 +230,7 @@ class TestDockerEnvironment(TestCase):
'A build environment is currently running for this version')
self.assertEqual(self.mocks.docker_client.exec_create.call_count, 0)
self.assertTrue(build_env.failed)
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
def test_container_timeout(self):
'''Docker container timeout and command failure'''
@ -241,7 +250,7 @@ class TestDockerEnvironment(TestCase):
})
build_env = DockerEnvironment(version=self.version, project=self.project,
build={})
build={'id': DUMMY_BUILD_ID})
with build_env:
build_env.run('echo', 'test', cwd='/tmp')
@ -250,6 +259,7 @@ class TestDockerEnvironment(TestCase):
'Build exited due to time out')
self.assertEqual(self.mocks.docker_client.exec_create.call_count, 1)
self.assertTrue(build_env.failed)
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
class TestBuildCommand(TestCase):