dvc: refactor config (#3298)

* dvc: refactor config

Advantages:
- read like a dict
- modify as a dict for testing or manipulation purposes
- wrap modifications into context manager to save them:

    with config.edit("local") as conf:
        conf["core"]["remote"] = "new-default"

Design decisions:
- dropped string constants
- remote sections are parsed into a simple subdict upon load
- remote config schema is now defined by url scheme
- relative paths are handled transparently upon load/save
- user config and remote manipulations are moved to commands
- same dev manipulations are done by simply modifying a config dict

* test: do not set cache.type for remotes that doesn't support it

* dvc: fix windows absolute paths validation in config

* test: fix windows separate drive test

* dvc: stylistic fixes in config

dvc/analytics.py

@@ -54,11 +54,8 @@ def is_enabled():
         return False
 
     enabled = to_bool(
-        Config(validate=False)
-        .config.get(Config.SECTION_CORE, {})
-        .get(Config.SECTION_CORE_ANALYTICS, "true")
+        Config(validate=False).get("core", {}).get("analytics", "true")
     )
-
     logger.debug("Analytics is {}abled.".format("en" if enabled else "dis"))
 
     return enabled
@@ -144,7 +141,7 @@ def _find_or_create_user_id():
 
     IDs are generated randomly with UUID.
     """
-    config_dir = Config.get_global_config_dir()
+    config_dir = Config.get_dir("global")
     fname = os.path.join(config_dir, "user_id")
     lockfile = os.path.join(config_dir, "user_id.lock")
 

dvc/cache.py

@@ -1,25 +1,8 @@
 """Manages cache of a DVC repo."""
-import os
 from collections import defaultdict
 
 from funcy import cached_property
 
-from dvc.config import Config
-
-
-class CacheConfig(object):
-    def __init__(self, config):
-        self.config = config
-
-    def set_dir(self, dname, level=None):
-        from dvc.remote.config import RemoteConfig
-
-        configobj = self.config.get_configobj(level)
-        path = RemoteConfig.resolve_path(dname, configobj.filename)
-        self.config.set(
-            Config.SECTION_CACHE, Config.SECTION_CACHE_DIR, path, level=level
-        )
-
 
 def _make_remote_property(name):
     """
@@ -70,37 +53,22 @@ def __init__(self, repo):
         from dvc.remote import Remote
 
         self.repo = repo
+        self.config = config = repo.config["cache"]
 
-        self.config = config = repo.config.config[Config.SECTION_CACHE]
-        local = config.get(Config.SECTION_CACHE_LOCAL)
+        local = config.get("local")
 
         if local:
-            name = Config.SECTION_REMOTE_FMT.format(local)
-            settings = repo.config.config[name]
+            settings = {"name": local}
         else:
-            default_cache_dir = os.path.join(repo.dvc_dir, self.CACHE_DIR)
-            cache_dir = config.get(Config.SECTION_CACHE_DIR, default_cache_dir)
-            cache_type = config.get(Config.SECTION_CACHE_TYPE)
-            protected = config.get(Config.SECTION_CACHE_PROTECTED)
-            shared = config.get(Config.SECTION_CACHE_SHARED)
-
-            settings = {
-                Config.PRIVATE_CWD: config.get(
-                    Config.PRIVATE_CWD, repo.dvc_dir
-                ),
-                Config.SECTION_REMOTE_URL: cache_dir,
-                Config.SECTION_CACHE_TYPE: cache_type,
-                Config.SECTION_CACHE_PROTECTED: protected,
-                Config.SECTION_CACHE_SHARED: shared,
-            }
+            settings = {**config, "url": config["dir"]}
 
         self.local = Remote(repo, **settings)
 
-    s3 = _make_remote_property(Config.SECTION_CACHE_S3)
-    gs = _make_remote_property(Config.SECTION_CACHE_GS)
-    ssh = _make_remote_property(Config.SECTION_CACHE_SSH)
-    hdfs = _make_remote_property(Config.SECTION_CACHE_HDFS)
-    azure = _make_remote_property(Config.SECTION_CACHE_AZURE)
+    s3 = _make_remote_property("s3")
+    gs = _make_remote_property("gs")
+    ssh = _make_remote_property("ssh")
+    hdfs = _make_remote_property("hdfs")
+    azure = _make_remote_property("azure")
 
 
 class NamedCache(object):

dvc/command/add.py

@@ -1,11 +1,8 @@
 import argparse
 import logging
 
-from dvc.command.base import append_doc_link
-from dvc.command.base import CmdBase
-from dvc.exceptions import DvcException
-from dvc.exceptions import RecursiveAddingWhileUsingFilename
-
+from dvc.command.base import append_doc_link, CmdBase
+from dvc.exceptions import DvcException, RecursiveAddingWhileUsingFilename
 
 logger = logging.getLogger(__name__)
 

dvc/command/base.py

@@ -35,7 +35,7 @@ def __init__(self, args):
         self.repo = Repo()
         self.config = self.repo.config
         self.args = args
-        hardlink_lock = self.config.config["core"].get("hardlink_lock", False)
+        hardlink_lock = self.config["core"].get("hardlink_lock", False)
         updater = Updater(self.repo.dvc_dir, hardlink_lock=hardlink_lock)
         updater.check()
 

dvc/command/cache.py

@@ -1,18 +1,13 @@
 import argparse
 
-from dvc.cache import CacheConfig
-from dvc.command.base import append_doc_link
-from dvc.command.base import fix_subparsers
+from dvc.command.base import append_doc_link, fix_subparsers
 from dvc.command.config import CmdConfig
 
 
 class CmdCacheDir(CmdConfig):
-    def __init__(self, args):
-        super().__init__(args)
-        self.cache_config = CacheConfig(self.config)
-
     def run(self):
-        self.cache_config.set_dir(self.args.value, level=self.args.level)
+        with self.config.edit(level=self.args.level) as edit:
+            edit["cache"]["dir"] = self.args.value
         return 0
 
 

dvc/command/config.py

@@ -1,10 +1,8 @@
 import argparse
 import logging
 
-from dvc.command.base import append_doc_link
-from dvc.command.base import CmdBaseNoRepo
-from dvc.config import Config
-
+from dvc.command.base import append_doc_link, CmdBaseNoRepo
+from dvc.config import Config, ConfigError
 
 logger = logging.getLogger(__name__)
 
@@ -18,17 +16,21 @@ def __init__(self, args):
     def run(self):
         section, opt = self.args.name.lower().strip().split(".", 1)
 
-        if self.args.unset:
-            self.config.unset(section, opt, level=self.args.level)
-        elif self.args.value is None:
-            logger.info(self.config.get(section, opt, level=self.args.level))
-        else:
-            self.config.set(
-                section, opt, self.args.value, level=self.args.level
-            )
+        if self.args.value is None and not self.args.unset:
+            conf = self.config.load_one(self.args.level)
+            self._check(conf, section, opt)
+            logger.info(conf[section][opt])
+            return 0
+
+        with self.config.edit(self.args.level) as conf:
+            if self.args.unset:
+                self._check(conf, section, opt)
+                del conf[section][opt]
+            else:
+                self._check(conf, section)
+                conf[section][opt] = self.args.value
 
-        is_write = self.args.unset or self.args.value is not None
-        if is_write and self.args.name == "cache.type":
+        if self.args.name == "cache.type":
             logger.warning(
                 "You have changed the 'cache.type' option. This doesn't update"
                 " any existing workspace file links, but it can be done with:"
@@ -37,30 +39,39 @@ def run(self):
 
         return 0
 
+    def _check(self, conf, section, opt=None):
+        if section not in conf:
+            msg = "section {} doesn't exist"
+            raise ConfigError(msg.format(self.args.name))
+
+        if opt and opt not in conf[section]:
+            msg = "option {} doesn't exist"
+            raise ConfigError(msg.format(self.args.name))
+
 
 parent_config_parser = argparse.ArgumentParser(add_help=False)
 parent_config_parser.add_argument(
     "--global",
     dest="level",
     action="store_const",
-    const=Config.LEVEL_GLOBAL,
+    const="global",
     help="Use global config.",
 )
 parent_config_parser.add_argument(
     "--system",
     dest="level",
     action="store_const",
-    const=Config.LEVEL_SYSTEM,
+    const="system",
     help="Use system config.",
 )
 parent_config_parser.add_argument(
     "--local",
     dest="level",
     action="store_const",
-    const=Config.LEVEL_LOCAL,
+    const="local",
     help="Use local config.",
 )
-parent_config_parser.set_defaults(level=Config.LEVEL_REPO)
+parent_config_parser.set_defaults(level="repo")
 
 
 def add_parser(subparsers, parent_parser):

dvc/command/remote.py

@@ -1,72 +1,99 @@
 import argparse
 import logging
 
-from dvc.command.base import append_doc_link
-from dvc.command.base import fix_subparsers
+from dvc.config import ConfigError
+from dvc.command.base import append_doc_link, fix_subparsers
 from dvc.command.config import CmdConfig
-from dvc.remote.config import RemoteConfig
 from dvc.utils import format_link
 
 logger = logging.getLogger(__name__)
 
 
-class CmdRemoteConfig(CmdConfig):
+class CmdRemote(CmdConfig):
     def __init__(self, args):
         super().__init__(args)
-        self.remote_config = RemoteConfig(self.config)
 
+        if getattr(self.args, "name", None):
+            self.args.name = self.args.name.lower()
 
-class CmdRemoteAdd(CmdRemoteConfig):
+    def _check_exists(self, conf):
+        if self.args.name not in conf["remote"]:
+            raise ConfigError(
+                "remote '{}' doesn't exists.".format(self.args.name)
+            )
+
+
+class CmdRemoteAdd(CmdRemote):
     def run(self):
         if self.args.default:
             logger.info(
                 "Setting '{}' as a default remote.".format(self.args.name)
             )
-        self.remote_config.add(
-            self.args.name,
-            self.args.url,
-            force=self.args.force,
-            default=self.args.default,
-            level=self.args.level,
-        )
+
+        with self.config.edit(self.args.level) as conf:
+            if self.args.name in conf["remote"] and not self.args.force:
+                raise ConfigError(
+                    "remote '{}' already exists. Use `-f|--force` to "
+                    "overwrite it.".format(self.args.name)
+                )
+
+            conf["remote"][self.args.name] = {"url": self.args.url}
+            if self.args.default:
+                conf["core"]["remote"] = self.args.name
+
         return 0
 
 
-class CmdRemoteRemove(CmdRemoteConfig):
+class CmdRemoteRemove(CmdRemote):
     def run(self):
-        self.remote_config.remove(self.args.name, level=self.args.level)
+        with self.config.edit(self.args.level) as conf:
+            self._check_exists(conf)
+            del conf["remote"][self.args.name]
+
+        # Remove core.remote refs to this remote in any shadowing configs
+        for level in reversed(self.config.LEVELS):
+            with self.config.edit(level) as conf:
+                if conf["core"].get("remote") == self.args.name:
+                    del conf["core"]["remote"]
+
+            if level == self.args.level:
+                break
+
         return 0
 
 
-class CmdRemoteModify(CmdRemoteConfig):
+class CmdRemoteModify(CmdRemote):
     def run(self):
-        self.remote_config.modify(
-            self.args.name,
-            self.args.option,
-            self.args.value,
-            level=self.args.level,
-        )
+        with self.config.edit(self.args.level) as conf:
+            self._check_exists(conf)
+            conf["remote"][self.args.name][self.args.option] = self.args.value
         return 0
 
 
-class CmdRemoteDefault(CmdRemoteConfig):
+class CmdRemoteDefault(CmdRemote):
     def run(self):
+
         if self.args.name is None and not self.args.unset:
-            name = self.remote_config.get_default(level=self.args.level)
-            print(name)
+            conf = self.config.load_one(self.args.level)
+            try:
+                print(conf["core"]["remote"])
+            except KeyError:
+                logger.info("No default remote set")
+                return 1
         else:
-            self.remote_config.set_default(
-                self.args.name, unset=self.args.unset, level=self.args.level
-            )
+            with self.config.edit(self.args.level) as conf:
+                if self.args.unset:
+                    conf["core"].pop("remote", None)
+                else:
+                    conf["core"]["remote"] = self.args.name
         return 0
 
 
-class CmdRemoteList(CmdRemoteConfig):
+class CmdRemoteList(CmdRemote):
     def run(self):
-        for name, url in self.remote_config.list(
-            level=self.args.level
-        ).items():
-            logger.info("{}\t{}".format(name, url))
+        conf = self.config.load_one(self.args.level)
+        for name, conf in conf["remote"].items():
+            logger.info("{}\t{}".format(name, conf["url"]))
         return 0