Repository: dwavesystems/dwave-cloud-client

Issue:
Progressive config files override
Make CWD config partially overrides user-local config, which partially overrides the system-wide config files.

For example, if `/usr/local/share/dwave/dwave.conf` has:
```
[defaults]
endpoint = <production>
client = qpu

[prod]
token = <token>
```

and `~/.config/dwave/dwave.conf` has:
```
[alpha]
endpoint = <alpha>
token = <token>
```

Then the user should be able to run `dwave ping --profile prod` **and** `dwave ping --profile alpha`.

Discussion:

Patch:
diff --git a/dwave/cloud/cli.py b/dwave/cloud/cli.py
index 5914660..3189154 100644
--- a/dwave/cloud/cli.py
+++ b/dwave/cloud/cli.py
@@ -8,8 +8,8 @@ from dwave.cloud.utils import readline_input
 from dwave.cloud.exceptions import (
     SolverAuthenticationError, InvalidAPIResponseError, UnsupportedSolverError)
 from dwave.cloud.config import (
-    load_config_from_file, get_default_config,
-    detect_configfile_path, get_default_configfile_path)
+    load_config_from_files, get_default_config,
+    get_configfile_path, get_default_configfile_path)
 
 
 @click.group()
@@ -30,7 +30,7 @@ def configure(config_file, profile):
         print("Using config file:", config_file)
     else:
         # path not given, try to detect; or use default, but allow user to override
-        config_file = detect_configfile_path()
+        config_file = get_configfile_path()
         if config_file:
             print("Found existing config file:", config_file)
         else:
@@ -40,8 +40,8 @@ def configure(config_file, profile):
 
     # try loading existing config, or use defaults
     try:
-        config = load_config_from_file(config_file)
-    except ValueError:
+        config = load_config_from_files([config_file])
+    except:
         config = get_default_config()
 
     # determine profile
diff --git a/dwave/cloud/config.py b/dwave/cloud/config.py
index 1593e5c..424f95a 100644
--- a/dwave/cloud/config.py
+++ b/dwave/cloud/config.py
@@ -9,32 +9,39 @@ CONF_AUTHOR = "dwavesystem"
 CONF_FILENAME = "dwave.conf"
 
 
-def detect_configfile_path():
-    """Returns the first existing file that it finds in a list of possible
-    candidates, and `None` if the list was exhausted, but no candidate config
-    file exists.
+def detect_existing_configfile_paths():
+    """Returns the list of existing config files found on disk.
 
