Various clean-ups in certbot-apache. Use f-strings. (#9132)

* Various clean-ups in certbot-apache. Use f-strings.

* Smaller tweaks

certbot-apache/certbot_apache/_internal/apache_util.py

@@ -172,8 +172,7 @@ def parse_includes(apachectl):
     :rtype: list of str
     """
 
-    inc_cmd = [apachectl, "-t", "-D",
+    inc_cmd = [apachectl, "-t", "-D", "DUMP_INCLUDES"]
-               "DUMP_INCLUDES"]
     return parse_from_subprocess(inc_cmd, r"\(.*\) (.*)")
 
 
@@ -188,8 +187,7 @@ def parse_modules(apachectl):
     :rtype: list of str
     """
 
-    mod_cmd = [apachectl, "-t", "-D",
+    mod_cmd = [apachectl, "-t", "-D", "DUMP_MODULES"]
-               "DUMP_MODULES"]
     return parse_from_subprocess(mod_cmd, r"(.*)_module")
 
 

certbot-apache/certbot_apache/_internal/apacheparser.py

@@ -14,7 +14,8 @@ class ApacheParserNode(interfaces.ParserNode):
     """
 
     def __init__(self, **kwargs):
-        ancestor, dirty, filepath, metadata = util.parsernode_kwargs(kwargs)  # pylint: disable=unused-variable
+        ancestor, dirty, filepath, metadata = util.parsernode_kwargs(
+            kwargs)  # pylint: disable=unused-variable
         super().__init__(**kwargs)
         self.ancestor = ancestor
         self.filepath = filepath

certbot-apache/certbot_apache/_internal/augeasparser.py

@@ -388,7 +388,7 @@ class AugeasBlockNode(AugeasDirectiveNode):
         """
         Deletes a ParserNode from the sequence of children, and raises an
         exception if it's unable to do so.
-        :param AugeasParserNode: child: A node to delete.
+        :param AugeasParserNode child: A node to delete.
         """
         if not self.parser.aug.remove(child.metadata["augeaspath"]):
 
@@ -531,7 +531,7 @@ class AugeasBlockNode(AugeasDirectiveNode):
                 position
             )
 
-        return (insert_path, resulting_path, before)
+        return insert_path, resulting_path, before
 
 
 interfaces.CommentNode.register(AugeasCommentNode)

certbot-apache/certbot_apache/_internal/configurator.py

@@ -154,7 +154,7 @@ class ApacheConfigurator(common.Configurator):
         # So for old versions of Apache we pick a configuration without this option.
         min_openssl_version = util.parse_loose_version('1.0.2l')
         openssl_version = self.openssl_version(warn_on_no_mod_ssl)
-        if self.version < (2, 4, 11) or not openssl_version or\
+        if self.version < (2, 4, 11) or not openssl_version or \
             util.parse_loose_version(openssl_version) < min_openssl_version:
             return apache_util.find_ssl_apache_conf("old")
         return apache_util.find_ssl_apache_conf("current")
@@ -470,10 +470,11 @@ class ApacheConfigurator(common.Configurator):
         """Initializes the ParserNode parser root instance."""
 
         if HAS_APACHECONFIG:
-            apache_vars = {}
+            apache_vars = {
-            apache_vars["defines"] = apache_util.parse_defines(self.options.ctl)
+                "defines": apache_util.parse_defines(self.options.ctl),
-            apache_vars["includes"] = apache_util.parse_includes(self.options.ctl)
+                "includes": apache_util.parse_includes(self.options.ctl),
-            apache_vars["modules"] = apache_util.parse_modules(self.options.ctl)
+                "modules": apache_util.parse_modules(self.options.ctl),
+            }
             metadata["apache_vars"] = apache_vars
 
             with open(self.parser.loc["root"]) as f:
@@ -928,7 +929,7 @@ class ApacheConfigurator(common.Configurator):
             # Get last ServerName as each overwrites the previous
             servername = self.parser.get_arg(servername_match[-1])
 
-        return (servername, serveraliases)
+        return servername, serveraliases
 
     def _add_servernames(self, host):
         """Helper function for get_virtual_hosts().
@@ -976,7 +977,7 @@ class ApacheConfigurator(common.Configurator):
                 is_ssl = True
 
         filename = apache_util.get_file_path(
-            self.parser.aug.get("/augeas/files%s/path" % apache_util.get_file_path(path)))
+            self.parser.aug.get(f"/augeas/files{apache_util.get_file_path(path)}/path"))
         if filename is None:
             return None
 
