Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Be explicit when using setup_env #6451

Merged
merged 3 commits into from Dec 17, 2019
Merged

Be explicit when using setup_env #6451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba3f7e2
Be explicit when using setup_env
stsewd Dec 10, 2019
d81a0f4
Fix tests
stsewd Dec 10, 2019
17ab950
Pass environmemt to signals
stsewd Dec 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions readthedocs/projects/signals.py
View file
Open in desktop
Expand Up @@ -3,10 +3,10 @@
import django.dispatch


before_vcs = django.dispatch.Signal(providing_args=['version'])
before_vcs = django.dispatch.Signal(providing_args=['version', 'environmemt'])
after_vcs = django.dispatch.Signal(providing_args=['version'])

before_build = django.dispatch.Signal(providing_args=['version'])
before_build = django.dispatch.Signal(providing_args=['version', 'environmemt'])
after_build = django.dispatch.Signal(providing_args=['version'])

project_import = django.dispatch.Signal(providing_args=['project'])
Expand Down
71 changes: 45 additions & 26 deletions readthedocs/projects/tasks.py
View file
Open in desktop
Expand Up @@ -104,20 +104,21 @@ def get_version(version_pk):
version_data = api_v2.version(version_pk).get()
return APIVersion(**version_data)

def get_vcs_repo(self):
"""Get the VCS object of the current project."""
def get_vcs_repo(self, environment):
"""
Get the VCS object of the current project.

All VCS commands will be executed using `environment`.
"""
version_repo = self.project.vcs_repo(
self.version.slug,
# When called from ``SyncRepositoryTask.run`` we don't have
# a ``setup_env`` so we use just ``None`` and commands won't
# be recorded
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we create the environment with record=False when using it from SyncRepositoryTask, we don't need this hack anymore.

getattr(self, 'setup_env', None),
version=self.version.slug,
environment=environment,
verbose_name=self.version.verbose_name,
version_type=self.version.type
)
return version_repo

def sync_repo(self):
def sync_repo(self, environment):
"""Update the project's repository and hit ``sync_versions`` API."""
# Make Dirs
if not os.path.exists(self.project.doc_path):
Expand All @@ -143,7 +144,7 @@ def sync_repo(self):
'msg': msg,
}
)
version_repo = self.get_vcs_repo()
version_repo = self.get_vcs_repo(environment)
version_repo.update()
self.sync_versions(version_repo)
identifier = getattr(self, 'commit', None) or self.version.identifier
Expand Down Expand Up @@ -238,9 +239,18 @@ def run(self, version_pk): # pylint: disable=arguments-differ
try:
self.version = self.get_version(version_pk)
self.project = self.version.project
before_vcs.send(sender=self.version)

environment = LocalBuildEnvironment(
stsewd marked this conversation as resolved.
Show resolved Hide resolved
project=self.project,
version=self.version,
build=self.build,
record=False,
update_on_success=False,
)