-    For details, see :func:`load_config_from_file`.
+    Candidates examined depend on the OS, but for Linux possible list is:
+    ``dwave.conf`` in CWD, user-local ``.config/dwave/``, system-wide
+    ``/etc/dwave/``. For details, see :func:`load_config_from_file`.
     """
-    # look for `./dwave.conf`
-    candidates = ["."]
 
-    # then for something like `~/.config/dwave/dwave.conf`
+    # system-wide has the lowest priority, `/etc/dwave/dwave.conf`
+    candidates = homebase.site_config_dir_list(
+        app_author=CONF_AUTHOR, app_name=CONF_APP,
+        use_virtualenv=False, create=False)
+
+    # user-local will override it, `~/.config/dwave/dwave.conf`
     candidates.append(homebase.user_config_dir(
         app_author=CONF_AUTHOR, app_name=CONF_APP, roaming=False,
         use_virtualenv=False, create=False))
 
-    # and finally for e.g. `/etc/dwave/dwave.conf`
-    candidates.extend(homebase.site_config_dir_list(
-        app_author=CONF_AUTHOR, app_name=CONF_APP,
-        use_virtualenv=False, create=False))
+    # highest priority (overrides all): `./dwave.conf`
+    candidates.append(".")
+
+    paths = [os.path.join(base, CONF_FILENAME) for base in candidates]
+    existing_paths = [path for path in paths if os.path.exists(path)]
+
+    return existing_paths
 
-    for base in candidates:
-        path = os.path.join(base, CONF_FILENAME)
-        if os.path.exists(path):
-            return path
 
-    return None
+def get_configfile_path():
+    """Returns the highest-priority existing config file from a list
+    of possible candidates returned by `detect_existing_configfile_paths()`, and
+    ``None`` if no candidate config file exists."""
+    paths = detect_existing_configfile_paths()
+    return paths[-1] if paths else None
 
 
 def get_default_configfile_path():
@@ -47,12 +54,15 @@ def get_default_configfile_path():
     return path
 
 
-def load_config_from_file(filename=None):
-    """Load D-Wave cloud client configuration from ``filename``.
+def load_config_from_files(filenames=None):
+    """Load D-Wave cloud client configuration from a list of ``filenames``.
 
     The format of the config file is the standard Windows INI-like format,
     parsable with the Python's :mod:`configparser`.
 
+    Each filename in the list (each config file loaded) progressively upgrades
+    the final configuration (on a key by key basis, per each section).
+
     The section containing default values inherited by other sections is called
     ``defaults``. For example::
 
@@ -75,19 +85,19 @@ def load_config_from_file(filename=None):
         token = ...
 
     Args:
-        filename (str, default=None):
-            D-Wave cloud client configuration file location.
+        filenames (list[str], default=None):
+            D-Wave cloud client configuration file locations.
 
-            If unspecified, a config file named ``dwave.conf`` is searched for in
-            the current directory, then in the user-local config dir, and then
-            in all system-wide config dirs. For example, on Unix, we try to load
-            the config from these paths (in order) and possibly others
+            If set to ``None``, a config file named ``dwave.conf`` is searched for
+            in all system-wide config dirs, then in the user-local config dir,
+            and finally in the current directory. For example, on Unix, we try
+            to load the config from these paths (in order) and possibly others
             (depending on your Unix flavour)::
 
-                ./dwave.conf
-                ~/.config/dwave/dwave.conf
-                /usr/local/share/dwave/dwave.conf
                 /usr/share/dwave/dwave.conf
+                /usr/local/share/dwave/dwave.conf
+                ~/.config/dwave/dwave.conf
+                ./dwave.conf
 
             On Windows 7+, config file should be located in:
             ``C:\\Users\\<username>\\AppData\\Local\\dwavesystem\\dwave\\dwave.conf``,
@@ -102,31 +112,24 @@ def load_config_from_file(filename=None):
             mapping of per-profile keys holding values.
 
     Raises:
-        :exc:`ValueError`:
-            Config file location unspecified and undetected.
-
         :exc:`~dwave.cloud.exceptions.ConfigFileReadError`:
             Config file specified or detected could not be opened or read.
 
         :exc:`~dwave.cloud.exceptions.ConfigFileParseError`:
             Config file parse failed.
     """
-    if filename is None:
-        filename = detect_configfile_path()
-        if not filename:
-            raise ValueError("Config file not given, and could not be detected")
+    if filenames is None:
+        filenames = detect_existing_configfile_paths()
 
     config = configparser.ConfigParser(default_section="defaults")
-    try:
-        with open(filename, 'r') as f:
-            config.read_file(f, filename)
-
-    except (IOError, OSError):
-        raise ConfigFileReadError("Failed to read {!r}".format(filename))
-
-    except configparser.Error:
-        raise ConfigFileParseError("Failed to parse {!r}".format(filename))
-
+    for filename in filenames:
+        try:
+            with open(filename, 'r') as f:
+                config.read_file(f, filename)
+        except (IOError, OSError):
+            raise ConfigFileReadError("Failed to read {!r}".format(filename))
+        except configparser.Error:
+            raise ConfigFileParseError("Failed to parse {!r}".format(filename))
     return config
 
 
@@ -159,11 +162,6 @@ def get_default_config():
     return config
 
 
