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

@@ -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

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

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."""

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):