-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Changes from all commits
d0802ab
e889166
19f5ba3
e807a24
fdca1f5
ae7ad79
ea547d8
6247906
2d012fd
fd13345
9b64427
390924d
80c964c
c35b44b
eab2f96
66094e5
2fb632b
d209ec3
92368d9
f7e15aa
d833b2d
e7ce7d7
7b34881
036d460
8c1fd17
30b7cef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, this meaning has changed now. 👍 on changing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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( | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 usingrecord
for something that know nothing about recording the commands.So, we could call methods like
pre_run_command
andpost_run_command
so inherited classes can do whatever they want (in this example, thepost_run_command
will record it)What do you think? In case you like it, another PR or the same one?
There was a problem hiding this comment.
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.