Log git checkout and expose to users (#3520)

* Log `git checkout` and expose to users

* Use `setup_env` and record for `git clone`

* Use LocalEnvironment to record VCS commands in the build

* Split LocalEnvironment into LocalBuildEnvironment and LocalEnvironment

* LocalEnvironment

* env fix

* Run VCS command in the proper environment

* Define env

* Proper env variables

* Pass proper env variables when running VCS commands

* warn_only in status

* Record command when inside a BuildEnvironment

* Remove --quiet options

* Do not record ``warn_only`` commands

* Refactor

* Add warn_only to other VCS

* Remove `update_imported_docs` and use UpdateDocsTask

* Hack the Environment logs to avoid fails because no version

* Abstraction for UpdateDocsTask and SyncRepositoryTask

These are two separated tasks that share some code by inheriting from
SyncRepositoryMixin.

The final goal (as ``core.views.hooks._build_url`` is *DEPRECATED*) is
to finally remove the SyncRepositoryTask and merge that code into
UpdateDocsTask.

* Allow to not record specific command

Either `record=False` or `warn_only=True` commands are not
recorded. This attributes can be set at initialization time or when
calling the `run()` method.

* Proper use of `record`, `warn_only` and the new `force_success`

* Use record_as_success instead of force_success

We need the real exit_code for some command since there are decisions
based on that code, but we want to record it as success.

So, the fake exit_code is saved in the database but the real exit_code
is used in the whole flow of the building process.

* Replace force_success to record_as_success in calls

* Lint

* Fix docs styling

* Remove unnecessary initializing variables

Author: humitos

docs/builds.rst

@@ -75,7 +75,7 @@ An example in code:
 
 .. code-block:: python
 
-    update_imported_docs(version)
+    update_docs_from_vcs(version)
     if exists('setup.py'):
         run('python setup.py install')
     if project.requirements_file:

readthedocs/core/management/commands/pull.py

@@ -19,6 +19,7 @@ class Command(BaseCommand):
     def handle(self, *args, **options):
         if args:
             for slug in args:
-                tasks.update_imported_docs(
-                    utils.version_from_slug(slug, LATEST).pk
+                version = utils.version_from_slug(slug, LATEST)
+                tasks.SyncRepositoryTask().run(
+                    version.pk,
                 )

readthedocs/core/views/hooks.py

@@ -12,7 +12,7 @@
 from readthedocs.builds.constants import LATEST
 from readthedocs.projects import constants
 from readthedocs.projects.models import Project, Feature
-from readthedocs.projects.tasks import update_imported_docs
+from readthedocs.projects.tasks import SyncRepositoryTask
 
 import logging
 
@@ -123,8 +123,12 @@ def _build_url(url, projects, branches):
     for project in projects:
         (built, not_building) = build_branches(project, branches)
         if not built:
-            # Call update_imported_docs to update tag/branch info
-            update_imported_docs.delay(project.versions.get(slug=LATEST).pk)
+            # Call SyncRepositoryTask to update tag/branch info
+            version = project.versions.get(slug=LATEST)
+            sync_repository = SyncRepositoryTask()
+            sync_repository.apply_async(
+                args=(version.pk,),
+            )
             msg = '(URL Build) Syncing versions for %s' % project.slug
             log.info(msg)
         all_built[project.slug] = built

readthedocs/doc_builder/environments.py

@@ -1,3 +1,5 @@
+# -*- coding: utf-8 -*-
+
 """Documentation Builder Environments."""
 
 from __future__ import absolute_import
@@ -14,7 +16,7 @@
 
 from readthedocs.core.utils import slugify
 from django.conf import settings
-from django.utils.translation import ugettext_lazy as _, ugettext_noop
+from django.utils.translation import ugettext_lazy as _
 from docker import Client
 from docker.utils import create_host_config
 from docker.errors import APIError as DockerAPIError, DockerException
@@ -40,7 +42,8 @@
 __all__ = (
     'api_v2',
     'BuildCommand', 'DockerBuildCommand',
-    'BuildEnvironment', 'LocalEnvironment', 'DockerEnvironment',
+    'LocalEnvironment',
+    'LocalBuildEnvironment', 'DockerBuildEnvironment',
 )
 
 
@@ -70,7 +73,7 @@ class BuildCommand(BuildCommandResultMixin):
 
     def __init__(self, command, cwd=None, shell=False, environment=None,
                  combine_output=True, input_data=None, build_env=None,
-                 bin_path=None, description=None):
+                 bin_path=None, description=None, record_as_success=False):
         self.command = command
         self.shell = shell
         if cwd is None:
@@ -93,6 +96,7 @@ def __init__(self, command, cwd=None, shell=False, environment=None,
         self.description = ''
         if description is not None:
             self.description = description
+        self.record_as_success = record_as_success
         self.exit_code = None
 
     def __str__(self):
@@ -180,12 +184,21 @@ def get_command(self):
 
     def save(self):
         """Save this command and result via the API."""
+        exit_code = self.exit_code
+
+        # Force record this command as success to avoid Build reporting errors
+        # on commands that are just for checking purposes and do not interferes
+        # in the Build
+        if self.record_as_success:
+            log.warning('Recording command exit_code as success')
+            exit_code = 0
+
         data = {
             'build': self.build_env.build.get('id'),
             'command': self.get_command(),
             'description': self.description,
             'output': self.output,
-            'exit_code': self.exit_code,
+            'exit_code': exit_code,
             'start_time': self.start_time,
             'end_time': self.end_time,
         }
@@ -268,7 +281,100 @@ def get_wrapped_command(self):
                                    for part in self.command]))))
 
 