@@ -1128,10 +1129,8 @@ class ApacheConfigurator(common.Configurator):
         :type host: :class:`~certbot_apache.obj.VirtualHost`
         """
 
-        servername_match = vhost.node.find_directives("ServerName",
+        servername_match = vhost.node.find_directives("ServerName", exclude=False)
-                                                      exclude=False)
+        serveralias_match = vhost.node.find_directives("ServerAlias", exclude=False)
-        serveralias_match = vhost.node.find_directives("ServerAlias",
-                                                       exclude=False)
 
         servername = None
         if servername_match:
@@ -1143,7 +1142,6 @@ class ApacheConfigurator(common.Configurator):
                     vhost.aliases.add(serveralias)
             vhost.name = servername
 
-
     def is_name_vhost(self, target_addr):
         """Returns if vhost is a name based vhost
 
@@ -1210,7 +1208,7 @@ class ApacheConfigurator(common.Configurator):
 
         # If HTTPS requested for nonstandard port, add service definition
         if https and port != "443":
-            port_service = "%s %s" % (port, "https")
+            port_service = f"{port} https"
         else:
             port_service = port
 
@@ -1262,15 +1260,15 @@ class ApacheConfigurator(common.Configurator):
             # We have wildcard, skip the rest
             self.parser.add_dir(parser.get_aug_path(self.parser.loc["listen"]),
                                 "Listen", port)
-            self.save_notes += "Added Listen %s directive to %s\n" % (
+            self.save_notes += (
-                port, self.parser.loc["listen"])
+                f"Added Listen {port} directive to {self.parser.loc['listen']}\n"
+            )
         else:
             for listen in new_listens:
                 self.parser.add_dir(parser.get_aug_path(
                     self.parser.loc["listen"]), "Listen", listen.split(" "))
-                self.save_notes += ("Added Listen %s directive to "
+                self.save_notes += (f"Added Listen {listen} directive to "
-                                    "%s\n") % (listen,
+                                    f"{self.parser.loc['listen']}\n")
-                                               self.parser.loc["listen"])
 
     def _add_listens_https(self, listens, listens_orig, port):
         """Helper method for ensure_listen to figure out which new
@@ -1283,7 +1281,7 @@ class ApacheConfigurator(common.Configurator):
 
         # Add service definition for non-standard ports
         if port != "443":
-            port_service = "%s %s" % (port, "https")
+            port_service = f"{port} https"
         else:
             port_service = port
 
@@ -1294,16 +1292,16 @@ class ApacheConfigurator(common.Configurator):
             self.parser.add_dir_to_ifmodssl(
                 parser.get_aug_path(self.parser.loc["listen"]),
                 "Listen", port_service.split(" "))
-            self.save_notes += "Added Listen %s directive to %s\n" % (
+            self.save_notes += (
-                port_service, self.parser.loc["listen"])
+                f"Added Listen {port_service} directive to {self.parser.loc['listen']}\n"
+            )
         else:
             for listen in new_listens:
                 self.parser.add_dir_to_ifmodssl(
                     parser.get_aug_path(self.parser.loc["listen"]),
                     "Listen", listen.split(" "))
-                self.save_notes += ("Added Listen %s directive to "
+                self.save_notes += (f"Added Listen {listen} directive to "
-                                    "%s\n") % (listen,
+                                    f"{self.parser.loc['listen']}\n")
-                                               self.parser.loc["listen"])
 
     def _has_port_already(self, listens, port):
         """Helper method for prepare_server_https to find out if user
