Skip to content

Log git checkout and expose to users #3520

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.

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

Merged
merged 26 commits into from
Feb 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
d0802ab
Log `git checkout` and expose to users
humitos Jan 15, 2018
e889166
Use `setup_env` and record for `git clone`
humitos Jan 15, 2018
19f5ba3
Use LocalEnvironment to record VCS commands in the build
humitos Jan 18, 2018
e807a24
Split LocalEnvironment into LocalBuildEnvironment and LocalEnvironment
humitos Jan 18, 2018
fdca1f5
LocalEnvironment
humitos Jan 18, 2018
ae7ad79
env fix
humitos Jan 18, 2018
ea547d8
Run VCS command in the proper environment
humitos Jan 18, 2018
6247906
Define env
humitos Jan 18, 2018
2d012fd
Proper env variables
humitos Jan 18, 2018
fd13345
Pass proper env variables when running VCS commands
humitos Jan 18, 2018
9b64427
warn_only in status
humitos Jan 18, 2018
390924d
Record command when inside a BuildEnvironment
humitos Jan 18, 2018
80c964c
Remove --quiet options
humitos Jan 18, 2018
c35b44b
Do not record ``warn_only`` commands
humitos Jan 18, 2018
eab2f96
Refactor
humitos Jan 18, 2018
66094e5
Add warn_only to other VCS
humitos Jan 18, 2018
2fb632b
Remove `update_imported_docs` and use UpdateDocsTask
humitos Jan 19, 2018
d209ec3
Hack the Environment logs to avoid fails because no version
humitos Jan 19, 2018
92368d9
Abstraction for UpdateDocsTask and SyncRepositoryTask
humitos Jan 23, 2018
f7e15aa
Allow to not record specific command
humitos Jan 23, 2018
d833b2d
Proper use of `record`, `warn_only` and the new `force_success`
humitos Jan 24, 2018
e7ce7d7
Use record_as_success instead of force_success
humitos Jan 24, 2018
7b34881
Replace force_success to record_as_success in calls
humitos Jan 24, 2018
036d460
Lint
humitos Jan 24, 2018
8c1fd17
Fix docs styling
humitos Jan 25, 2018
30b7cef
Remove unnecessary initializing variables
humitos Feb 16, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/builds.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
5 changes: 3 additions & 2 deletions readthedocs/core/management/commands/pull.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
10 changes: 7 additions & 3 deletions readthedocs/core/views/hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
186 changes: 135 additions & 51 deletions readthedocs/doc_builder/environments.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# -*- coding: utf-8 -*-

"""Documentation Builder Environments."""

from __future__ import absolute_import
Expand All @@ -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
Expand All @@ -40,7 +42,8 @@
__all__ = (
'api_v2',
'BuildCommand', 'DockerBuildCommand',
'BuildEnvironment', 'LocalEnvironment', 'DockerEnvironment',
'LocalEnvironment',
'LocalBuildEnvironment', 'DockerBuildEnvironment',
)


Expand Down Expand Up @@ -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:
Expand All @@ -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):
Expand Down Expand Up @@ -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,
}
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure we discussed this already, so apologies if we're rehashing this :)

What would be a better pattern to use here? Or is there something more specific we can assign to ourselves with this TODO?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we don't have an strong opinion in how to improve this but I was thinking on something like how the signal works: pre_something, post_something, etc. The behaviour will be the same, but it will be correctly named instead of using record for something that know nothing about recording the commands.

So, we could call methods like pre_run_command and post_run_command so inherited classes can do whatever they want (in this example, the post_run_command will record it)

What do you think? In case you like it, another PR or the same one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, interesting thought, I do like the plan. I don't think we need to do this now, but we could always turn this todo into a ticket. I have a "Refactoring" milestone that we can assign this to if so -- this would be a great place for people to jump in and clean up code.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

True, this meaning has changed now. 👍 on changing

Copy link
Contributor

Choose a reason for hiding this comment

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

However, maybe this is a later refactor after all. If we generalize the naming, we should probably also move out of doc_builder/ app.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened a ticket for this: #3541

command_class = BuildCommand


class BuildEnvironment(BaseEnvironment):

"""
Base build environment.
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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.
Expand All @@ -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(
Expand Down
5 changes: 3 additions & 2 deletions readthedocs/projects/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
19 changes: 14 additions & 5 deletions readthedocs/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)
Expand Down Expand Up @@ -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):
Expand Down
Loading