-def load_profile(name, filename=None):
-    """Load profile with ``name`` from config file ``filename``."""
-    return load_config_from_file(filename)[name]
-
-
 def load_config(config_file=None, profile=None, client=None,
                 endpoint=None, token=None, solver=None, proxy=None):
     """Load D-Wave cloud client configuration from ``config_file`` (either
@@ -292,23 +290,22 @@ def load_config(config_file=None, profile=None, client=None,
     """
     if config_file is None:
         config_file = os.getenv("DWAVE_CONFIG_FILE")
-    try:
-        config = load_config_from_file(config_file)
-        # determine profile name fallback:
-        #  (1) profile key under [defaults],
-        #  (2) first non-[defaults] section
-        first_section = next(iter(config.sections() + [None]))
-        config_defaults = config.defaults()
-        default_profile = config_defaults.get('profile', first_section)
-    except ValueError:
-        # config file not specified, or not detected: start with null-config
-        config = {}
-        config_defaults = {}
-        default_profile = None
-    except (ConfigFileReadError, ConfigFileParseError):
-        # unable to access/read/parse config file(s): explicitly fail
-        raise
 
+    # auto-detect if not specified with arg or env
+    filenames = [config_file] if config_file else None
+
+    # progressively build config from a file, or a list of auto-detected files
+    # raises ConfigFileReadError/ConfigFileParseError on error
+    config = load_config_from_files(filenames)
+
+    # determine profile name fallback:
+    #  (1) profile key under [defaults],
+    #  (2) first non-[defaults] section
+    first_section = next(iter(config.sections() + [None]))
+    config_defaults = config.defaults()
+    default_profile = config_defaults.get('profile', first_section)
+
+    # select profile from the config
     if profile is None:
         profile = os.getenv("DWAVE_PROFILE", default_profile)
     if profile:


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 671c29b..3f5b3cd 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -33,7 +33,7 @@ except ImportError:
 
 
 from dwave.cloud.config import (
-    detect_configfile_path, load_config_from_file, load_profile, load_config)
+    detect_existing_configfile_paths, load_config_from_files, load_config)
 
 
 class TestConfig(unittest.TestCase):
@@ -66,7 +66,7 @@ class TestConfig(unittest.TestCase):
 
     def test_config_load_from_file(self):
         with mock.patch('dwave.cloud.config.open', iterable_mock_open(self.config_body), create=True):
-            config = load_config_from_file(filename="filename")
+            config = load_config_from_files(filenames=["filename"])
             self.assertEqual(config.sections(), ['dw2000', 'software', 'alpha'])
             self.assertEqual(config['dw2000']['client'], 'qpu')
             self.assertEqual(config['software']['client'], 'sw')
