From a7a8e060e37fad21a9702ac0f143fd39609c8c2d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 6 Jul 2017 13:50:13 -0400 Subject: [PATCH] Finish adding configurable log rotation * Update log backupCount name and description. * Add additional error handling to --log-backups * test --log-backups flag * Pass log_backups value to RotatingFileHandler * Test that log_backups is properly used * add _test_success_common * Add test_success_with_rollover * Add test_success_without_rollover * mock stderr in cli tests * Set log_backups in PostArgParseSetupTest * Rename "log backups" to "max log backups" --- certbot/cli.py | 38 +++++++++++++++++++++++++++++--------- certbot/log.py | 3 ++- certbot/tests/cli_test.py | 12 ++++++++++++ certbot/tests/log_test.py | 27 ++++++++++++++++++++++----- 4 files changed, 65 insertions(+), 15 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index c74a8102f..eaafce762 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -850,15 +850,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis None, "-t", "--text", dest="text_mode", action="store_true", help=argparse.SUPPRESS) helpful.add( - # Note, 1 is subtracted from max_log_count in certbot/main.py - #> to correct an off by one error. - [None, "paths"], - "--max-log-count", dest="max_log_count", - type=int, default=1000, - help="Specifies the maximum number of certbot log files " - "that will be kept. 0 disables log rotation. 1 causes " - "only the log from the most recent run to be kept. " - "2+ enables log rotation at start of certbot execution.") + None, "--max-log-backups", type=nonnegative_int, default=1000, + help="Specifies the maximum number of backup logs that should " + "be kept by Certbot's built in log rotation. Setting this " + "flag to 0 disables log rotation entirely, causing " + "Certbot to always append to the same log file.") helpful.add( [None, "automation", "run", "certonly"], "-n", "--non-interactive", "--noninteractive", dest="noninteractive_mode", action="store_true", @@ -1385,3 +1381,27 @@ class _RenewHookAction(argparse.Action): raise argparse.ArgumentError( self, "conflicts with --deploy-hook value") namespace.renew_hook = values + + +def nonnegative_int(value): + """Converts value to an int and checks that it is not negative. + + This function should used as the type parameter for argparse + arguments. + + :param str value: value provided on the command line + + :returns: integer representation of value + :rtype: int + + :raises argparse.ArgumentTypeError: if value isn't a non-negative integer + + """ + try: + int_value = int(value) + except ValueError: + raise argparse.ArgumentTypeError("value must be an integer") + + if int_value < 0: + raise argparse.ArgumentTypeError("value must be non-negative") + return int_value diff --git a/certbot/log.py b/certbot/log.py index 889b5c50a..73b2e354f 100644 --- a/certbot/log.py +++ b/certbot/log.py @@ -138,7 +138,8 @@ def setup_log_file_handler(config, logfile, fmt): log_file_path = os.path.join(config.logs_dir, logfile) try: handler = logging.handlers.RotatingFileHandler( - log_file_path, maxBytes=2 ** 20, backupCount=1000) + log_file_path, maxBytes=2 ** 20, + backupCount=config.max_log_backups) except IOError as error: raise errors.Error(util.PERM_ERR_FMT.format(error)) # rotate on each invocation, rollover only possible when maxBytes diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index cad04f88e..108e07eb3 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -364,6 +364,18 @@ class ParseTest(unittest.TestCase): # pylint: disable=too-many-public-methods self.assertEqual(namespace.deploy_hook, None) self.assertEqual(namespace.renew_hook, value) + def test_max_log_backups_error(self): + with mock.patch('certbot.cli.sys.stderr'): + self.assertRaises( + SystemExit, self.parse, "--max-log-backups foo".split()) + self.assertRaises( + SystemExit, self.parse, "--max-log-backups -42".split()) + + def test_max_log_backups_success(self): + value = "42" + namespace = self.parse(["--max-log-backups", value]) + self.assertEqual(namespace.max_log_backups, int(value)) + class DefaultTest(unittest.TestCase): """Tests for certbot.cli._Default.""" diff --git a/certbot/tests/log_test.py b/certbot/tests/log_test.py index 72ff076dd..5ee9ad812 100644 --- a/certbot/tests/log_test.py +++ b/certbot/tests/log_test.py @@ -66,8 +66,8 @@ class PostArgParseSetupTest(test_util.TempDirTestCase): def setUp(self): super(PostArgParseSetupTest, self).setUp() self.config = mock.MagicMock( - debug=False, logs_dir=self.tempdir, quiet=False, - verbose_count=constants.CLI_DEFAULTS['verbose_count']) + debug=False, logs_dir=self.tempdir, max_log_backups=1000, + quiet=False, verbose_count=constants.CLI_DEFAULTS['verbose_count']) self.devnull = open(os.devnull, 'w') from certbot.log import ColoredStreamHandler @@ -129,7 +129,7 @@ class SetupLogFileHandlerTest(test_util.TempDirTestCase): def setUp(self): super(SetupLogFileHandlerTest, self).setUp() - self.config = mock.MagicMock(logs_dir=self.tempdir) + self.config = mock.MagicMock(logs_dir=self.tempdir, max_log_backups=42) @mock.patch('certbot.main.logging.handlers.RotatingFileHandler') def test_failure(self, mock_handler): @@ -142,15 +142,32 @@ class SetupLogFileHandlerTest(test_util.TempDirTestCase): else: # pragma: no cover self.fail('Error not raised.') - def test_success(self): + def test_success_with_rollover(self): + self._test_success_common(should_rollover=True) + + def test_success_without_rollover(self): + self.config.max_log_backups = 0 + self._test_success_common(should_rollover=False) + + def _test_success_common(self, should_rollover): log_file = 'test.log' handler, log_path = self._call(self.config, log_file, '%(message)s') + handler.close() + self.assertEqual(handler.level, logging.DEBUG) self.assertEqual(handler.formatter.converter, time.gmtime) expected_path = os.path.join(self.config.logs_dir, log_file) self.assertEqual(log_path, expected_path) - handler.close() + + backup_path = os.path.join(self.config.logs_dir, log_file + '.1') + self.assertEqual(os.path.exists(backup_path), should_rollover) + + @mock.patch('certbot.log.logging.handlers.RotatingFileHandler') + def test_max_log_backups_used(self, mock_handler): + self._call(self.config, 'test.log', '%(message)s') + backup_count = mock_handler.call_args[1]['backupCount'] + self.assertEqual(self.config.max_log_backups, backup_count) class ColoredStreamHandlerTest(unittest.TestCase):