Skip to content

Refactor BaseEnvironment #3687

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.

Sign up for GitHub

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

Closed
wants to merge 15 commits into from
Closed

Refactor BaseEnvironment #3687

Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
376b825
Remove call to custom logging
stsewd Feb 27, 2018
8cb9561
Define interface for pre and post run of the command
stsewd Feb 27, 2018
a969798
Remove record_as_success from BuildCommand
stsewd Feb 27, 2018
9f30295
Override run commands on BuildEnvironment
stsewd Feb 27, 2018
c30b916
Override run commands on LocalEnvironment
stsewd Feb 27, 2018
23290a8
Linter
stsewd Feb 27, 2018
443b0e0
Refactor to mixin
stsewd Mar 2, 2018
b49e896
Send self
stsewd Mar 2, 2018
754026d
Use only record and warn_only
stsewd Mar 2, 2018
e462b48
Fix number of paramters
stsewd Mar 2, 2018
418e74b
Linter
stsewd Mar 2, 2018
a513332
Merge branch 'master' into refactor-base-environment
stsewd Mar 28, 2018
29a71f1
Merge branch 'master' into refactor-base-environment
stsewd Dec 5, 2018
e9868ac
Merge branch 'master' into refactor-base-environment
stsewd Dec 6, 2018
5d99d71
Put back build_env
stsewd Dec 6, 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
130 changes: 74 additions & 56 deletions readthedocs/doc_builder/environments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,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, record_as_success=False):
bin_path=None, description=None):
self.command = command
self.shell = shell
if cwd is None:
Expand All @@ -96,7 +96,6 @@ 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 @@ -184,21 +183,12 @@ 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': exit_code,
'exit_code': self.exit_code,
'start_time': self.start_time,
'end_time': self.end_time,
}
Expand Down Expand Up @@ -295,46 +285,36 @@ def __init__(self, project, environment=None):
self.environment = environment or {}
self.commands = []

def record_command(self, command):
def pre_run_command(self):
"""
This method is called before the command is executed.

The command that will be executed can be accessed as
the last element of ``self.commands``.
"""
pass

def _log_warning(self, msg):
log.warning(LOG_TEMPLATE.format(
project=self.project.slug,
version='latest',
msg=msg,
))
def post_run_command(self):
"""
This method is called after the command is executed.

The command that was executed can be accessed as
the last element of ``self.commands``.
"""
pass

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):
def run_command_class(self, cls, cmd, warn_only=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
Copy link
Member

Choose a reason for hiding this comment

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

We are lossing the documentation for these attributes here. It should be added where it fit.

``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:
Copy link
Member

Choose a reason for hiding this comment

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

We do need this logic to calculate these values to decide later if we want to record or just warn or both.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I know, that's because I want to eliminate those parameters and just use record and warn_only #3687 (comment)

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)
Expand All @@ -347,13 +327,9 @@ def run_command_class(
# ``*BuildEnvironment``
build_cmd = cls(cmd, **kwargs)
self.commands.append(build_cmd)
self.pre_run_command()
build_cmd.run()
Copy link
Member

Choose a reason for hiding this comment

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

self.pre_run_command shuold be before this line and should receive only the build_cmd, warn_only and **kwargs


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)
self.post_run_command()

if build_cmd.failed:
msg = u'Command {cmd} failed'.format(cmd=build_cmd.get_command())
Expand All @@ -362,7 +338,11 @@ def run_command_class(
msg += u':\n{out}'.format(out=build_cmd.output)

if warn_only:
self._log_warning(msg)
log.warning(LOG_TEMPLATE.format(
project=self.project.slug,
version='latest',
msg=msg,
))
else:
raise BuildEnvironmentWarning(msg)
return build_cmd
Expand All @@ -373,6 +353,34 @@ class LocalEnvironment(BaseEnvironment):
# TODO: BuildCommand name doesn't make sense here, should be just Command
command_class = BuildCommand

def post_run_command(self):
command = self.commands[-1]
if self.record_as_success:
command.exit_code = 0
if self.record:
command.save()

def run(self, *cmd, **kwargs):
self.record = kwargs.pop('record', False)
self.record_as_success = kwargs.pop('record_as_success', False)
if not self.record:
kwargs['warn_only'] = True
if self.record_as_success:
self.record = True
kwargs['warn_only'] = True
return super(LocalEnvironment, self).run(*cmd, **kwargs)

# record, force_success, warn_only
Copy link
Member Author

@stsewd stsewd Feb 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I see that there is some magic with the parameters on the run_command_class (change according to other parameters). So I was thinking to use different parameters: record, force_success, warn_only that are independent from each other and play nice together.

def run_command_class(self, *cmd, **kwargs): # noqa
self.record = kwargs.pop('record', False)
self.record_as_success = kwargs.pop('record_as_success', False)
if not self.record:
kwargs['warn_only'] = True
if self.record_as_success:
self.record = True
kwargs['warn_only'] = True
return super(LocalEnvironment, self).run_command_class(*cmd, **kwargs)


class BuildEnvironment(BaseEnvironment):

Expand Down Expand Up @@ -452,27 +460,37 @@ def handle_exception(self, exc_type, exc_value, _):
self.failure = exc_value
return True

def record_command(self, command):
command.save()

def _log_warning(self, msg):
# :'(
log.warning(LOG_TEMPLATE.format(
project=self.project.slug,
version=self.version.slug,
msg=msg,
))
def post_run_command(self):
command = self.commands[-1]
if self.record_as_success:
command.exit_code = 0
if self.record:
command.save()

def run(self, *cmd, **kwargs):
kwargs.update({
'build_env': self,
})
self.record = kwargs.pop('record', self.record)
self.record_as_success = kwargs.pop('record_as_success', False)
if not self.record:
kwargs['warn_only'] = True
if self.record_as_success:
self.record = True
kwargs['warn_only'] = True
return super(BuildEnvironment, self).run(*cmd, **kwargs)

def run_command_class(self, *cmd, **kwargs): # pylint: disable=arguments-differ
def run_command_class(self, *cmd, **kwargs): # noqa
kwargs.update({
'build_env': self,
})
self.record = kwargs.pop('record', True)
self.record_as_success = kwargs.pop('record_as_success', False)
if not self.record:
kwargs['warn_only'] = True
if self.record_as_success:
self.record = True
kwargs['warn_only'] = True
return super(BuildEnvironment, self).run_command_class(*cmd, **kwargs)

@property
Expand Down