Be explicit when using setup_env

Currently when `sync_repository_task` gets triggered
we use a LocalEnvironment as environment.

We want to be explicit about respecting the docker setting
in all places where we do a clone.

Also, it's really confusing depending on the order of calls to have
self.setup_env set. Wasn't able to remove it completely, we still use it
outside the function that creates the environment.

Author: stsewd

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
+
+            environment = LocalBuildEnvironment(
+                project=self.project,
+                version=self.version,
+                build=self.build,
+                record=False,
+                update_on_success=False,
+            )
+
             before_vcs.send(sender=self.version)
             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)
                 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)
 
@@ -660,13 +676,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 +693,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