before_vcs.send(sender=self.version, environment=environment)
with self.project.repo_nonblockinglock(version=self.version):
self.sync_repo()
self.sync_repo(environment)
return True
except RepositoryError:
# Do not log as ERROR handled exceptions
Expand Down Expand Up @@ -343,6 +353,7 @@ def __init__(
if config is not None:
self.config = config
self.task = task
# TODO: remove this
self.setup_env = None

# pylint: disable=arguments-differ
Expand Down Expand Up @@ -437,23 +448,28 @@ def run_setup(self, record=True):

Return True if successful.
"""
self.setup_env = LocalBuildEnvironment(
environment = LocalBuildEnvironment(
project=self.project,
version=self.version,
build=self.build,
record=record,
update_on_success=False,
)

# TODO: Remove.
# There is code that still depends of this attribute
# outside this function. Don't use self.setup_env for new code.
self.setup_env = environment

# Environment used for code checkout & initial configuration reading
with self.setup_env:
with environment:
try:
before_vcs.send(sender=self.version)
before_vcs.send(sender=self.version, environment=environment)
if self.project.skip:
raise ProjectBuildsSkippedError
try:
with self.project.repo_nonblockinglock(version=self.version):
self.setup_vcs()
self.setup_vcs(environment)
except vcs_support_utils.LockTimeout as e:
self.task.retry(exc=e, throw=False)
raise VersionLockedError
Expand All @@ -467,13 +483,13 @@ def run_setup(self, record=True):
)

self.save_build_config()
self.additional_vcs_operations()
self.additional_vcs_operations(environment)
finally:
after_vcs.send(sender=self.version)

if self.setup_env.failure or self.config is None:
if environment.failure or self.config is None:
msg = 'Failing build because of setup failure: {}'.format(
self.setup_env.failure,
environment.failure,
)
log.info(
LOG_TEMPLATE,
Expand All @@ -488,23 +504,23 @@ def run_setup(self, record=True):
# of VersionLockedError: this exception occurs when a build is
# triggered before the previous one has finished (e.g. two webhooks,
# one after the other)
if not isinstance(self.setup_env.failure, VersionLockedError):
if not isinstance(environment.failure, VersionLockedError):
self.send_notifications(self.version.pk, self.build['id'], email=True)

return False

if self.setup_env.successful and not self.project.has_valid_clone:
if environment.successful and not self.project.has_valid_clone:
self.set_valid_clone()

return True

def additional_vcs_operations(self):
def additional_vcs_operations(self, environment):
"""
Execution of tasks that involve the project's VCS.

All this tasks have access to the configuration object.
"""
version_repo = self.get_vcs_repo()
version_repo = self.get_vcs_repo(environment)
if version_repo.supports_submodules:
version_repo.update_submodules(self.config)

Expand Down Expand Up @@ -553,6 +569,10 @@ def run_build(self, docker, record):
)

try:
before_build.send(
sender=self.version,
environment=self.build_env,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use the environment variable here to make consistency with the setup_env.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have the environment var here, I didn't refactor build_env (there are a lot of places that depend on the order call), better to refactor it with #6452 instead of passing it around each method.

)
with self.project.repo_nonblockinglock(version=self.version):
self.setup_python_environment()

Expand Down Expand Up @@ -660,13 +680,13 @@ def get_build(build_pk):
for key, val in build.items() if key not in private_keys
}

def setup_vcs(self):
def setup_vcs(self, environment):
"""
Update the checkout of the repo to make sure it's the latest.

This also syncs versions in the DB.
"""
self.setup_env.update_build(state=BUILD_STATE_CLONING)
environment.update_build(state=BUILD_STATE_CLONING)

log.info(
LOG_TEMPLATE,
Expand All @@ -677,7 +697,7 @@ def setup_vcs(self):
}
)
try:
self.sync_repo()
self.sync_repo(environment)
except RepositoryError:
log.warning('There was an error with the repository', exc_info=True)
# Re raise the exception to stop the build at this point
Expand Down Expand Up @@ -988,7 +1008,6 @@ def build_docs(self):
:rtype: dict
"""
self.build_env.update_build(state=BUILD_STATE_BUILDING)
before_build.send(sender=self.version)

outcomes = defaultdict(lambda: False)
outcomes['html'] = self.build_docs_html()
Expand Down
53 changes: 34 additions & 19 deletions readthedocs/rtd_tests/tests/test_celery.py
View file
Open in desktop
Expand Up @@ -3,19 +3,23 @@
from os.path import exists
from tempfile import mkdtemp

from allauth.socialaccount.models import SocialAccount
from django.contrib.auth.models import User
from django_dynamic_fixture import get
from messages_extends.models import Message
from mock import MagicMock, patch

from allauth.socialaccount.models import SocialAccount

from readthedocs.builds.constants import LATEST, BUILD_STATUS_SUCCESS, EXTERNAL
from readthedocs.builds.models import Build
from readthedocs.builds.constants import (
BUILD_STATE_TRIGGERED,
BUILD_STATUS_SUCCESS,
EXTERNAL,
LATEST,
)
from readthedocs.builds.models import Build, Version
from readthedocs.doc_builder.environments import LocalBuildEnvironment
from readthedocs.doc_builder.exceptions import VersionLockedError
from readthedocs.projects import tasks
from readthedocs.builds.models import Version
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.rtd_tests.base import RTDTestCase
Expand Down Expand Up @@ -51,6 +55,22 @@ def setUp(self):
)
self.project.users.add(self.eric)

def get_update_docs_task(self, version):
build_env = LocalBuildEnvironment(
version.project, version, record=False,
)

update_docs = tasks.UpdateDocsTaskStep(
build_env=build_env,
project=version.project,
version=version,
build={
'id': 99,
'state': BUILD_STATE_TRIGGERED,
},
)
return update_docs

def tearDown(self):
shutil.rmtree(self.repo)
super().tearDown()
Expand Down Expand Up @@ -236,18 +256,16 @@ def test_check_duplicate_reserved_version_latest(self, checkout_path, api_v2):
checkout_path.return_value = local_repo

