From 53bea8a72040ad4b96777e9d020029ce363f9ee7 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 29 Feb 2016 14:35:23 -0800 Subject: [PATCH 1/4] Refactor command dispatch to improve unit testing and support better error messages. Signed-off-by: Daniel Nephin --- compose/cli/docopt_command.py | 39 ++++++++--------- compose/cli/main.py | 79 +++++++++++++++++++++-------------- tests/unit/cli_test.py | 23 ++++------ 3 files changed, 75 insertions(+), 66 deletions(-) diff --git a/compose/cli/docopt_command.py b/compose/cli/docopt_command.py index 5b50189c..809a4b74 100644 --- a/compose/cli/docopt_command.py +++ b/compose/cli/docopt_command.py @@ -1,7 +1,6 @@ from __future__ import absolute_import from __future__ import unicode_literals -import sys from inspect import getdoc from docopt import docopt @@ -15,24 +14,21 @@ def docopt_full_help(docstring, *args, **kwargs): raise SystemExit(docstring) -class DocoptCommand(object): - def docopt_options(self): - return {'options_first': True} +class DocoptDispatcher(object): - def sys_dispatch(self): - self.dispatch(sys.argv[1:]) - - def dispatch(self, argv): - self.perform_command(*self.parse(argv)) + def __init__(self, command_class, options): + self.command_class = command_class + self.options = options def parse(self, argv): - options = docopt_full_help(getdoc(self), argv, **self.docopt_options()) + command_help = getdoc(self.command_class) + options = docopt_full_help(command_help, argv, **self.options) command = options['COMMAND'] if command is None: - raise SystemExit(getdoc(self)) + raise SystemExit(command_help) - handler = self.get_handler(command) + handler = get_handler(self.command_class, command) docstring = getdoc(handler) if docstring is None: @@ -41,17 +37,18 @@ class DocoptCommand(object): command_options = docopt_full_help(docstring, options['ARGS'], options_first=True) return options, handler, command_options - def get_handler(self, command): - command = command.replace('-', '_') - # we certainly want to have "exec" command, since that's what docker client has - # but in python exec is a keyword - if command == "exec": - command = "exec_command" - if not hasattr(self, command): - raise NoSuchCommand(command, self) +def get_handler(command_class, command): + command = command.replace('-', '_') + # we certainly want to have "exec" command, since that's what docker client has + # but in python exec is a keyword + if command == "exec": + command = "exec_command" - return getattr(self, command) + if not hasattr(command_class, command): + raise NoSuchCommand(command, command_class) + + return getattr(command_class, command) class NoSuchCommand(Exception): diff --git a/compose/cli/main.py b/compose/cli/main.py index afb777be..0584bf1a 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -3,6 +3,7 @@ from __future__ import print_function from __future__ import unicode_literals import contextlib +import functools import json import logging import re @@ -33,7 +34,8 @@ from ..service import NeedsBuildError from .command import friendly_error_message from .command import get_config_path_from_options from .command import project_from_options -from .docopt_command import DocoptCommand +from .docopt_command import DocoptDispatcher +from .docopt_command import get_handler from .docopt_command import NoSuchCommand from .errors import UserError from .formatter import ConsoleWarningFormatter @@ -52,19 +54,16 @@ console_handler = logging.StreamHandler(sys.stderr) def main(): setup_logging() + command = dispatch() + try: - command = TopLevelCommand() - command.sys_dispatch() + command() except (KeyboardInterrupt, signals.ShutdownException): log.error("Aborting.") sys.exit(1) except (UserError, NoSuchService, ConfigurationError) as e: log.error(e.msg) sys.exit(1) - except NoSuchCommand as e: - commands = "\n".join(parse_doc_section("commands:", getdoc(e.supercommand))) - log.error("No such command: %s\n\n%s", e.command, commands) - sys.exit(1) except APIError as e: log_api_error(e) sys.exit(1) @@ -88,6 +87,40 @@ def main(): sys.exit(1) +def dispatch(): + dispatcher = DocoptDispatcher( + TopLevelCommand, + {'options_first': True, 'version': get_version_info('compose')}) + + try: + options, handler, command_options = dispatcher.parse(sys.argv[1:]) + except NoSuchCommand as e: + commands = "\n".join(parse_doc_section("commands:", getdoc(e.supercommand))) + log.error("No such command: %s\n\n%s", e.command, commands) + sys.exit(1) + + setup_console_handler(console_handler, options.get('--verbose')) + return functools.partial(perform_command, options, handler, command_options) + + +def perform_command(options, handler, command_options): + if options['COMMAND'] in ('help', 'version'): + # Skip looking up the compose file. + handler(command_options) + return + + if options['COMMAND'] == 'config': + command = TopLevelCommand(None) + handler(command, options, command_options) + return + + project = project_from_options('.', options) + command = TopLevelCommand(project) + with friendly_error_message(): + # TODO: use self.project + handler(command, project, command_options) + + def log_api_error(e): if 'client is newer than server' in e.explanation: # we need JSON formatted errors. In the meantime... @@ -134,7 +167,7 @@ def parse_doc_section(name, source): return [s.strip() for s in pattern.findall(source)] -class TopLevelCommand(DocoptCommand): +class TopLevelCommand(object): """Define and run multi-container applications with Docker. Usage: @@ -173,26 +206,8 @@ class TopLevelCommand(DocoptCommand): """ base_dir = '.' - def docopt_options(self): - options = super(TopLevelCommand, self).docopt_options() - options['version'] = get_version_info('compose') - return options - - def perform_command(self, options, handler, command_options): - setup_console_handler(console_handler, options.get('--verbose')) - - if options['COMMAND'] in ('help', 'version'): - # Skip looking up the compose file. - handler(None, command_options) - return - - if options['COMMAND'] == 'config': - handler(options, command_options) - return - - project = project_from_options(self.base_dir, options) - with friendly_error_message(): - handler(project, command_options) + def __init__(self, project): + self.project = project def build(self, project, options): """ @@ -352,13 +367,14 @@ class TopLevelCommand(DocoptCommand): exit_code = project.client.exec_inspect(exec_id).get("ExitCode") sys.exit(exit_code) - def help(self, project, options): + @classmethod + def help(cls, options): """ Get help on a command. Usage: help COMMAND """ - handler = self.get_handler(options['COMMAND']) + handler = get_handler(cls, options['COMMAND']) raise SystemExit(getdoc(handler)) def kill(self, project, options): @@ -739,7 +755,8 @@ class TopLevelCommand(DocoptCommand): print("Aborting on container exit...") project.stop(service_names=service_names, timeout=timeout) - def version(self, project, options): + @classmethod + def version(cls, options): """ Show version informations diff --git a/tests/unit/cli_test.py b/tests/unit/cli_test.py index cbe9ea6f..c609d832 100644 --- a/tests/unit/cli_test.py +++ b/tests/unit/cli_test.py @@ -64,26 +64,20 @@ class CLITestCase(unittest.TestCase): self.assertTrue(project.client) self.assertTrue(project.services) - def test_help(self): - command = TopLevelCommand() - with self.assertRaises(SystemExit): - command.dispatch(['-h']) - def test_command_help(self): - with self.assertRaises(SystemExit) as ctx: - TopLevelCommand().dispatch(['help', 'up']) + with pytest.raises(SystemExit) as exc: + TopLevelCommand.help({'COMMAND': 'up'}) - self.assertIn('Usage: up', str(ctx.exception)) + assert 'Usage: up' in exc.exconly() def test_command_help_nonexistent(self): - with self.assertRaises(NoSuchCommand): - TopLevelCommand().dispatch(['help', 'nonexistent']) + with pytest.raises(NoSuchCommand): + TopLevelCommand.help({'COMMAND': 'nonexistent'}) @pytest.mark.xfail(IS_WINDOWS_PLATFORM, reason="requires dockerpty") @mock.patch('compose.cli.main.RunOperation', autospec=True) @mock.patch('compose.cli.main.PseudoTerminal', autospec=True) def test_run_interactive_passes_logs_false(self, mock_pseudo_terminal, mock_run_operation): - command = TopLevelCommand() mock_client = mock.create_autospec(docker.Client) project = Project.from_config( name='composetest', @@ -92,6 +86,7 @@ class CLITestCase(unittest.TestCase): 'service': {'image': 'busybox'} }), ) + command = TopLevelCommand(project) with pytest.raises(SystemExit): command.run(project, { @@ -126,7 +121,7 @@ class CLITestCase(unittest.TestCase): }), ) - command = TopLevelCommand() + command = TopLevelCommand(project) command.run(project, { 'SERVICE': 'service', 'COMMAND': None, @@ -147,7 +142,7 @@ class CLITestCase(unittest.TestCase): 'always' ) - command = TopLevelCommand() + command = TopLevelCommand(project) command.run(project, { 'SERVICE': 'service', 'COMMAND': None, @@ -168,7 +163,6 @@ class CLITestCase(unittest.TestCase): ) def test_command_manula_and_service_ports_together(self): - command = TopLevelCommand() project = Project.from_config( name='composetest', client=None, @@ -176,6 +170,7 @@ class CLITestCase(unittest.TestCase): 'service': {'image': 'busybox'}, }), ) + command = TopLevelCommand(project) with self.assertRaises(UserError): command.run(project, { From 9f9dcc098a8f5df7168e8c9574ab2aebe8bf808a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 8 Mar 2016 14:35:54 -0500 Subject: [PATCH 2/4] Make TopLevelCommand use the project field. Signed-off-by: Daniel Nephin --- compose/cli/main.py | 101 ++++++++++++++++++++--------------------- tests/unit/cli_test.py | 8 ++-- 2 files changed, 54 insertions(+), 55 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 0584bf1a..b020a14b 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -117,8 +117,7 @@ def perform_command(options, handler, command_options): project = project_from_options('.', options) command = TopLevelCommand(project) with friendly_error_message(): - # TODO: use self.project - handler(command, project, command_options) + handler(command, command_options) def log_api_error(e): @@ -204,12 +203,12 @@ class TopLevelCommand(object): up Create and start containers version Show the Docker-Compose version information """ - base_dir = '.' - def __init__(self, project): + def __init__(self, project, project_dir='.'): self.project = project + self.project_dir = '.' - def build(self, project, options): + def build(self, options): """ Build or rebuild services. @@ -224,7 +223,7 @@ class TopLevelCommand(object): --no-cache Do not use cache when building the image. --pull Always attempt to pull a newer version of the image. """ - project.build( + self.project.build( service_names=options['SERVICE'], no_cache=bool(options.get('--no-cache', False)), pull=bool(options.get('--pull', False)), @@ -243,7 +242,7 @@ class TopLevelCommand(object): """ config_path = get_config_path_from_options(config_options) - compose_config = config.load(config.find(self.base_dir, config_path)) + compose_config = config.load(config.find(self.project_dir, config_path)) if options['--quiet']: return @@ -254,7 +253,7 @@ class TopLevelCommand(object): print(serialize_config(compose_config)) - def create(self, project, options): + def create(self, options): """ Creates containers for a service. @@ -270,13 +269,13 @@ class TopLevelCommand(object): """ service_names = options['SERVICE'] - project.create( + self.project.create( service_names=service_names, strategy=convergence_strategy_from_opts(options), do_build=build_action_from_opts(options), ) - def down(self, project, options): + def down(self, options): """ Stop containers and remove containers, networks, volumes, and images created by `up`. Only containers and networks are removed by default. @@ -290,9 +289,9 @@ class TopLevelCommand(object): -v, --volumes Remove data volumes """ image_type = image_type_from_opt('--rmi', options['--rmi']) - project.down(image_type, options['--volumes']) + self.project.down(image_type, options['--volumes']) - def events(self, project, options): + def events(self, options): """ Receive real time events from containers. @@ -311,12 +310,12 @@ class TopLevelCommand(object): event['time'] = event['time'].isoformat() return json.dumps(event) - for event in project.events(): + for event in self.project.events(): formatter = json_format_event if options['--json'] else format_event print(formatter(event)) sys.stdout.flush() - def exec_command(self, project, options): + def exec_command(self, options): """ Execute a command in a running container @@ -332,7 +331,7 @@ class TopLevelCommand(object): instances of a service [default: 1] """ index = int(options.get('--index')) - service = project.get_service(options['SERVICE']) + service = self.project.get_service(options['SERVICE']) try: container = service.get_container(number=index) except ValueError as e: @@ -356,15 +355,15 @@ class TopLevelCommand(object): signals.set_signal_handler_to_shutdown() try: operation = ExecOperation( - project.client, + self.project.client, exec_id, interactive=tty, ) - pty = PseudoTerminal(project.client, operation) + pty = PseudoTerminal(self.project.client, operation) pty.start() except signals.ShutdownException: log.info("received shutdown exception: closing") - exit_code = project.client.exec_inspect(exec_id).get("ExitCode") + exit_code = self.project.client.exec_inspect(exec_id).get("ExitCode") sys.exit(exit_code) @classmethod @@ -377,7 +376,7 @@ class TopLevelCommand(object): handler = get_handler(cls, options['COMMAND']) raise SystemExit(getdoc(handler)) - def kill(self, project, options): + def kill(self, options): """ Force stop service containers. @@ -389,9 +388,9 @@ class TopLevelCommand(object): """ signal = options.get('-s', 'SIGKILL') - project.kill(service_names=options['SERVICE'], signal=signal) + self.project.kill(service_names=options['SERVICE'], signal=signal) - def logs(self, project, options): + def logs(self, options): """ View output from containers. @@ -404,7 +403,7 @@ class TopLevelCommand(object): --tail="all" Number of lines to show from the end of the logs for each container. """ - containers = project.containers(service_names=options['SERVICE'], stopped=True) + containers = self.project.containers(service_names=options['SERVICE'], stopped=True) monochrome = options['--no-color'] tail = options['--tail'] @@ -421,16 +420,16 @@ class TopLevelCommand(object): print("Attaching to", list_containers(containers)) LogPrinter(containers, monochrome=monochrome, log_args=log_args).run() - def pause(self, project, options): + def pause(self, options): """ Pause services. Usage: pause [SERVICE...] """ - containers = project.pause(service_names=options['SERVICE']) + containers = self.project.pause(service_names=options['SERVICE']) exit_if(not containers, 'No containers to pause', 1) - def port(self, project, options): + def port(self, options): """ Print the public port for a port binding. @@ -442,7 +441,7 @@ class TopLevelCommand(object): instances of a service [default: 1] """ index = int(options.get('--index')) - service = project.get_service(options['SERVICE']) + service = self.project.get_service(options['SERVICE']) try: container = service.get_container(number=index) except ValueError as e: @@ -451,7 +450,7 @@ class TopLevelCommand(object): options['PRIVATE_PORT'], protocol=options.get('--protocol') or 'tcp') or '') - def ps(self, project, options): + def ps(self, options): """ List containers. @@ -461,8 +460,8 @@ class TopLevelCommand(object): -q Only display IDs """ containers = sorted( - project.containers(service_names=options['SERVICE'], stopped=True) + - project.containers(service_names=options['SERVICE'], one_off=True), + self.project.containers(service_names=options['SERVICE'], stopped=True) + + self.project.containers(service_names=options['SERVICE'], one_off=True), key=attrgetter('name')) if options['-q']: @@ -488,7 +487,7 @@ class TopLevelCommand(object): ]) print(Formatter().table(headers, rows)) - def pull(self, project, options): + def pull(self, options): """ Pulls images for services. @@ -497,12 +496,12 @@ class TopLevelCommand(object): Options: --ignore-pull-failures Pull what it can and ignores images with pull failures. """ - project.pull( + self.project.pull( service_names=options['SERVICE'], ignore_pull_failures=options.get('--ignore-pull-failures') ) - def rm(self, project, options): + def rm(self, options): """ Remove stopped service containers. @@ -517,21 +516,21 @@ class TopLevelCommand(object): -f, --force Don't ask to confirm removal -v Remove volumes associated with containers """ - all_containers = project.containers(service_names=options['SERVICE'], stopped=True) + all_containers = self.project.containers(service_names=options['SERVICE'], stopped=True) stopped_containers = [c for c in all_containers if not c.is_running] if len(stopped_containers) > 0: print("Going to remove", list_containers(stopped_containers)) if options.get('--force') \ or yesno("Are you sure? [yN] ", default=False): - project.remove_stopped( + self.project.remove_stopped( service_names=options['SERVICE'], v=options.get('-v', False) ) else: print("No stopped containers") - def run(self, project, options): + def run(self, options): """ Run a one-off command on a service. @@ -560,7 +559,7 @@ class TopLevelCommand(object): -T Disable pseudo-tty allocation. By default `docker-compose run` allocates a TTY. """ - service = project.get_service(options['SERVICE']) + service = self.project.get_service(options['SERVICE']) detach = options['-d'] if IS_WINDOWS_PLATFORM and not detach: @@ -608,9 +607,9 @@ class TopLevelCommand(object): if options['--name']: container_options['name'] = options['--name'] - run_one_off_container(container_options, project, service, options) + run_one_off_container(container_options, self.project, service, options) - def scale(self, project, options): + def scale(self, options): """ Set number of containers to run for a service. @@ -636,18 +635,18 @@ class TopLevelCommand(object): except ValueError: raise UserError('Number of containers for service "%s" is not a ' 'number' % service_name) - project.get_service(service_name).scale(num, timeout=timeout) + self.project.get_service(service_name).scale(num, timeout=timeout) - def start(self, project, options): + def start(self, options): """ Start existing containers. Usage: start [SERVICE...] """ - containers = project.start(service_names=options['SERVICE']) + containers = self.project.start(service_names=options['SERVICE']) exit_if(not containers, 'No containers to start', 1) - def stop(self, project, options): + def stop(self, options): """ Stop running containers without removing them. @@ -660,9 +659,9 @@ class TopLevelCommand(object): (default: 10) """ timeout = int(options.get('--timeout') or DEFAULT_TIMEOUT) - project.stop(service_names=options['SERVICE'], timeout=timeout) + self.project.stop(service_names=options['SERVICE'], timeout=timeout) - def restart(self, project, options): + def restart(self, options): """ Restart running containers. @@ -673,19 +672,19 @@ class TopLevelCommand(object): (default: 10) """ timeout = int(options.get('--timeout') or DEFAULT_TIMEOUT) - containers = project.restart(service_names=options['SERVICE'], timeout=timeout) + containers = self.project.restart(service_names=options['SERVICE'], timeout=timeout) exit_if(not containers, 'No containers to restart', 1) - def unpause(self, project, options): + def unpause(self, options): """ Unpause services. Usage: unpause [SERVICE...] """ - containers = project.unpause(service_names=options['SERVICE']) + containers = self.project.unpause(service_names=options['SERVICE']) exit_if(not containers, 'No containers to unpause', 1) - def up(self, project, options): + def up(self, options): """ Builds, (re)creates, starts, and attaches to containers for a service. @@ -735,8 +734,8 @@ class TopLevelCommand(object): if detached and cascade_stop: raise UserError("--abort-on-container-exit and -d cannot be combined.") - with up_shutdown_context(project, service_names, timeout, detached): - to_attach = project.up( + with up_shutdown_context(self.project, service_names, timeout, detached): + to_attach = self.project.up( service_names=service_names, start_deps=start_deps, strategy=convergence_strategy_from_opts(options), @@ -753,7 +752,7 @@ class TopLevelCommand(object): if cascade_stop: print("Aborting on container exit...") - project.stop(service_names=service_names, timeout=timeout) + self.project.stop(service_names=service_names, timeout=timeout) @classmethod def version(cls, options): diff --git a/tests/unit/cli_test.py b/tests/unit/cli_test.py index c609d832..1d7c13e7 100644 --- a/tests/unit/cli_test.py +++ b/tests/unit/cli_test.py @@ -89,7 +89,7 @@ class CLITestCase(unittest.TestCase): command = TopLevelCommand(project) with pytest.raises(SystemExit): - command.run(project, { + command.run({ 'SERVICE': 'service', 'COMMAND': None, '-e': [], @@ -122,7 +122,7 @@ class CLITestCase(unittest.TestCase): ) command = TopLevelCommand(project) - command.run(project, { + command.run({ 'SERVICE': 'service', 'COMMAND': None, '-e': [], @@ -143,7 +143,7 @@ class CLITestCase(unittest.TestCase): ) command = TopLevelCommand(project) - command.run(project, { + command.run({ 'SERVICE': 'service', 'COMMAND': None, '-e': [], @@ -173,7 +173,7 @@ class CLITestCase(unittest.TestCase): command = TopLevelCommand(project) with self.assertRaises(UserError): - command.run(project, { + command.run({ 'SERVICE': 'service', 'COMMAND': None, '-e': [], From 886328640f5665c337bb9dd1f065cc0e350364f0 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 8 Mar 2016 14:42:51 -0500 Subject: [PATCH 3/4] Convert some cli tests to pytest. Signed-off-by: Daniel Nephin --- tests/unit/cli/main_test.py | 67 +++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/tests/unit/cli/main_test.py b/tests/unit/cli/main_test.py index e7c52003..9b24776f 100644 --- a/tests/unit/cli/main_test.py +++ b/tests/unit/cli/main_test.py @@ -3,6 +3,8 @@ from __future__ import unicode_literals import logging +import pytest + from compose import container from compose.cli.errors import UserError from compose.cli.formatter import ConsoleWarningFormatter @@ -11,7 +13,6 @@ from compose.cli.main import convergence_strategy_from_opts from compose.cli.main import setup_console_handler from compose.service import ConvergenceStrategy from tests import mock -from tests import unittest def mock_container(service, number): @@ -22,7 +23,14 @@ def mock_container(service, number): name_without_project='{0}_{1}'.format(service, number)) -class CLIMainTestCase(unittest.TestCase): +@pytest.fixture +def logging_handler(): + stream = mock.Mock() + stream.isatty.return_value = True + return logging.StreamHandler(stream=stream) + + +class TestCLIMainTestCase(object): def test_build_log_printer(self): containers = [ @@ -34,7 +42,7 @@ class CLIMainTestCase(unittest.TestCase): ] service_names = ['web', 'db'] log_printer = build_log_printer(containers, service_names, True, False, {'follow': True}) - self.assertEqual(log_printer.containers, containers[:3]) + assert log_printer.containers == containers[:3] def test_build_log_printer_all_services(self): containers = [ @@ -44,58 +52,53 @@ class CLIMainTestCase(unittest.TestCase): ] service_names = [] log_printer = build_log_printer(containers, service_names, True, False, {'follow': True}) - self.assertEqual(log_printer.containers, containers) + assert log_printer.containers == containers -class SetupConsoleHandlerTestCase(unittest.TestCase): +class TestSetupConsoleHandlerTestCase(object): - def setUp(self): - self.stream = mock.Mock() - self.stream.isatty.return_value = True - self.handler = logging.StreamHandler(stream=self.stream) + def test_with_tty_verbose(self, logging_handler): + setup_console_handler(logging_handler, True) + assert type(logging_handler.formatter) == ConsoleWarningFormatter + assert '%(name)s' in logging_handler.formatter._fmt + assert '%(funcName)s' in logging_handler.formatter._fmt - def test_with_tty_verbose(self): - setup_console_handler(self.handler, True) - assert type(self.handler.formatter) == ConsoleWarningFormatter - assert '%(name)s' in self.handler.formatter._fmt - assert '%(funcName)s' in self.handler.formatter._fmt + def test_with_tty_not_verbose(self, logging_handler): + setup_console_handler(logging_handler, False) + assert type(logging_handler.formatter) == ConsoleWarningFormatter + assert '%(name)s' not in logging_handler.formatter._fmt + assert '%(funcName)s' not in logging_handler.formatter._fmt - def test_with_tty_not_verbose(self): - setup_console_handler(self.handler, False) - assert type(self.handler.formatter) == ConsoleWarningFormatter - assert '%(name)s' not in self.handler.formatter._fmt - assert '%(funcName)s' not in self.handler.formatter._fmt - - def test_with_not_a_tty(self): - self.stream.isatty.return_value = False - setup_console_handler(self.handler, False) - assert type(self.handler.formatter) == logging.Formatter + def test_with_not_a_tty(self, logging_handler): + logging_handler.stream.isatty.return_value = False + setup_console_handler(logging_handler, False) + assert type(logging_handler.formatter) == logging.Formatter -class ConvergeStrategyFromOptsTestCase(unittest.TestCase): +class TestConvergeStrategyFromOptsTestCase(object): def test_invalid_opts(self): options = {'--force-recreate': True, '--no-recreate': True} - with self.assertRaises(UserError): + with pytest.raises(UserError): convergence_strategy_from_opts(options) def test_always(self): options = {'--force-recreate': True, '--no-recreate': False} - self.assertEqual( - convergence_strategy_from_opts(options), + assert ( + convergence_strategy_from_opts(options) == ConvergenceStrategy.always ) def test_never(self): options = {'--force-recreate': False, '--no-recreate': True} - self.assertEqual( - convergence_strategy_from_opts(options), + assert ( + convergence_strategy_from_opts(options) == ConvergenceStrategy.never ) def test_changed(self): options = {'--force-recreate': False, '--no-recreate': False} - self.assertEqual( - convergence_strategy_from_opts(options), + assert ( + convergence_strategy_from_opts(options) == ConvergenceStrategy.changed ) From 0a091055d24bec9501e918353fe1c5009f88dc0c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 8 Mar 2016 15:39:11 -0500 Subject: [PATCH 4/4] Improve handling of connection errors and error messages. Signed-off-by: Daniel Nephin --- compose/cli/command.py | 35 ++------- compose/cli/errors.py | 125 +++++++++++++++++++++++++-------- compose/cli/main.py | 39 ++-------- tests/unit/cli/command_test.py | 16 ----- tests/unit/cli/errors_test.py | 51 ++++++++++++++ 5 files changed, 153 insertions(+), 113 deletions(-) create mode 100644 tests/unit/cli/errors_test.py diff --git a/compose/cli/command.py b/compose/cli/command.py index 98de2104..55f6df01 100644 --- a/compose/cli/command.py +++ b/compose/cli/command.py @@ -1,52 +1,25 @@ from __future__ import absolute_import from __future__ import unicode_literals -import contextlib import logging import os import re import six -from requests.exceptions import ConnectionError -from requests.exceptions import SSLError -from . import errors from . import verbose_proxy from .. import config from ..const import API_VERSIONS from ..project import Project from .docker_client import docker_client -from .utils import call_silently from .utils import get_version_info -from .utils import is_mac -from .utils import is_ubuntu log = logging.getLogger(__name__) -@contextlib.contextmanager -def friendly_error_message(): - try: - yield - except SSLError as e: - raise errors.UserError('SSL error: %s' % e) - except ConnectionError: - if call_silently(['which', 'docker']) != 0: - if is_mac(): - raise errors.DockerNotFoundMac() - elif is_ubuntu(): - raise errors.DockerNotFoundUbuntu() - else: - raise errors.DockerNotFoundGeneric() - elif call_silently(['which', 'docker-machine']) == 0: - raise errors.ConnectionErrorDockerMachine() - else: - raise errors.ConnectionErrorGeneric(get_client().base_url) - - -def project_from_options(base_dir, options): +def project_from_options(project_dir, options): return get_project( - base_dir, + project_dir, get_config_path_from_options(options), project_name=options.get('--project-name'), verbose=options.get('--verbose'), @@ -76,8 +49,8 @@ def get_client(verbose=False, version=None): return client -def get_project(base_dir, config_path=None, project_name=None, verbose=False): - config_details = config.find(base_dir, config_path) +def get_project(project_dir, config_path=None, project_name=None, verbose=False): + config_details = config.find(project_dir, config_path) project_name = get_project_name(config_details.working_dir, project_name) config_data = config.load(config_details) diff --git a/compose/cli/errors.py b/compose/cli/errors.py index 03d6a50c..a16cad2f 100644 --- a/compose/cli/errors.py +++ b/compose/cli/errors.py @@ -1,10 +1,27 @@ from __future__ import absolute_import from __future__ import unicode_literals +import contextlib +import logging from textwrap import dedent +from docker.errors import APIError +from requests.exceptions import ConnectionError as RequestsConnectionError +from requests.exceptions import ReadTimeout +from requests.exceptions import SSLError + +from ..const import API_VERSION_TO_ENGINE_VERSION +from ..const import HTTP_TIMEOUT +from .utils import call_silently +from .utils import is_mac +from .utils import is_ubuntu + + +log = logging.getLogger(__name__) + class UserError(Exception): + def __init__(self, msg): self.msg = dedent(msg).strip() @@ -14,44 +31,90 @@ class UserError(Exception): __str__ = __unicode__ -class DockerNotFoundMac(UserError): - def __init__(self): - super(DockerNotFoundMac, self).__init__(""" - Couldn't connect to Docker daemon. You might need to install docker-osx: - - https://github.com/noplay/docker-osx - """) +class ConnectionError(Exception): + pass -class DockerNotFoundUbuntu(UserError): - def __init__(self): - super(DockerNotFoundUbuntu, self).__init__(""" - Couldn't connect to Docker daemon. You might need to install Docker: - - https://docs.docker.com/engine/installation/ubuntulinux/ - """) +@contextlib.contextmanager +def handle_connection_errors(client): + try: + yield + except SSLError as e: + log.error('SSL error: %s' % e) + raise ConnectionError() + except RequestsConnectionError: + if call_silently(['which', 'docker']) != 0: + if is_mac(): + exit_with_error(docker_not_found_mac) + if is_ubuntu(): + exit_with_error(docker_not_found_ubuntu) + exit_with_error(docker_not_found_generic) + if call_silently(['which', 'docker-machine']) == 0: + exit_with_error(conn_error_docker_machine) + exit_with_error(conn_error_generic.format(url=client.base_url)) + except APIError as e: + log_api_error(e, client.api_version) + raise ConnectionError() + except ReadTimeout as e: + log.error( + "An HTTP request took too long to complete. Retry with --verbose to " + "obtain debug information.\n" + "If you encounter this issue regularly because of slow network " + "conditions, consider setting COMPOSE_HTTP_TIMEOUT to a higher " + "value (current value: %s)." % HTTP_TIMEOUT) + raise ConnectionError() -class DockerNotFoundGeneric(UserError): - def __init__(self): - super(DockerNotFoundGeneric, self).__init__(""" - Couldn't connect to Docker daemon. You might need to install Docker: +def log_api_error(e, client_version): + if 'client is newer than server' not in e.explanation: + log.error(e.explanation) + return - https://docs.docker.com/engine/installation/ - """) + version = API_VERSION_TO_ENGINE_VERSION.get(client_version) + if not version: + # They've set a custom API version + log.error(e.explanation) + return + + log.error( + "The Docker Engine version is less than the minimum required by " + "Compose. Your current project requires a Docker Engine of " + "version {version} or greater.".format(version=version)) -class ConnectionErrorDockerMachine(UserError): - def __init__(self): - super(ConnectionErrorDockerMachine, self).__init__(""" - Couldn't connect to Docker daemon - you might need to run `docker-machine start default`. - """) +def exit_with_error(msg): + log.error(dedent(msg).strip()) + raise ConnectionError() -class ConnectionErrorGeneric(UserError): - def __init__(self, url): - super(ConnectionErrorGeneric, self).__init__(""" - Couldn't connect to Docker daemon at %s - is it running? +docker_not_found_mac = """ + Couldn't connect to Docker daemon. You might need to install docker-osx: - If it's at a non-standard location, specify the URL with the DOCKER_HOST environment variable. - """ % url) + https://github.com/noplay/docker-osx +""" + + +docker_not_found_ubuntu = """ + Couldn't connect to Docker daemon. You might need to install Docker: + + https://docs.docker.com/engine/installation/ubuntulinux/ +""" + + +docker_not_found_generic = """ + Couldn't connect to Docker daemon. You might need to install Docker: + + https://docs.docker.com/engine/installation/ +""" + + +conn_error_docker_machine = """ + Couldn't connect to Docker daemon - you might need to run `docker-machine start default`. +""" + + +conn_error_generic = """ + Couldn't connect to Docker daemon at {url} - is it running? + + If it's at a non-standard location, specify the URL with the DOCKER_HOST environment variable. +""" diff --git a/compose/cli/main.py b/compose/cli/main.py index b020a14b..66362168 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -11,18 +11,14 @@ import sys from inspect import getdoc from operator import attrgetter -from docker.errors import APIError -from requests.exceptions import ReadTimeout - +from . import errors from . import signals from .. import __version__ from ..config import config from ..config import ConfigurationError from ..config import parse_environment from ..config.serialize import serialize_config -from ..const import API_VERSION_TO_ENGINE_VERSION from ..const import DEFAULT_TIMEOUT -from ..const import HTTP_TIMEOUT from ..const import IS_WINDOWS_PLATFORM from ..progress_stream import StreamOutputError from ..project import NoSuchService @@ -31,7 +27,6 @@ from ..service import BuildError from ..service import ConvergenceStrategy from ..service import ImageType from ..service import NeedsBuildError -from .command import friendly_error_message from .command import get_config_path_from_options from .command import project_from_options from .docopt_command import DocoptDispatcher @@ -53,7 +48,6 @@ console_handler = logging.StreamHandler(sys.stderr) def main(): - setup_logging() command = dispatch() try: @@ -64,9 +58,6 @@ def main(): except (UserError, NoSuchService, ConfigurationError) as e: log.error(e.msg) sys.exit(1) - except APIError as e: - log_api_error(e) - sys.exit(1) except BuildError as e: log.error("Service '%s' failed to build: %s" % (e.service.name, e.reason)) sys.exit(1) @@ -76,18 +67,12 @@ def main(): except NeedsBuildError as e: log.error("Service '%s' needs to be built, but --no-build was passed." % e.service.name) sys.exit(1) - except ReadTimeout as e: - log.error( - "An HTTP request took too long to complete. Retry with --verbose to " - "obtain debug information.\n" - "If you encounter this issue regularly because of slow network " - "conditions, consider setting COMPOSE_HTTP_TIMEOUT to a higher " - "value (current value: %s)." % HTTP_TIMEOUT - ) + except errors.ConnectionError: sys.exit(1) def dispatch(): + setup_logging() dispatcher = DocoptDispatcher( TopLevelCommand, {'options_first': True, 'version': get_version_info('compose')}) @@ -116,26 +101,10 @@ def perform_command(options, handler, command_options): project = project_from_options('.', options) command = TopLevelCommand(project) - with friendly_error_message(): + with errors.handle_connection_errors(project.client): handler(command, command_options) -def log_api_error(e): - if 'client is newer than server' in e.explanation: - # we need JSON formatted errors. In the meantime... - # TODO: fix this by refactoring project dispatch - # http://github.com/docker/compose/pull/2832#commitcomment-15923800 - client_version = e.explanation.split('client API version: ')[1].split(',')[0] - log.error( - "The engine version is lesser than the minimum required by " - "compose. Your current project requires a Docker Engine of " - "version {version} or superior.".format( - version=API_VERSION_TO_ENGINE_VERSION[client_version] - )) - else: - log.error(e.explanation) - - def setup_logging(): root_logger = logging.getLogger() root_logger.addHandler(console_handler) diff --git a/tests/unit/cli/command_test.py b/tests/unit/cli/command_test.py index 1ca671fe..b524a5f3 100644 --- a/tests/unit/cli/command_test.py +++ b/tests/unit/cli/command_test.py @@ -4,28 +4,12 @@ from __future__ import unicode_literals import os import pytest -from requests.exceptions import ConnectionError -from compose.cli import errors -from compose.cli.command import friendly_error_message from compose.cli.command import get_config_path_from_options from compose.const import IS_WINDOWS_PLATFORM from tests import mock -class TestFriendlyErrorMessage(object): - - def test_dispatch_generic_connection_error(self): - with pytest.raises(errors.ConnectionErrorGeneric): - with mock.patch( - 'compose.cli.command.call_silently', - autospec=True, - side_effect=[0, 1] - ): - with friendly_error_message(): - raise ConnectionError() - - class TestGetConfigPathFromOptions(object): def test_path_from_options(self): diff --git a/tests/unit/cli/errors_test.py b/tests/unit/cli/errors_test.py new file mode 100644 index 00000000..a99356b4 --- /dev/null +++ b/tests/unit/cli/errors_test.py @@ -0,0 +1,51 @@ +from __future__ import absolute_import +from __future__ import unicode_literals + +import pytest +from docker.errors import APIError +from requests.exceptions import ConnectionError + +from compose.cli import errors +from compose.cli.errors import handle_connection_errors +from tests import mock + + +@pytest.yield_fixture +def mock_logging(): + with mock.patch('compose.cli.errors.log', autospec=True) as mock_log: + yield mock_log + + +def patch_call_silently(side_effect): + return mock.patch( + 'compose.cli.errors.call_silently', + autospec=True, + side_effect=side_effect) + + +class TestHandleConnectionErrors(object): + + def test_generic_connection_error(self, mock_logging): + with pytest.raises(errors.ConnectionError): + with patch_call_silently([0, 1]): + with handle_connection_errors(mock.Mock()): + raise ConnectionError() + + _, args, _ = mock_logging.error.mock_calls[0] + assert "Couldn't connect to Docker daemon at" in args[0] + + def test_api_error_version_mismatch(self, mock_logging): + with pytest.raises(errors.ConnectionError): + with handle_connection_errors(mock.Mock(api_version='1.22')): + raise APIError(None, None, "client is newer than server") + + _, args, _ = mock_logging.error.mock_calls[0] + assert "Docker Engine of version 1.10.0 or greater" in args[0] + + def test_api_error_version_other(self, mock_logging): + msg = "Something broke!" + with pytest.raises(errors.ConnectionError): + with handle_connection_errors(mock.Mock(api_version='1.22')): + raise APIError(None, None, msg) + + mock_logging.error.assert_called_once_with(msg)