@@ -88,26 +88,22 @@ class TestConfig(unittest.TestCase):
             key = val
         """
         with mock.patch('dwave.cloud.config.open', iterable_mock_open(myconfig), create=True):
-            self.assertRaises(ConfigFileParseError, load_config_from_file, filename="filename")
+            self.assertRaises(ConfigFileParseError, load_config_from_files, filenames=["filename"])
             self.assertRaises(ConfigFileParseError, load_config, config_file="filename", profile="section")
 
     def test_no_config_detected(self):
-        with mock.patch("dwave.cloud.config.detect_configfile_path", lambda: None):
-            self.assertRaises(ValueError, load_config_from_file)
+        """When no config file detected, `load_config_from_files` should return
+        empty config."""
+        with mock.patch("dwave.cloud.config.detect_existing_configfile_paths", lambda: []):
+            self.assertFalse(load_config_from_files().sections())
 
     def test_invalid_filename_given(self):
-        self.assertRaises(ConfigFileReadError, load_config_from_file, filename='/path/to/non/existing/config')
-
-    def test_config_load_profile(self):
-        with mock.patch('dwave.cloud.config.open', iterable_mock_open(self.config_body), create=True):
-            profile = load_profile(name="alpha", filename="filename")
-            self.assertEqual(profile['token'], 'alpha-token')
-            self.assertRaises(KeyError, load_profile, name="non-existing-section", filename="filename")
+        self.assertRaises(ConfigFileReadError, load_config_from_files, filenames=['/path/to/non/existing/config'])
 
     def test_config_file_detection_cwd(self):
         configpath = "./dwave.conf"
         with mock.patch("os.path.exists", lambda path: path == configpath):
-            self.assertEqual(detect_configfile_path(), configpath)
+            self.assertEqual(detect_existing_configfile_paths(), [configpath])
 
     def test_config_file_detection_user(self):
         if sys.platform == 'win32':
@@ -119,7 +115,7 @@ class TestConfig(unittest.TestCase):
             configpath = os.path.expanduser("~/.config/dwave/dwave.conf")
 
         with mock.patch("os.path.exists", lambda path: path == configpath):
-            self.assertEqual(detect_configfile_path(), configpath)
+            self.assertEqual(detect_existing_configfile_paths(), [configpath])
 
     def test_config_file_detection_system(self):
         if sys.platform == 'win32':
@@ -131,11 +127,11 @@ class TestConfig(unittest.TestCase):
             configpath = "/etc/xdg/dwave/dwave.conf"
 
         with mock.patch("os.path.exists", lambda path: path == configpath):
-            self.assertEqual(detect_configfile_path(), configpath)
+            self.assertEqual(detect_existing_configfile_paths(), [configpath])
 
     def test_config_file_detection_nonexisting(self):
         with mock.patch("os.path.exists", lambda path: False):
-            self.assertEqual(detect_configfile_path(), None)
+            self.assertEqual(detect_existing_configfile_paths(), [])
 
 
     def _assert_config_valid(self, config):
@@ -144,7 +140,7 @@ class TestConfig(unittest.TestCase):
         # default values are inherited
         self.assertEqual(config['client'], "qpu")
 
-    def _load_config_from_file(self, asked, provided, data=None):
+    def _load_config_from_files(self, asked, provided, data=None):
         self.assertEqual(asked, provided)
         if data is None:
             data = self.config_body
@@ -152,38 +148,38 @@ class TestConfig(unittest.TestCase):
 
 
     def test_config_load_configfile_arg(self):
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file, provided='myfile')):
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files, provided=['myfile'])):
             self._assert_config_valid(load_config(config_file='myfile', profile='alpha'))
 
     def test_config_load_configfile_env(self):
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file, provided='myfile')):
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files, provided=['myfile'])):
             with mock.patch.dict(os.environ, {'DWAVE_CONFIG_FILE': 'myfile'}):
                 self._assert_config_valid(load_config(config_file=None, profile='alpha'))
 
     def test_config_load_configfile_detect(self):
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file, provided=None)):
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files, provided=None)):
             self._assert_config_valid(load_config(config_file=None, profile='alpha'))
 
     def test_config_load_configfile_detect_profile_env(self):
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file, provided=None)):
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files, provided=None)):
             with mock.patch.dict(os.environ, {'DWAVE_PROFILE': 'alpha'}):
                 self._assert_config_valid(load_config())
 
     def test_config_load_configfile_env_profile_env(self):
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file, provided='myfile')):
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files, provided=['myfile'])):
             with mock.patch.dict(os.environ, {'DWAVE_CONFIG_FILE': 'myfile',
                                               'DWAVE_PROFILE': 'alpha'}):
                 self._assert_config_valid(load_config())
 
     def test_config_load_configfile_env_profile_env_key_arg(self):
         """Explicitly provided values should override env/file."""
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file, provided='myfile')):
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files, provided=['myfile'])):
             with mock.patch.dict(os.environ, {'DWAVE_CONFIG_FILE': 'myfile',
                                               'DWAVE_PROFILE': 'alpha'}):
                 self.assertEqual(load_config(endpoint='manual')['endpoint'], 'manual')
@@ -196,8 +192,8 @@ class TestConfig(unittest.TestCase):
         """load_config should fail if the profile specified in kwargs or env in
         non-existing.
         """
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file, provided=None)):
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files, provided=None)):
             self.assertRaises(ValueError, load_config, profile="nonexisting")
             with mock.patch.dict(os.environ, {'DWAVE_PROFILE': 'nonexisting'}):
                 self.assertRaises(ValueError, load_config)
@@ -206,8 +202,8 @@ class TestConfig(unittest.TestCase):
         """Check the right profile is loaded when `profile` specified only in
         [defaults] section.
         """
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file, provided='myfile')):
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files, provided=['myfile'])):
             profile = load_config(config_file='myfile')
             self.assertEqual(profile['solver'], 'c4-sw_sample')
 
@@ -219,8 +215,8 @@ class TestConfig(unittest.TestCase):
             [first]
             solver = DW_2000Q_1
         """
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file,
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files,
                                 provided=None, data=myconfig)):
             profile = load_config()
             self.assertIn('solver', profile)
