Repository: iterative/dvc

Issue:
dvc remote default accepts anything
```
dvc remote add -d test test
dvc remote default 
# prints test
dvc remote default anything
dvc remote default 
# prints anything
```
Should indicate an error if the default remote is not in the list

**Please provide information about your setup**
DVC version: 0.87.0
Python version: 3.6.9
Platform: Linux-4.9.184-linuxkit-x86_64-with-Ubuntu-18.04-bionic
Binary: False
Package: pip
Cache: reflink - not supported, hardlink - supported, symlink - supported



Discussion:
I'm looking into this issue , but got some difficulty. 

Currently,  the DVC would take a validation on every change of the configuation. There are two types of validation, one is of data type ( core.no_scm must be boolen, timeout  and port of a ssh mus be int) another is of logic ( like #3470 core.remote must be in remote list).

The data type validation relies on only one configuration file, but the logic validation needs to consider all of the four configuration files. These four files are not always binding to each others, a validation may fail if we move the whole project to another environment.

And even worse, some modification involving configuartion of multi-levels might change configuration in a wrong way. For example, if we had four level of configuration like 

local 
repo {"core":{"remote": "my-remote"}, "remote":{"my-remote":{"url": "{{someurl}}"}}
global {"core":{"remote": "my-remote"}, "remote":{"my-remote":{"url": "{{anotherurl}}"}}
system

command dvc remote remove --global my-remote would delete the repo-level of default remote by wrong. It confused repositories in different level with a same name. (This is a issue?)
![image](https://user-images.githubusercontent.com/6745454/80359058-e0c6ca00-88af-11ea-884f-caad96b1f672.png)


So do we have to validate the logic of configuration when ever it changed, as we might be in a different environment when we actually run it, or we should only do validation at running time. 

It seems that Git makes no validation of its configuration.


> I'm looking into this issue , but got some difficulty.
> 
> Currently, the DVC would take a validation on every change of the configuation. There are two types of validation, one is of data type ( core.no_scm must be boolen, timeout and port of a ssh mus be int) another is of logic ( like #3470 core.remote must be in remote list).
> 
> The data type validation relies on only one configuration file, but the logic validation needs to consider all of the four configuration files. These four files are not always binding to each others, a validation may fail if we move the whole project to another environment.
> 
> And even worse, some modification involving configuartion of multi-levels might change configuration in a wrong way. For example, if we had four level of configuration like
> 
> local
> repo {"core":{"remote": "my-remote"}, "remote":{"my-remote":{"url": "{{someurl}}"}}
> global {"core":{"remote": "my-remote"}, "remote":{"my-remote":{"url": "{{anotherurl}}"}}
> system
> 
> command dvc remote remove --global my-remote would delete the repo-level of default remote by wrong. It confused repositories in different level with a same name. (This is a issue?)
> ![image](https://user-images.githubusercontent.com/6745454/80359058-e0c6ca00-88af-11ea-884f-caad96b1f672.png)
> 
> So do we have to validate the logic of configuration when ever it changed, as we might be in a different environment when we actually run it, or we should only do validation at running time.
> 
> It seems that Git makes no validation of its configuration.

A third choice is that we restrict our logic validation in one level of configuration.

As to removing the configuration of local remote:
From current implementation perspective I would say that it is a bug. 
Removing global `myremote` clears the default `myremote` even if it was set up 
to be local after the global one. Like in this script:

```
#!/bin/bash

rm -rf repo sglobal slocal
mkdir repo sglobal slocal

main=$(pwd)

set -ex

pushd repo
git init --quiet
dvc init -q

#remember to clear global config from myremote before running the script
dvc remote add -d --global myremote $main/sglobal
dvc remote add -d myremote $main/slocal

dvc remote remove myremote --global
# no remote
dvc remote default
```
Considering that there is avialable `myremote`, we should not remove it from config. 

Having said so I  think that we should throw error when defining remote with same name. Behavior should be consistent no matter the config level, and we should actually raise error when new remote is added and it conflicts with existing remote. Thats what happens when we initialize two remotes with the same name in local config.
eg:

```
dvc remote add -d myremote $main/sglobal
dvc remote add -d myremote $main/slocal
```
will result in: 
```
ERROR: configuration error - config file error: remote 'myremote' already
 exists. Use `-f|--force` to overwrite it.
```


So I think first issue that needs to be solved here is that we allow user to have multiple remotes of sam name. 

Patch:
diff --git a/dvc/command/remote.py b/dvc/command/remote.py
--- a/dvc/command/remote.py
+++ b/dvc/command/remote.py
@@ -85,7 +85,18 @@ def run(self):
                 if self.args.unset:
                     conf["core"].pop("remote", None)
                 else:
-                    conf["core"]["remote"] = self.args.name
+                    merged_conf = self.config.load_config_to_level(
+                        self.args.level
+                    )
+                    if (
+                        self.args.name in conf["remote"]
+                        or self.args.name in merged_conf["remote"]
+                    ):
+                        conf["core"]["remote"] = self.args.name
+                    else:
+                        raise ConfigError(
+                            "default remote must be present in remote list."
+                        )
         return 0
 
 
diff --git a/dvc/config.py b/dvc/config.py
--- a/dvc/config.py
+++ b/dvc/config.py
@@ -278,7 +278,7 @@ def load(self, validate=True):
         Raises:
             ConfigError: thrown if config has an invalid format.
         """
-        conf = self._load_config_to_level()
+        conf = self.load_config_to_level()
 
         if validate:
             conf = self.validate(conf)
@@ -330,7 +330,7 @@ def _map_dirs(conf, func):
         dirs_schema = {"cache": {"dir": func}, "remote": {str: {"url": func}}}
         return Schema(dirs_schema, extra=ALLOW_EXTRA)(conf)
 
-    def _load_config_to_level(self, level=None):
+    def load_config_to_level(self, level=None):
         merged_conf = {}
         for merge_level in self.LEVELS:
             if merge_level == level:
@@ -349,7 +349,7 @@ def edit(self, level="repo"):
 
         conf = self._save_paths(conf, self.files[level])
 
-        merged_conf = self._load_config_to_level(level)
+        merged_conf = self.load_config_to_level(level)
         _merge(merged_conf, conf)
         self.validate(merged_conf)
 


Test patch:
diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py
--- a/tests/func/test_remote.py
+++ b/tests/func/test_remote.py
@@ -121,6 +121,7 @@ def test(self):
 
 
 def test_show_default(dvc, capsys):
+    assert main(["remote", "add", "foo", "s3://bucket/name"]) == 0
     assert main(["remote", "default", "foo"]) == 0
     assert main(["remote", "default"]) == 0
     out, _ = capsys.readouterr()
@@ -270,3 +271,21 @@ def test_remote_modify_validation(dvc):
     )
     config = configobj.ConfigObj(dvc.config.files["repo"])
     assert unsupported_config not in config['remote "{}"'.format(remote_name)]
+
+
+def test_remote_modify_default(dvc):
+    remote_repo = "repo_level"
+    remote_local = "local_level"
+    wrong_name = "anything"
+    assert main(["remote", "add", remote_repo, "s3://bucket/repo"]) == 0
+    assert main(["remote", "add", remote_local, "s3://bucket/local"]) == 0
+
+    assert main(["remote", "default", wrong_name]) == 251
+    assert main(["remote", "default", remote_repo]) == 0
+    assert main(["remote", "default", "--local", remote_local]) == 0
+
+    repo_config = configobj.ConfigObj(dvc.config.files["repo"])
+    local_config = configobj.ConfigObj(dvc.config.files["local"])
+
+    assert repo_config["core"]["remote"] == remote_repo
+    assert local_config["core"]["remote"] == remote_local