From d836973a04fbba01e693e74c4124f2dbceb4b57b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 29 Oct 2015 14:06:50 -0400 Subject: [PATCH 1/2] Use colors when logging warnings or errors, so they are more obvious. Signed-off-by: Daniel Nephin --- compose/cli/formatter.py | 23 +++++++++++++++++++++++ compose/cli/main.py | 22 ++++++++++++++-------- compose/config/interpolation.py | 2 +- tests/unit/cli/main_test.py | 29 +++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 9 deletions(-) diff --git a/compose/cli/formatter.py b/compose/cli/formatter.py index 9ed52c4aa..d0ed0f87e 100644 --- a/compose/cli/formatter.py +++ b/compose/cli/formatter.py @@ -1,10 +1,13 @@ from __future__ import absolute_import from __future__ import unicode_literals +import logging import os import texttable +from compose.cli import colors + def get_tty_width(): tty_size = os.popen('stty size', 'r').read().split() @@ -15,6 +18,7 @@ def get_tty_width(): class Formatter(object): + """Format tabular data for printing.""" def table(self, headers, rows): table = texttable.Texttable(max_width=get_tty_width()) table.set_cols_dtype(['t' for h in headers]) @@ -23,3 +27,22 @@ class Formatter(object): table.set_chars(['-', '|', '+', '-']) return table.draw() + + +class ConsoleWarningFormatter(logging.Formatter): + """A logging.Formatter which prints WARNING and ERROR messages with + a prefix of the log level colored appropriate for the log level. + """ + + def get_level_message(self, record): + separator = ': ' + if record.levelno == logging.WARNING: + return colors.yellow(record.levelname) + separator + if record.levelno == logging.ERROR: + return colors.red(record.levelname) + separator + + return '' + + def format(self, record): + message = super(ConsoleWarningFormatter, self).format(record) + return self.get_level_message(record) + message diff --git a/compose/cli/main.py b/compose/cli/main.py index 5505b89f5..1542f52fd 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -28,6 +28,7 @@ from .command import project_from_options from .docopt_command import DocoptCommand from .docopt_command import NoSuchCommand from .errors import UserError +from .formatter import ConsoleWarningFormatter from .formatter import Formatter from .log_printer import LogPrinter from .utils import get_version_info @@ -41,7 +42,7 @@ log = logging.getLogger(__name__) console_handler = logging.StreamHandler(sys.stderr) INSECURE_SSL_WARNING = """ -Warning: --allow-insecure-ssl is deprecated and has no effect. +--allow-insecure-ssl is deprecated and has no effect. It will be removed in a future version of Compose. """ @@ -91,13 +92,18 @@ def setup_logging(): logging.getLogger("requests").propagate = False -def setup_console_handler(verbose): - if verbose: - console_handler.setFormatter(logging.Formatter('%(name)s.%(funcName)s: %(message)s')) - console_handler.setLevel(logging.DEBUG) +def setup_console_handler(handler, verbose): + if handler.stream.isatty(): + format_class = ConsoleWarningFormatter else: - console_handler.setFormatter(logging.Formatter()) - console_handler.setLevel(logging.INFO) + format_class = logging.Formatter + + if verbose: + handler.setFormatter(format_class('%(name)s.%(funcName)s: %(message)s')) + handler.setLevel(logging.DEBUG) + else: + handler.setFormatter(format_class()) + handler.setLevel(logging.INFO) # stolen from docopt master @@ -153,7 +159,7 @@ class TopLevelCommand(DocoptCommand): return options def perform_command(self, options, handler, command_options): - setup_console_handler(options.get('--verbose')) + setup_console_handler(console_handler, options.get('--verbose')) if options['COMMAND'] in ('help', 'version'): # Skip looking up the compose file. diff --git a/compose/config/interpolation.py b/compose/config/interpolation.py index f870ab4b2..f8e1da610 100644 --- a/compose/config/interpolation.py +++ b/compose/config/interpolation.py @@ -78,7 +78,7 @@ class BlankDefaultDict(dict): except KeyError: if key not in self.missing_keys: log.warn( - "The {} variable is not set. Substituting a blank string." + "The {} variable is not set. Defaulting to a blank string." .format(key) ) self.missing_keys.append(key) diff --git a/tests/unit/cli/main_test.py b/tests/unit/cli/main_test.py index a5b369808..ee837fcd4 100644 --- a/tests/unit/cli/main_test.py +++ b/tests/unit/cli/main_test.py @@ -1,11 +1,15 @@ from __future__ import absolute_import +import logging + from compose import container from compose.cli.errors import UserError +from compose.cli.formatter import ConsoleWarningFormatter from compose.cli.log_printer import LogPrinter from compose.cli.main import attach_to_logs from compose.cli.main import build_log_printer from compose.cli.main import convergence_strategy_from_opts +from compose.cli.main import setup_console_handler from compose.project import Project from compose.service import ConvergenceStrategy from tests import mock @@ -60,6 +64,31 @@ class CLIMainTestCase(unittest.TestCase): timeout=timeout) +class SetupConsoleHandlerTestCase(unittest.TestCase): + + 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): + 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): + 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 + + class ConvergeStrategyFromOptsTestCase(unittest.TestCase): def test_invalid_opts(self): From 841ed4ed218e9d281a10a098d5f2ca81a5d4c46c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 29 Oct 2015 14:15:08 -0400 Subject: [PATCH 2/2] Remove the duplicate 'Warning' prefix now that the logger adds the prefix. Signed-off-by: Daniel Nephin --- compose/cli/main.py | 5 ++--- compose/config/validation.py | 8 +++++--- compose/service.py | 2 +- tests/unit/cli/formatter_test.py | 35 ++++++++++++++++++++++++++++++++ tests/unit/config/config_test.py | 2 +- 5 files changed, 44 insertions(+), 8 deletions(-) create mode 100644 tests/unit/cli/formatter_test.py diff --git a/compose/cli/main.py b/compose/cli/main.py index 1542f52fd..34c63e775 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -59,9 +59,8 @@ def main(): log.error(e.msg) sys.exit(1) except NoSuchCommand as e: - log.error("No such command: %s", e.command) - log.error("") - log.error("\n".join(parse_doc_section("commands:", getdoc(e.supercommand)))) + 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.error(e.explanation) diff --git a/compose/config/validation.py b/compose/config/validation.py index 8cfc405fe..542081d52 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -57,9 +57,11 @@ def format_boolean_in_environment(instance): """ if isinstance(instance, bool): log.warn( - "Warning: There is a boolean value in the 'environment' key.\n" - "Environment variables can only be strings.\nPlease add quotes to any boolean values to make them string " - "(eg, 'True', 'yes', 'N').\nThis warning will become an error in a future release. \r\n" + "There is a boolean value in the 'environment' key.\n" + "Environment variables can only be strings.\n" + "Please add quotes to any boolean values to make them string " + "(eg, 'True', 'yes', 'N').\n" + "This warning will become an error in a future release. \r\n" ) return True diff --git a/compose/service.py b/compose/service.py index ad29f87f4..8d716c0bb 100644 --- a/compose/service.py +++ b/compose/service.py @@ -862,7 +862,7 @@ class ServiceNet(object): if containers: return 'container:' + containers[0].id - log.warn("Warning: Service %s is trying to use reuse the network stack " + log.warn("Service %s is trying to use reuse the network stack " "of another service that is not running." % (self.id)) return None diff --git a/tests/unit/cli/formatter_test.py b/tests/unit/cli/formatter_test.py new file mode 100644 index 000000000..1c3b6a68e --- /dev/null +++ b/tests/unit/cli/formatter_test.py @@ -0,0 +1,35 @@ +from __future__ import absolute_import +from __future__ import unicode_literals + +import logging + +from compose.cli import colors +from compose.cli.formatter import ConsoleWarningFormatter +from tests import unittest + + +MESSAGE = 'this is the message' + + +def makeLogRecord(level): + return logging.LogRecord('name', level, 'pathame', 0, MESSAGE, (), None) + + +class ConsoleWarningFormatterTestCase(unittest.TestCase): + + def setUp(self): + self.formatter = ConsoleWarningFormatter() + + def test_format_warn(self): + output = self.formatter.format(makeLogRecord(logging.WARN)) + expected = colors.yellow('WARNING') + ': ' + assert output == expected + MESSAGE + + def test_format_error(self): + output = self.formatter.format(makeLogRecord(logging.ERROR)) + expected = colors.red('ERROR') + ': ' + assert output == expected + MESSAGE + + def test_format_info(self): + output = self.formatter.format(makeLogRecord(logging.INFO)) + assert output == MESSAGE diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 5ad7e1c0c..7246b6618 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -380,7 +380,7 @@ class ConfigTest(unittest.TestCase): @mock.patch('compose.config.validation.log') def test_logs_warning_for_boolean_in_environment(self, mock_logging): - expected_warning_msg = "Warning: There is a boolean value in the 'environment' key." + expected_warning_msg = "There is a boolean value in the 'environment' key." config.load( build_config_details( {'web': {