@@ -1743,8 +1741,7 @@ class ApacheConfigurator(common.Configurator):
             for test_vh in self.vhosts:
                 if (vhost.filep != test_vh.filep and
                     any(test_addr in addrs for
-                            test_addr in test_vh.addrs) and
+                        test_addr in test_vh.addrs) and not self.is_name_vhost(addr)):
-                        not self.is_name_vhost(addr)):
                     self.add_name_vhost(addr)
                     logger.info("Enabling NameVirtualHosts on %s", addr)
                     need_to_save = True
@@ -1940,10 +1937,7 @@ class ApacheConfigurator(common.Configurator):
         Searches AutoHSTS managed VirtualHosts that belong to the lineage.
         Matches the private key path.
         """
-
+        return bool(self.parser.find_dir("SSLCertificateKeyFile", lineage.key_path, vhost.path))
-        return bool(
-            self.parser.find_dir("SSLCertificateKeyFile",
-                                 lineage.key_path, vhost.path))
 
     def _enable_ocsp_stapling(self, ssl_vhost, unused_options):
         """Enables OCSP Stapling
@@ -2073,7 +2067,7 @@ class ApacheConfigurator(common.Configurator):
             for match in header_path:
                 if re.search(pat, self.parser.aug.get(match).lower()):
                     raise errors.PluginEnhancementAlreadyPresent(
-                        "Existing %s header" % (header_substring))
+                        "Existing %s header" % header_substring)
 
     def _enable_redirect(self, ssl_vhost, unused_options):
         """Redirect all equivalent HTTP traffic to ssl_vhost.
@@ -2285,22 +2279,19 @@ class ApacheConfigurator(common.Configurator):
         else:
             rewrite_rule_args = constants.REWRITE_HTTPS_ARGS
 
-        return ("<VirtualHost %s>\n"
+        return (
-                "%s \n"
+            f"<VirtualHost {' '.join(str(addr) for addr in self._get_proposed_addrs(ssl_vhost))}>\n"
-                "%s \n"
+            f"{servername} \n"
-                "ServerSignature Off\n"
+            f"{serveralias} \n"
+            f"ServerSignature Off\n"
+            f"\n"
+            f"RewriteEngine On\n"
+            f"RewriteRule {' '.join(rewrite_rule_args)}\n"
             "\n"
-                "RewriteEngine On\n"
+            f"ErrorLog {self.options.logs_root}/redirect.error.log\n"
-                "RewriteRule %s\n"
+            f"LogLevel warn\n"
-                "\n"
+            f"</VirtualHost>\n"
-                "ErrorLog %s/redirect.error.log\n"
+        )
-                "LogLevel warn\n"
-                "</VirtualHost>\n"
-                % (" ".join(str(addr) for
-                            addr in self._get_proposed_addrs(ssl_vhost)),
-                   servername, serveralias,
-                   " ".join(rewrite_rule_args),
-                   self.options.logs_root))
 
     def _write_out_redirect(self, ssl_vhost, text):
         # This is the default name
@@ -2409,11 +2400,13 @@ class ApacheConfigurator(common.Configurator):
             generic fashion.
 
         """
