Merge pull request #6451 from stsewd/decouple-setup-env

Be explicit when using setup_env

Author: stsewd

readthedocs/projects/signals.py

@@ -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'])

readthedocs/projects/tasks.py

@@ -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
-            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):
@@ -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
@@ -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(
+                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
@@ -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
@@ -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
@@ -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,
@@ -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)
 
@@ -553,6 +569,10 @@ def run_build(self, docker, record):
             )
 
             try:
+                before_build.send(
+                    sender=self.version,
+                    environment=self.build_env,
+                )
                 with self.project.repo_nonblockinglock(version=self.version):
                     self.setup_python_environment()
 
@@ -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,
@@ -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
@@ -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()

readthedocs/rtd_tests/tests/test_celery.py

@@ -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
@@ -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()
@@ -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')
@@ -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,
@@ -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):

readthedocs/rtd_tests/tests/test_config_integration.py

@@ -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,
@@ -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):
@@ -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)
@@ -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'}
@@ -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()
 
@@ -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()