PR cleanup

letsencrypt/cli.py

@@ -1,9 +1,7 @@
 """Let's Encrypt CLI."""
 # TODO: Sanity check all input.  Be sure to avoid shell code etc...
-
 import argparse
 import atexit
-import configobj
 import functools
 import logging
 import logging.handlers
@@ -14,6 +12,7 @@ import time
 import traceback
 
 import configargparse
+import configobj
 import OpenSSL
 import zope.component
 import zope.interface.exceptions
@@ -173,22 +172,22 @@ def _find_duplicative_certs(domains, config, renew_config):
     identical_names_cert, subset_names_cert = None, None
 
     configs_dir = renew_config.renewal_configs_dir
+    cli_config = configuration.RenewerConfiguration(config)
     for renewal_file in os.listdir(configs_dir):
         full_path = os.path.join(configs_dir, renewal_file)
         rc_config = configobj.ConfigObj(renew_config.renewer_config_file)
         rc_config.merge(configobj.ConfigObj(full_path))
         rc_config.filename = full_path
-        cli_config = configuration.RenewerConfiguration(config)
 
-        candidate_lineage = storage.RenewableCert(rc_config, None, cli_config)
+        candidate_lineage = storage.RenewableCert(rc_config, config_opts=None,
+                                                  cli_config=cli_config)
         # TODO: Handle these differently depending on whether they are
         #       expired or still valid?
         candidate_names = set(candidate_lineage.names())
         if candidate_names == set(domains):
-            identical_names_cert = (renewal_file, candidate_lineage)
+            identical_names_cert = candidate_lineage
         elif candidate_names.issubset(set(domains)):
-            subset_names_cert = (renewal_file, candidate_lineage,
-                                 candidate_lineage.names())
+            subset_names_cert = candidate_lineage
 
     return identical_names_cert, subset_names_cert
 
@@ -229,20 +228,21 @@ def run(args, config, plugins):  # pylint: disable=too-many-branches,too-many-lo
         # I am not sure whether that correctly reads the systemwide
         # configuration file.
         question = None
-        if identical_names_cert:
+        if identical_names_cert is not None:
             question = (
                 "You have an existing certificate that contains exactly the "
                 "same domains you requested (ref: {0})\n\nDo you want to "
                 "renew and replace this certificate with a newly-issued one?"
-                ).format(identical_names_cert[0])
-        elif subset_names_cert:
+                ).format(identical_names_cert.configfile.filename)
+        elif subset_names_cert is not None:
             question = (
                 "You have an existing certificate that contains a portion of "
                 "the domains you requested (ref: {0})\n\nIt contains these "
                 "names: {1}\n\nYou requested these names for the new "
                 "certificate: {2}.\n\nDo you want to replace this existing "
                 "certificate with the new certificate?"
-                ).format(subset_names_cert[0], ", ".join(subset_names_cert[2]),
+                ).format(subset_names_cert.configfile.filename,
+                         ", ".join(subset_names_cert.names()),
                          ", ".join(domains))
         if question is None:
             # We aren't in a duplicative-names situation at all, so we don't
@@ -257,19 +257,16 @@ def run(args, config, plugins):  # pylint: disable=too-many-branches,too-many-lo
                 "To obtain a new certificate that {0} an existing certificate "
                 "in its domain-name coverage, you must use the --duplicate "
                 "option.\n\nFor example:\n\n{1} --duplicate {2}").format(
-                    "duplicates" if identical_names_cert else "overlaps with",
-                    sys.argv[0], " ".join(sys.argv[1:])),
-                                      reporter_util.HIGH_PRIORITY, True)
-            sys.exit(1)
+                    "duplicates" if identical_names_cert is not None else
+                    "overlaps with", sys.argv[0], " ".join(sys.argv[1:])),
+                                      reporter_util.HIGH_PRIORITY)
+            return 1
 
     # Attempting to obtain the certificate
     # TODO: Handle errors from _init_le_client?
     le_client = _init_le_client(args, config, authenticator, installer)
     if treat_as_renewal:
-        if identical_names_cert:
-            lineage = identical_names_cert[1]
-        else:
-            lineage = subset_names_cert[1]
+        lineage = identical_names_cert if identical_names_cert is not None else subset_names_cert
         # TODO: Use existing privkey instead of generating a new one
         new_certr, new_chain, new_key, _ = le_client.obtain_certificate(domains)
         # TODO: Check whether it worked!

letsencrypt/storage.py

@@ -437,8 +437,7 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         else:
             target = self.version("cert", version)
         with open(target) as f:
-            sans = crypto_util.get_sans_from_cert(f.read())
-        return sans
+            return crypto_util.get_sans_from_cert(f.read())
 
     def should_autodeploy(self):
         """Should this lineage now automatically deploy a newer version?

letsencrypt/tests/cli_test.py

@@ -1,5 +1,4 @@
 """Tests for letsencrypt.cli."""
-import configobj
 import itertools
 import os
 import shutil
@@ -7,6 +6,7 @@ import traceback
 import tempfile
 import unittest
 
+import configobj
 import mock
 
 from letsencrypt import account
@@ -14,7 +14,6 @@ from letsencrypt import configuration
 from letsencrypt import errors
 from letsencrypt import storage
 
-from letsencrypt.storage import ALL_FOUR
 from letsencrypt.tests import test_util
 
 
@@ -178,7 +177,7 @@ class DuplicativeCertsTest(unittest.TestCase):
         os.makedirs(os.path.join(self.tempdir, "archive", "example.org"))
         os.makedirs(os.path.join(self.tempdir, "configs"))
         config = configobj.ConfigObj()
-        for kind in ALL_FOUR:
+        for kind in storage.ALL_FOUR:
             config[kind] = os.path.join(self.tempdir, "live", "example.org",
                                         kind + ".pem")
         config.filename = os.path.join(self.tempdir, "configs",
@@ -188,7 +187,7 @@ class DuplicativeCertsTest(unittest.TestCase):
         self.defaults = configobj.ConfigObj()
         self.test_rc = storage.RenewableCert(
             self.config, self.defaults, self.cli_config)
-        for kind in ALL_FOUR:
+        for kind in storage.ALL_FOUR:
             where = getattr(self.test_rc, kind)
             os.symlink(os.path.join("..", "..", "archive", "example.org",
                                     "{0}12.pem".format(kind)), where)
@@ -219,15 +218,15 @@ class DuplicativeCertsTest(unittest.TestCase):
         # Totally identical
         result = _find_duplicative_certs(["example.com", "www.example.com"],
                                          self.config, self.cli_config)
-        self.assertEqual(result[0][0], "example.org.conf")
+        self.assertTrue(result[0].configfile.filename.endswith("example.org.conf"))
         self.assertEqual(result[1], None)
 
         # Superset
         result = _find_duplicative_certs(["example.com", "www.example.com",
                                           "something.new"], self.config,
                                          self.cli_config)
-        self.assertEqual(result[1][0], "example.org.conf")
         self.assertEqual(result[0], None)
+        self.assertTrue(result[1].configfile.filename.endswith("example.org.conf"))
 
         # Partial overlap doesn't count
         result = _find_duplicative_certs(["example.com", "something.new"],