-        mod_message = ("Apache needs to have module  \"{0}\" active for the " +
+        mod_message = (
-            "requested installation options. Unfortunately Certbot is unable " +
+            f"Apache needs to have module  \"{mod_name}\" active for the "
-            "to install or enable it for you. Please install the module, and " +
+            "requested installation options. Unfortunately Certbot is unable "
-            "run Certbot again.")
+            "to install or enable it for you. Please install the module, and "
-        raise errors.MisconfigurationError(mod_message.format(mod_name))
+            "run Certbot again."
+        )
+        raise errors.MisconfigurationError(mod_message)
 
     def restart(self):
         """Runs a config test and reloads the Apache server.

certbot-apache/certbot_apache/_internal/display_ops.py

@@ -108,17 +108,17 @@ def _vhost_menu(domain, vhosts):
 
     try:
         code, tag = display_util.menu(
-            "We were unable to find a vhost with a ServerName "
+            f"We were unable to find a vhost with a ServerName "
-            "or Address of {0}.{1}Which virtual host would you "
+            f"or Address of {domain}.{os.linesep}Which virtual host would you "
-            "like to choose?".format(domain, os.linesep),
+            f"like to choose?",
             choices, force_interactive=True)
     except errors.MissingCommandlineFlag:
         msg = (
-            "Encountered vhost ambiguity when trying to find a vhost for "
+            f"Encountered vhost ambiguity when trying to find a vhost for "
-            "{0} but was unable to ask for user "
+            f"{domain} but was unable to ask for user "
-            "guidance in non-interactive mode. Certbot may need "
+            f"guidance in non-interactive mode. Certbot may need "
-            "vhosts to be explicitly labelled with ServerName or "
+            f"vhosts to be explicitly labelled with ServerName or "
-            "ServerAlias directives.".format(domain))
+            f"ServerAlias directives.")
         logger.error(msg)
         raise errors.MissingCommandlineFlag(msg)
 

certbot-apache/certbot_apache/_internal/interfaces.py

@@ -102,7 +102,7 @@ For this reason the internal representation of data should not ignore the case.
 import abc
 
 
-class ParserNode(object, metaclass=abc.ABCMeta):
+class ParserNode(metaclass=abc.ABCMeta):
     """
     ParserNode is the basic building block of the tree of such nodes,
     representing the structure of the configuration. It is largely meant to keep

certbot-apache/certbot_apache/_internal/obj.py

@@ -21,7 +21,7 @@ class Addr(common.Addr):
         return False
 
     def __repr__(self):
-        return "certbot_apache._internal.obj.Addr(" + repr(self.tup) + ")"
+        return f"certbot_apache._internal.obj.Addr({repr(self.tup)})"
 
     def __hash__(self):  # pylint: disable=useless-super-delegation
         # Python 3 requires explicit overridden for __hash__ if __eq__ or
@@ -147,34 +147,24 @@ class VirtualHost:
 
     def __str__(self):
         return (
-            "File: {filename}\n"
+            f"File: {self.filep}\n"
-            "Vhost path: {vhpath}\n"
+            f"Vhost path: {self.path}\n"
-            "Addresses: {addrs}\n"
+            f"Addresses: {', '.join(str(addr) for addr in self.addrs)}\n"
-            "Name: {name}\n"
+            f"Name: {self.name if self.name is not None else ''}\n"
-            "Aliases: {aliases}\n"
+            f"Aliases: {', '.join(name for name in self.aliases)}\n"
-            "TLS Enabled: {tls}\n"
+            f"TLS Enabled: {'Yes' if self.ssl else 'No'}\n"
-            "Site Enabled: {active}\n"
+            f"Site Enabled: {'Yes' if self.enabled else 'No'}\n"
-            "mod_macro Vhost: {modmacro}".format(
+            f"mod_macro Vhost: {'Yes' if self.modmacro else 'No'}"
-                filename=self.filep,
+        )
-                vhpath=self.path,
-                addrs=", ".join(str(addr) for addr in self.addrs),
-                name=self.name if self.name is not None else "",
-                aliases=", ".join(name for name in self.aliases),
-                tls="Yes" if self.ssl else "No",
-                active="Yes" if self.enabled else "No",
-                modmacro="Yes" if self.modmacro else "No"))
 
     def display_repr(self):
         """Return a representation of VHost to be used in dialog"""
         return (
-            "File: {filename}\n"
+            f"File: {self.filep}\n"
-            "Addresses: {addrs}\n"
+            f"Addresses: {', '.join(str(addr) for addr in self.addrs)}\n"
-            "Names: {names}\n"
+            f"Names: {', '.join(self.get_names())}\n"
-            "HTTPS: {https}\n".format(
+            f"HTTPS: {'Yes' if self.ssl else 'No'}\n"
-                filename=self.filep,
+        )
-                addrs=", ".join(str(addr) for addr in self.addrs),
-                names=", ".join(self.get_names()),
-                https="Yes" if self.ssl else "No"))
 
     def __eq__(self, other):
         if isinstance(other, self.__class__):