-class BuildEnvironment(object):
+class BaseEnvironment(object):
+
+    """
+    Base environment class.
+
+    Used to run arbitrary commands outside a build.
+    """
+
+    def __init__(self, project, environment=None):
+        # TODO: maybe we can remove this Project dependency also
+        self.project = project
+        self.environment = environment or {}
+        self.commands = []
+
+    def record_command(self, command):
+        pass
+
+    def _log_warning(self, msg):
+        log.warning(LOG_TEMPLATE.format(
+            project=self.project.slug,
+            version='latest',
+            msg=msg,
+        ))
+
+    def run(self, *cmd, **kwargs):
+        """Shortcut to run command from environment."""
+        return self.run_command_class(cls=self.command_class, cmd=cmd, **kwargs)
+
+    def run_command_class(
+            self, cls, cmd, record=None, warn_only=False,
+            record_as_success=False, **kwargs):
+        """
+        Run command from this environment.
+
+        :param cls: command class to instantiate a command
+        :param cmd: command (as a list) to execute in this environment
+        :param record: whether or not to record this particular command
+            (``False`` implies ``warn_only=True``)
+        :param warn_only: don't raise an exception on command failure
+        :param record_as_success: force command ``exit_code`` to be saved as
+            ``0`` (``True`` implies ``warn_only=True`` and ``record=True``)
+        """
+        if record is None:
+            # ``self.record`` only exists when called from ``*BuildEnvironment``
+            record = getattr(self, 'record', False)
+
+        if not record:
+            warn_only = True
+
+        if record_as_success:
+            record = True
+            warn_only = True
+            # ``record_as_success`` is needed to instantiate the BuildCommand
+            kwargs.update({'record_as_success': record_as_success})
+
+        # Remove PATH from env, and set it to bin_path if it isn't passed in
+        env_path = self.environment.pop('BIN_PATH', None)
+        if 'bin_path' not in kwargs and env_path:
+            kwargs['bin_path'] = env_path
+        assert 'environment' not in kwargs, "environment can't be passed in via commands."
+        kwargs['environment'] = self.environment
+
+        # ``build_env`` is passed as ``kwargs`` when it's called from a
+        # ``*BuildEnvironment``
+        build_cmd = cls(cmd, **kwargs)
+        self.commands.append(build_cmd)
+        build_cmd.run()
+
+        if record:
+            # TODO: I don't like how it's handled this entry point here since
+            # this class should know nothing about a BuildCommand (which are the
+            # only ones that can be saved/recorded)
+            self.record_command(build_cmd)
+
+        if build_cmd.failed:
+            msg = u'Command {cmd} failed'.format(cmd=build_cmd.get_command())
+
+            if build_cmd.output:
+                msg += u':\n{out}'.format(out=build_cmd.output)
+
+            if warn_only:
+                self._log_warning(msg)
+            else:
+                raise BuildEnvironmentWarning(msg)
+        return build_cmd
+
+
+class LocalEnvironment(BaseEnvironment):
+
+    # TODO: BuildCommand name doesn't make sense here, should be just Command
+    command_class = BuildCommand
+
+
+class BuildEnvironment(BaseEnvironment):
 
     """
     Base build environment.
@@ -303,15 +409,13 @@ class BuildEnvironment(object):
 
     def __init__(self, project=None, version=None, build=None, config=None,
                  record=True, environment=None, update_on_success=True):
-        self.project = project
+        super(BuildEnvironment, self).__init__(project, environment)
         self.version = version
         self.build = build
         self.config = config
         self.record = record
-        self.environment = environment or {}
         self.update_on_success = update_on_success
 
-        self.commands = []
         self.failure = None
         self.start_time = datetime.utcnow()
 
@@ -348,48 +452,28 @@ def handle_exception(self, exc_type, exc_value, _):
                 self.failure = exc_value
             return True
 
-    def run(self, *cmd, **kwargs):
-        """Shortcut to run command from environment."""
-        return self.run_command_class(cls=self.command_class, cmd=cmd, **kwargs)
+    def record_command(self, command):
+        command.save()
 
-    def run_command_class(self, cls, cmd, **kwargs):
-        """
-        Run command from this environment.
-
-        Use ``cls`` to instantiate a command
-
-        :param warn_only: Don't raise an exception on command failure
-        """
-        warn_only = kwargs.pop('warn_only', False)
-        # Remove PATH from env, and set it to bin_path if it isn't passed in
-        env_path = self.environment.pop('BIN_PATH', None)
-        if 'bin_path' not in kwargs and env_path:
-            kwargs['bin_path'] = env_path
-        assert 'environment' not in kwargs, "environment can't be passed in via commands."
-        kwargs['environment'] = self.environment
-        kwargs['build_env'] = self
-        build_cmd = cls(cmd, **kwargs)
-        self.commands.append(build_cmd)
-        build_cmd.run()
-
-        # Save to database
-        if self.record:
-            build_cmd.save()
-
-        if build_cmd.failed:
-            msg = u'Command {cmd} failed'.format(cmd=build_cmd.get_command())
+    def _log_warning(self, msg):
+        # :'(
+        log.warning(LOG_TEMPLATE.format(
+            project=self.project.slug,
+            version=self.version.slug,
+            msg=msg,
+        ))
 
-            if build_cmd.output:
-                msg += u':\n{out}'.format(out=build_cmd.output)
+    def run(self, *cmd, **kwargs):
+        kwargs.update({
+            'build_env': self,
+        })
+        return super(BuildEnvironment, self).run(*cmd, **kwargs)
 
-            if warn_only:
-                log.warning(LOG_TEMPLATE
-                            .format(project=self.project.slug,
-                                    version=self.version.slug,
-                                    msg=msg))
-            else:
-                raise BuildEnvironmentWarning(msg)
-        return build_cmd
+    def run_command_class(self, *cmd, **kwargs):  # pylint: disable=arguments-differ
+        kwargs.update({
+            'build_env': self,
+        })
+        return super(BuildEnvironment, self).run_command_class(*cmd, **kwargs)
 
     @property
     def successful(self):
@@ -497,14 +581,14 @@ def update_build(self, state=None):
                 log.exception("Unknown build exception")
 
 
-class LocalEnvironment(BuildEnvironment):
+class LocalBuildEnvironment(BuildEnvironment):
 
-    """Local execution environment."""
+    """Local execution build environment."""
 
     command_class = BuildCommand
 
 
-class DockerEnvironment(BuildEnvironment):
+class DockerBuildEnvironment(BuildEnvironment):
 
     """
     Docker build environment, uses docker to contain builds.
