ref: refactoring and unit testing config (#348)

## 📝 Description

**What does this PR do and why is this change necessary?**

Refactor all code relevant to generating the linode-cli config file
Add unit tests to the config generation

Author: jriddle-linode

linodecli/configuration/__init__.py

@@ -3,18 +3,18 @@
 used elsewhere.
 """
 
-import argparse
 import os
 import sys
-import webbrowser
+import argparse
 
 from .auth import (
+    _get_token_web,
     _check_full_access,
     _do_get_request,
     _get_token_terminal,
-    _get_token_web,
 )
 from .helpers import (
+    _check_browsers,
     _default_thing_input,
     _get_config,
     _get_config_path,
@@ -23,22 +23,6 @@
 
 ENV_TOKEN_NAME = "LINODE_CLI_TOKEN"
 
-# this is a list of browser that _should_ work for web-based auth.  This is mostly
-# intended to exclude lynx and other terminal browsers which could be opened, but
-# won't work.
-KNOWN_GOOD_BROWSERS = {
-    "chrome",
-    "firefox",
-    "mozilla",
-    "netscape",
-    "opera",
-    "safari",
-    "chromium",
-    "chromium-browser",
-    "epiphany",
-}
-
-
 class CLIConfig:
     """
     Generates the necessary config for the Linode CLI
@@ -73,17 +57,6 @@ def __init__(self, base_url, username=None, skip_config=False):
         elif environ_token is not None:
             self.used_env_token = True
 
-    def set_user(self, username):
-        """
-        Sets the acting username.  If this username is not in the config, this is
-        an error.  This overrides the default username
-        """
-        if not self.config.has_section(username):
-            print(f"User {username} is not configured!")
-            sys.exit(1)
-
-        self.username = username
-
     def default_username(self):
         """
         Returns the default-user Username
@@ -92,83 +65,16 @@ def default_username(self):
             return self.config.get("DEFAULT", "default-user")
         return ""
 
-    def update_namespace(self, namespace, new_dict):
-        """
-        In order to update the namespace, we need to turn it into a dict, modify it there,
-        then reconstruct it with the exploded dict.
-        """
-        ns_dict = vars(namespace)
-        warn_dict = {}
-        for k in new_dict:
-            if k.startswith("plugin-"):
-                # plugins set config options that start with 'plugin-' - these don't
-                # get included in the updated namespace
-                continue
-            if k in ns_dict and isinstance(k, list):
-                ns_dict[k].append(new_dict[k])
-            if k in ns_dict and ns_dict[k] is None:
-                warn_dict[k] = new_dict[k]
-                ns_dict[k] = new_dict[k]
-        if not any(
-            x in ["--suppress-warnings", "--no-headers"] for x in sys.argv
-        ):
-            print(
-                f"using default values: {warn_dict}, use --no-defaults flag to disable defaults"
-            )
-        return argparse.Namespace(**ns_dict)
-
-    def update(self, namespace, allowed_defaults):
+    def set_user(self, username):
         """
-        This updates a Namespace (as returned by ArgumentParser) with config values
-        if they aren't present in the Namespace already.
+        Sets the acting username.  If this username is not in the config, this is
+        an error.  This overrides the default username
         """
-        if self.used_env_token and self.config is None:
-            # the CLI is using a token defined in the environment; as such, we may
-            # not have actually loaded a config file.  That's fine, there are just
-            # no defaults
-            return None
-
-        username = self.username or self.default_username()
-
-        if not self.config.has_option(username, "token") and not os.environ.get(
-            ENV_TOKEN_NAME, None
-        ):
-            print(f"User {username} is not configured.")
+        if not self.config.has_section(username):
+            print(f"User {username} is not configured!")
             sys.exit(1)
 
-        if self.config.has_section(username) and allowed_defaults:
-            # update_dicts = {
-            #     default_key: self.config.get(username, default_key)
-            #     for default_key in allowed_defaults
-            #     if self.config.has_option(username, default_key)
-            #     }
-            update_dicts = {}
-            for default_key in allowed_defaults:
-                if not self.config.has_option(username, default_key):
-                    continue
-                value = self.config.get(username, default_key)
-                if default_key == "authorized_users":
-                    update_dicts[default_key] = [value]
-                else:
-                    update_dicts[default_key] = value
-
-            return self.update_namespace(namespace, update_dicts)
-        return namespace
-
-    def get_token(self):
-        """
-        Returns the token for a configured user
-        """
-        if self.used_env_token:
-            return os.environ.get(ENV_TOKEN_NAME, None)
-
-        if self.config.has_option(
-            self.username or self.default_username(), "token"
-        ):
-            return self.config.get(
-                self.username or self.default_username(), "token"
-            )
-        return ""
+        self.username = username
 
     def remove_user(self, username):
         """
@@ -177,7 +83,7 @@ def remove_user(self, username):
         """
         if self.default_username() == username:
             print(
-                f"Cannot remote {username} as they are the default user! You can "
+                f"Cannot remove {username} as they are the default user! You can "
                 "change the default user with: `linode-cli set-user USERNAME`"
             )
             sys.exit(1)
@@ -210,8 +116,17 @@ def set_default_user(self, username):
         self.config.set("DEFAULT", "default-user", username)
         self.write_config()
 
-    # plugin methods - these are intended for plugins to utilize to store their
-    # own persistent config information
+    def get_token(self):
+        """
+        Returns the token for a configured user
+        """
+        if self.used_env_token:
+            return os.environ.get(ENV_TOKEN_NAME, None)
+
+        if self.config.has_option(self.username or self.default_username(), "token"):
+            return self.config.get(self.username or self.default_username(), "token")
+        return ""
+
     def get_value(self, key):
         """
         Retrieves and returns an existing config value for the current user.  This
@@ -236,6 +151,8 @@ def get_value(self, key):
 
         return self.config.get(username, key)
 
+    # plugin methods - these are intended for plugins to utilize to store their
+    # own persistent config information
     def plugin_set_value(self, key, value):
         """
         Sets a new config value for a plugin for the current user.  Plugin config
@@ -286,31 +203,65 @@ def plugin_get_value(self, key):
 
         return self.config.get(username, full_key)
 
-    def write_config(self, silent=False):
+    # TODO: this is more of an argparsing function than it is a config function
+    # might be better to move this to argparsing during refactor and just have
+    # configuration return defaults or keys or something
+    def update(self, namespace, allowed_defaults):
+        """
+        This updates a Namespace (as returned by ArgumentParser) with config values
+        if they aren't present in the Namespace already.
+        """
+        if self.used_env_token and self.config is None:
+            return None
+        username = self.username or self.default_username()
+        if (not self.config.has_option(username, "token")
+            and not os.environ.get(ENV_TOKEN_NAME, None)):
+            print(f"User {username} is not configured.")
+            sys.exit(1)
+        if (not self.config.has_section(username)
+            or allowed_defaults is None):
+            return namespace
+
+        warn_dict = {}
+        ns_dict = vars(namespace)
+        for key in allowed_defaults:
+            if key not in ns_dict:
+                continue
+            if ns_dict[key] is not None:
+                continue
+            # plugins set config options that start with 'plugin-'
+            # these don't get included in the updated namespace
+            if key.startswith("plugin-"):
+                continue
+            if self.config.has_option(username, key):
+                value = self.config.get(username, key)
+            else:
+                value = allowed_defaults[key]
+            if key == "authorized_users":
+                ns_dict[key] = [value]
+                warn_dict[key] = [value]
+            else:
+                ns_dict[key] = value
+                warn_dict[key] = value
+
+        if not any(x in ["--suppress-warnings", "--no-headers"] for x in sys.argv):
+            print(f"using default values: {warn_dict}, "
+                "use --no-defaults flag to disable defaults")
+        return argparse.Namespace(**ns_dict)
+
+    def write_config(self):
         """
         Saves the config file as it is right now.  This can be used by plugins
         to save values they've set, and is used internally to update the config
         on disk when a new user if configured.
-
-        :param silent: If True, does not print a message noting the config file
-                       has been updated.  This is primarily intended for silently
-                       updated the config file from one version to another.
-        :type silent: bool
         """
-
-        # Create the ~/.config directory if it does not exist
         if not os.path.exists(f"{os.path.expanduser('~')}/.config"):
             os.makedirs(f"{os.path.expanduser('~')}/.config")
-
         with open(_get_config_path(), "w", encoding="utf-8") as f:
             self.config.write(f)
 
-        if not silent:
-            print(f"\nConfig written to {_get_config_path()}")
 
-    def configure(
-        self,
-    ):  # pylint: disable=too-many-branches,too-many-statements
+    def configure(self):  # pylint: disable=too-many-branches,too-many-statements
         """
         This assumes we're running interactively, and prompts the user
         for a series of defaults in order to make future CLI calls
@@ -326,67 +277,36 @@ def configure(
         username = None
         token = None
 
-        print(
-            """Welcome to the Linode CLI.  This will walk you through some initial setup."""
-        )
+        print("Welcome to the Linode CLI.  This will walk you through some initial setup.")
 
         if ENV_TOKEN_NAME in os.environ:
             print(
-                """Using token from {env_token_name}.
+                f"""Using token from {ENV_TOKEN_NAME}.
 Note that no token will be saved in your configuration file.
-    * If you lose or remove {env_token_name}.
-    * All profiles will use {env_token_name}.""".format(
-                    env_token_name=ENV_TOKEN_NAME
-                )
+    * If you lose or remove {ENV_TOKEN_NAME}.
+    * All profiles will use {ENV_TOKEN_NAME}."""
             )
             username = "DEFAULT"
             token = os.getenv(ENV_TOKEN_NAME)
 
         else:
-            # let's see if we _can_ use web
-            can_use_browser = True
-            try:
-                webbrowser.get()
-            except webbrowser.Error:
-                # there are no browsers installed
-                can_use_browser = False
-
-            if can_use_browser and not KNOWN_GOOD_BROWSERS.intersection(
-                webbrowser._tryorder  # pylint: disable=protected-access
-            ):
-                print()
-                print(
-                    "This tool defaults to web-based authentication, however "
-                    "no known-working browsers were found."
-                )
-
-                while True:
-                    r = input("Try it anyway? [y/N]: ")
-                    if r.lower() in "yn ":
-                        can_use_browser = r.lower() == "y"
-                        break
-
-            if self.configure_with_pat or not can_use_browser:
-                username, config["token"] = _get_token_terminal(self.base_url)
-            else:
-                # pylint: disable=line-too-long
-                print()
-                print(
-                    "The CLI will use its web-based authentication to log you in.  "
-                    "If you prefer to supply a Personal Access Token, use `linode-cli configure --token`. "
-                )
-                print()
+            if _check_browsers() and not self.configure_with_pat:
+                print("""
+The CLI will use its web-based authentication to log you in.
+If you prefer to supply a Personal Access Token, use `linode-cli configure --token`.
+                    """)
                 input(
-                    "Press enter to continue.  This will open a browser and proceed with authentication."
+                    "Press enter to continue. "
+                    "This will open a browser and proceed with authentication."
                 )
                 username, config["token"] = _get_token_web(self.base_url)
-                # pylint: enable=line-too-long
-
+            else:
+                username, config["token"] = _get_token_terminal(self.base_url)
             token = config["token"]
 
-        print()
-        print(f"Configuring {username}")
-        print()
+        print(f"\nConfiguring {username}\n")
+
+        # Configuring Defaults
 
         regions = [
             r["id"] for r in _do_get_request(self.base_url, "/regions")["data"]
@@ -452,20 +372,14 @@ def configure(
         if not is_default:
             if username != self.default_username():
                 while True:
-                    value = input(
-                        "Make this user the default when using the CLI? [y/N]: "
-                    )
-
+                    value = input("Make this user the default when using the CLI? [y/N]: ")
                     if value.lower() in "yn":
                         is_default = value.lower() == "y"
                         break
                     if not value.strip():
                         break
-
             if not is_default:  # they didn't change the default user
-                print(
-                    f"Active user will remain {self.config.get('DEFAULT', 'default-user')}"
-                )
+                print(f"Active user will remain {self.config.get('DEFAULT', 'default-user')}")
 
         if is_default:
             # if this is the default user, make it so