version = self.project.versions.get(slug=LATEST)
sync_repository = tasks.UpdateDocsTaskStep()
sync_repository.version = version
sync_repository.project = self.project
sync_repository = self.get_update_docs_task(version)
with self.assertRaises(RepositoryError) as e:
sync_repository.sync_repo()
sync_repository.sync_repo(sync_repository.build_env)
self.assertEqual(
str(e.exception),
RepositoryError.DUPLICATED_RESERVED_VERSIONS,
)

delete_git_branch(self.repo, 'latest')
sync_repository.sync_repo()
sync_repository.sync_repo(sync_repository.build_env)
api_v2.project().sync_versions.post.assert_called()

@patch('readthedocs.projects.tasks.api_v2')
Expand All @@ -262,11 +280,9 @@ def test_check_duplicate_reserved_version_stable(self, checkout_path, api_v2):
checkout_path.return_value = local_repo

version = self.project.versions.get(slug=LATEST)
sync_repository = tasks.UpdateDocsTaskStep()
sync_repository.version = version
sync_repository.project = self.project
sync_repository = self.get_update_docs_task(version)
with self.assertRaises(RepositoryError) as e:
sync_repository.sync_repo()
sync_repository.sync_repo(sync_repository.build_env)
self.assertEqual(
str(e.exception),
RepositoryError.DUPLICATED_RESERVED_VERSIONS,
Expand All @@ -281,11 +297,10 @@ def test_check_duplicate_no_reserved_version(self, api_v2):
create_git_tag(self.repo, 'no-reserved')

version = self.project.versions.get(slug=LATEST)
sync_repository = tasks.UpdateDocsTaskStep()
sync_repository.version = version
sync_repository.project = self.project
sync_repository.sync_repo()

sync_repository = self.get_update_docs_task(version)

sync_repository.sync_repo(sync_repository.build_env)
api_v2.project().sync_versions.post.assert_called()

def test_public_task_exception(self):
Expand Down
12 changes: 6 additions & 6 deletions readthedocs/rtd_tests/tests/test_config_integration.py
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@
from django_dynamic_fixture import get
from mock import MagicMock, PropertyMock, patch

from readthedocs.builds.constants import EXTERNAL, BUILD_STATE_TRIGGERED
from readthedocs.builds.constants import BUILD_STATE_TRIGGERED, EXTERNAL
from readthedocs.builds.models import Version
from readthedocs.config import (
ALL,
Expand All @@ -20,12 +20,12 @@
from readthedocs.config.models import PythonInstallRequirements
from readthedocs.config.tests.utils import apply_fs
from readthedocs.doc_builder.config import load_yaml_config
from readthedocs.doc_builder.constants import DOCKER_IMAGE_SETTINGS
from readthedocs.doc_builder.environments import LocalBuildEnvironment
from readthedocs.doc_builder.python_environments import Conda, Virtualenv
from readthedocs.projects import tasks
from readthedocs.projects.models import Project
from readthedocs.rtd_tests.utils import create_git_submodule, make_git_repo
from readthedocs.doc_builder.constants import DOCKER_IMAGE_SETTINGS


def create_load(config=None):
Expand Down Expand Up @@ -1062,7 +1062,7 @@ def test_submodules_include(

update_docs = self.get_update_docs_task()
checkout_path.return_value = git_repo
update_docs.additional_vcs_operations()
update_docs.additional_vcs_operations(update_docs.build_env)

args, kwargs = checkout_submodules.call_args
assert set(args[0]) == set(expected)
Expand Down Expand Up @@ -1091,7 +1091,7 @@ def test_submodules_exclude(

update_docs = self.get_update_docs_task()
checkout_path.return_value = git_repo
update_docs.additional_vcs_operations()
update_docs.additional_vcs_operations(update_docs.build_env)

args, kwargs = checkout_submodules.call_args
assert set(args[0]) == {'two', 'three'}
Expand Down Expand Up @@ -1120,7 +1120,7 @@ def test_submodules_exclude_all(

update_docs = self.get_update_docs_task()
checkout_path.return_value = git_repo
update_docs.additional_vcs_operations()
update_docs.additional_vcs_operations(update_docs.build_env)

checkout_submodules.assert_not_called()

Expand All @@ -1143,6 +1143,6 @@ def test_submodules_default_exclude_all(

update_docs = self.get_update_docs_task()
checkout_path.return_value = git_repo
update_docs.additional_vcs_operations()
update_docs.additional_vcs_operations(update_docs.build_env)

checkout_submodules.assert_not_called()