@@ -527,7 +611,7 @@ class DockerEnvironment(BuildEnvironment):
 
     def __init__(self, *args, **kwargs):
         self.docker_socket = kwargs.pop('docker_socket', DOCKER_SOCKET)
-        super(DockerEnvironment, self).__init__(*args, **kwargs)
+        super(DockerBuildEnvironment, self).__init__(*args, **kwargs)
         self.client = None
         self.container = None
         self.container_name = slugify(

readthedocs/projects/apps.py

@@ -7,6 +7,7 @@ class ProjectsConfig(AppConfig):
     name = 'readthedocs.projects'
 
     def ready(self):
-        from readthedocs.projects.tasks import UpdateDocsTask
+        from readthedocs.projects import tasks
         from readthedocs.worker import app
-        app.tasks.register(UpdateDocsTask)
+        app.tasks.register(tasks.SyncRepositoryTask)
+        app.tasks.register(tasks.UpdateDocsTask)

readthedocs/projects/models.py

@@ -33,7 +33,6 @@
     determine_stable_version, version_windows)
 from readthedocs.restapi.client import api
 from readthedocs.vcs_support.backends import backend_cls
-from readthedocs.vcs_support.base import VCSProject
 from readthedocs.vcs_support.utils import Lock, NonBlockingLock
 
 log = logging.getLogger(__name__)
@@ -605,14 +604,24 @@ def has_htmlzip(self, version_slug=LATEST):
     def sponsored(self):
         return False
 
-    def vcs_repo(self, version=LATEST):
+    def vcs_repo(self, version=LATEST, environment=None):
+        """
+        Return a Backend object for this project able to handle VCS commands.
+
+        :param environment: environment to run the commands
+        :type environment: doc_builder.environments.BuildEnvironment
+        :param version: version slug for the backend (``LATEST`` by default)
+        :type version: str
+        """
+        # TODO: this seems to be the only method that receives a
+        # ``version.slug`` instead of a ``Version`` instance (I prefer an
+        # instance here)
+
         backend = backend_cls.get(self.repo_type)
         if not backend:
             repo = None
         else:
-            proj = VCSProject(
-                self.name, self.default_branch, self.checkout_path(version), self.clean_repo)
-            repo = backend(proj, version)
+            repo = backend(self, version, environment)
         return repo
 
     def repo_nonblockinglock(self, version, max_lock_age=5):