Review comment fixes

certbot-apache/certbot_apache/configurator.py

@@ -303,13 +303,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         for vhost in vhosts:
             self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path)
 
-    def choose_vhosts(self, domain, cached=False, create_if_no_ssl=True):
+    def choose_vhosts(self, domain, create_if_no_ssl=True):
         """
         Finds VirtualHosts that can be used with the provided domain
 
         :param str domain: Domain name to match VirtualHosts to
-        :param bool cached: Return VHosts from a list cached previously on this
-            execution.
         :param bool create_if_no_ssl: If found VirtualHost doesn't have a HTTPS
             counterpart, should one get created
 
@@ -318,7 +316,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         """
 
         if self._wildcard_domain(domain):
-            if cached and domain in self._wildcard_vhosts:
+            if domain in self._wildcard_vhosts:
                 # Vhosts for a wildcard domain were already selected
                 return self._wildcard_vhosts[domain]
             # Ask user which VHosts to support.
@@ -368,7 +366,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 if vhost.ssl:
                     # Always prefer SSL vhosts
                     filtered_vhosts[name] = vhost
-                elif name not in filtered_vhosts:
+                elif name not in filtered_vhosts and create_ssl:
                     # Add if not in list previously
                     filtered_vhosts[name] = vhost
 
@@ -382,7 +380,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # if requested.
         return_vhosts = list()
         for vhost in dialog_output:
-            if not vhost.ssl and create_ssl:
+            if not vhost.ssl:
                 return_vhosts.append(self.make_vhost_ssl(vhost))
             else:
                 return_vhosts.append(vhost)
@@ -1493,7 +1491,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             raise errors.PluginError(
                 "Unsupported enhancement: {0}".format(enhancement))
 
-        vhosts = self.choose_vhosts(domain, cached=True, create_if_no_ssl=False)
+        vhosts = self.choose_vhosts(domain, create_if_no_ssl=False)
         try:
             for vhost in vhosts:
                 func(vhost, options)

certbot-apache/certbot_apache/display_ops.py

@@ -24,15 +24,17 @@ def select_vhost_multiple(vhosts):
     """
     if not vhosts:
         return list()
-    tags_list = [vhost.display_repr() for vhost in vhosts]
-    while True:
-        code, names = zope.component.getUtility(interfaces.IDisplay).checklist(
-            "Which VirtualHosts would you like to install the wildcard certificate for?",
-            tags=tags_list, force_interactive=True)
-        if code == display_util.OK:
-            return_vhosts = _reversemap_vhosts(names, vhosts)
-            return return_vhosts
-        return []
+    tags_list = [vhost.display_repr()+"\n" for vhost in vhosts]
+    # Remove the extra newline from the last entry
+    if len(tags_list):
+        tags_list[-1] = tags_list[-1][:-1]
+    code, names = zope.component.getUtility(interfaces.IDisplay).checklist(
+        "Which VirtualHosts would you like to install the wildcard certificate for?",
+        tags=tags_list, force_interactive=True)
+    if code == display_util.OK:
+        return_vhosts = _reversemap_vhosts(names, vhosts)
+        return return_vhosts
+    return []
 
 def _reversemap_vhosts(names, vhosts):
     """Helper function for select_vhost_multiple for mapping string
@@ -41,7 +43,7 @@ def _reversemap_vhosts(names, vhosts):
 
     for selection in names:
         for vhost in vhosts:
-            if vhost.display_repr() == selection:
+            if vhost.display_repr().strip() == selection.strip():
                 return_vhosts.append(vhost)
     return return_vhosts
 
@@ -57,12 +59,11 @@ def select_vhost(domain, vhosts):
     """
     if not vhosts:
         return None
-    while True:
-        code, tag = _vhost_menu(domain, vhosts)
-        if code == display_util.OK:
-            return vhosts[tag]
-        else:
-            return None
+    code, tag = _vhost_menu(domain, vhosts)
+    if code == display_util.OK:
+        return vhosts[tag]
+    else:
+        return None
 
 def _vhost_menu(domain, vhosts):
     """Select an appropriate Apache Vhost.

certbot-apache/certbot_apache/obj.py

@@ -173,7 +173,7 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
             "File: {filename}\n"
             "Addresses: {addrs}\n"
             "Names: {names}\n"
-            "HTTPS: {https}\n\n".format(
+            "HTTPS: {https}\n".format(
                 filename=self.filep,
                 addrs=", ".join(str(addr) for addr in self.addrs),
                 names=", ".join(self.get_names()),

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -1367,11 +1367,11 @@ class MultipleVhostsTest(util.ApacheTest):
         # pylint: disable=protected-access
         mock_path = "certbot_apache.display_ops.select_vhost_multiple"
         with mock.patch(mock_path) as mock_select_vhs:
-            mock_select_vhs.return_value = [self.vh_truth[3]]
+            mock_select_vhs.return_value = [self.vh_truth[1]]
             vhs = self.config._choose_vhosts_wildcard("*.certbot.demo",
                                                      create_ssl=False)
             self.assertFalse(mock_makessl.called)
-            self.assertEquals(vhs[0], self.vh_truth[3])
+            self.assertEquals(vhs[0], self.vh_truth[1])
 
     @mock.patch("certbot_apache.configurator.ApacheConfigurator._vhosts_for_wildcard")
     @mock.patch("certbot_apache.configurator.ApacheConfigurator.make_vhost_ssl")

certbot/tests/display/ops_test.py

@@ -300,7 +300,7 @@ class ChooseNamesTest(unittest.TestCase):
         from certbot.display.ops import get_valid_domains
         all_valid = ["example.com", "second.example.com",
                      "also.example.com", "under_score.example.com",
-                     "justtld"]
+                     "justtld", "*.wildcard.com"]
         all_invalid = ["öóòps.net", "uniçodé.com"]
         two_valid = ["example.com", "úniçøde.com", "also.example.com"]
         self.assertEqual(get_valid_domains(all_valid), all_valid)