@@ -235,8 +231,8 @@ class TestConfig(unittest.TestCase):
             [defaults]
             solver = DW_2000Q_1
         """
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file,
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files,
                                 provided=None, data=myconfig)):
             profile = load_config()
             self.assertIn('solver', profile)
@@ -253,7 +249,41 @@ class TestConfig(unittest.TestCase):
             [some]
             solver = DW_2000Q_1
         """
-        with mock.patch("dwave.cloud.config.load_config_from_file",
-                        partial(self._load_config_from_file,
-                                provided='myfile', data=myconfig)):
+        with mock.patch("dwave.cloud.config.load_config_from_files",
+                        partial(self._load_config_from_files,
+                                provided=['myfile'], data=myconfig)):
             self.assertRaises(ValueError, load_config, config_file='myfile')
+
+    def test_config_load_multiple_configfiles(self):
+        """Test more specific config overrides less specific one,
+        on a key by key basis."""
+
+        config_system = u"""
+            [alpha]
+            endpoint = alpha
+            solver = DW_2000Q_1
+        """
+        config_user = u"""
+            [alpha]
+            solver = DW_2000Q_2
+            [beta]
+            endpoint = beta
+        """
+
+        with mock.patch("dwave.cloud.config.detect_existing_configfile_paths",
+                        lambda: ['config_system', 'config_user']):
+
+            # test per-key override
+            with mock.patch('dwave.cloud.config.open', create=True) as m:
+                m.side_effect=[iterable_mock_open(config_system)(),
+                               iterable_mock_open(config_user)()]
+                section = load_config(profile='alpha')
+                self.assertEqual(section['endpoint'], 'alpha')
+                self.assertEqual(section['solver'], 'DW_2000Q_2')
+
+            # test per-section override (section addition)
+            with mock.patch('dwave.cloud.config.open', create=True) as m:
+                m.side_effect=[iterable_mock_open(config_system)(),
+                               iterable_mock_open(config_user)()]
+                section = load_config(profile='beta')
+                self.assertEqual(section['endpoint'], 'beta')
diff --git a/tests/test_mock_solver_loading.py b/tests/test_mock_solver_loading.py
index 2010c9e..2e8dc38 100644
--- a/tests/test_mock_solver_loading.py
+++ b/tests/test_mock_solver_loading.py
@@ -204,7 +204,7 @@ alpha|file-alpha-url,file-alpha-token,,alpha-solver
 
 
 # patch the new config loading mechanism, to test only legacy config loading
-@mock.patch("dwave.cloud.config.detect_configfile_path", lambda: None)
+@mock.patch("dwave.cloud.config.detect_existing_configfile_paths", lambda: [])
 class MockConfiguration(unittest.TestCase):
     """Ensure that the precedence of configuration sources is followed."""