Repository: iterative/dvc
Pull request: Add a new rename command 
fixed #3599
1. add new command `dvc remote rename <old> <new>`.
2. add tests for this new command.

* [x] ❗ I have followed the [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) checklist.

* [x] 📖 If this PR requires [documentation](https://dvc.org/doc) updates, I have created a separate PR (or issue, at least) in [dvc.org](https://github.com/iterative/dvc.org) and linked it here. If the CLI API is changed, I have updated [tab completion scripts](https://github.com/iterative/dvc/tree/master/scripts/completion).
  https://github.com/iterative/dvc.org/issues/1301

* [x] ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏


Problem statement:
remote modify: add `name` param?
So you can change the name of a remote e.g.

```console
$ dvc remote add myremote some/path
$ dvc remote list
myremote some/path
$ dvc remote modify myremote name supercoolremote
$ dvc remote list
supercoolremote some/path
```

The workaround rn is to just add the remote again and remove the previous one. But if it has some special configuration then it's more difficult. A better workaroud is to edit the .dvc/config file but relatively advanced.

`git remote` lets you change a remote name BTW.

Hints from issue discussion:
BTW if you actually run my example command to modify the inexistent `name` param, it gets written to .dvc/config, which causes an error output whenever you try to change the config later:

```
ERROR: configuration error - config file error:
extra keys not allowed @ data['remote']['myremote']['name']
```

You have to manually fix the config file to fix this.
> which causes an error output whenever you try to change the config later:

(Yep, I hit the same issue here https://github.com/iterative/dvc/issues/3552 - quite annoying one :) )

> A better workaroud is to edit the .dvc/config file but relatively advanced.

not sure this is very advanced to be honest

Not advanced in terms of skill, but you need to know that remote config is in that file. It won't be obvious knowledge for DVC beginners. Also for gdrive remotes the file in .dvc/tmp/...json would also need to be changed manually, I think (if it already exists).
>  Also for gdrive remotes the file in .dvc/tmp/...json would also need to be changed manually, I think (if it already exists).

it doesn't contain the remote name yet. With GDrive multiple remotes are not supported effectively yet.

>  It won't be obvious knowledge for DVC beginners.

it's a good question. I think for people familiar with command line, git, etc it should be more or less intuitive that there is a config somewhere. 
OK. No strong opinion. I would err on the side of adding this which I'm assuming should be pretty easy, since `git remote` allows you to do it, so you may expect this if you're used to Git. + the other reasons outlines. Feel fee to close this though, up to you! Thanks
I'd like to look into it. @jorgeorpinel How about make it `dvc remote rename <old> <new>` following the similar command in Git `git remote rename <old> <new>`.
Good idea, indeed that's how it works on Git. But since we already have a `modify` subcommand to change all possible remote config I'd prefer to add a `name` config param for that.
It does not hurt to add both? Is there some modify command analog in GIt?
Not really. They have a subcommand for each remote config param you can change: https://git-scm.com/docs/git-remote
`name` is ugly. `dvc remote rename` is much better.

Commit message: fixed #3599

1. add new command `dvc remote rename <old> <new>`.
2. add tests for this new command.

Patch to review:
diff --git a/dvc/command/remote.py b/dvc/command/remote.py
index c5f46a1a08..4afc66776a 100644
--- a/dvc/command/remote.py
+++ b/dvc/command/remote.py
@@ -108,6 +108,34 @@ def run(self):
         return 0
 
 
+class CmdRemoteRename(CmdRemote):
+    def run(self):
+        conf = self.config.load_one(self.args.level)
+        self._check_exists(conf)
+
+        all_config = self.config.load_config_to_level("all")
+        if self.args.new in all_config.get("remote", {}):
+            raise ConfigError(
+                "Rename failed.Remote name {} already exists.".format(
+                    {self.args.new}
+                )
+            )
+
+        with self.config.edit(self.args.level) as conf:
+            conf["remote"][self.args.new] = conf["remote"][self.args.name]
+            del conf["remote"][self.args.name]
+
+        for level in reversed(self.config.LEVELS):
+            with self.config.edit(level) as conf:
+                if conf["core"].get("remote") == self.args.name:
+                    conf["core"]["remote"] = self.args.new
+
+            if level == self.args.level:
+                break
+
+        return 0
+
+
 def add_parser(subparsers, parent_parser):
     from dvc.command.config import parent_config_parser
 
@@ -224,3 +252,14 @@ def add_parser(subparsers, parent_parser):
         "name", help="Name of the remote to remove."
     )
     remote_remove_parser.set_defaults(func=CmdRemoteRemove)
+    REMOTE_RENAME_HELP = "Rename a data remote"
+    remote_rename_parser = remote_subparsers.add_parser(
+        "rename",
+        parents=[parent_config_parser, parent_parser],
+        description=append_doc_link(REMOTE_RENAME_HELP, "remote/rename"),
+        help=REMOTE_RENAME_HELP,
+        formatter_class=argparse.RawDescriptionHelpFormatter,
+    )
+    remote_rename_parser.add_argument("name", help="Remote to be renamed")
+    remote_rename_parser.add_argument("new", help="New name of the remote")
+    remote_rename_parser.set_defaults(func=CmdRemoteRename)
diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py
index 8e349efb98..f447cb6942 100644
--- a/tests/func/test_remote.py
+++ b/tests/func/test_remote.py
@@ -285,3 +285,74 @@ def test_remote_modify_default(dvc):
 
     assert repo_config["core"]["remote"] == remote_repo
     assert local_config["core"]["remote"] == remote_local
+
+
+def test_remote_rename(dvc):
+    remote_name = "drive"
+    remote_url = "gdrive://test/test"
+    new_name = "new"
+    other_name = "other"
+    # prepare
+    assert main(["remote", "add", remote_name, remote_url]) == 0
+    config = dvc.config.load_one("repo")
+    assert config["remote"][remote_name]["url"] == remote_url
+    assert new_name not in config.get("remote", {})
+
+    # rename failed
+    assert main(["remote", "rename", remote_name]) == 254
+    assert main(["remote", "rename", new_name, other_name]) == 251
+    config = dvc.config.load_one("repo")
+    assert config["remote"][remote_name]["url"] == remote_url
+    assert new_name not in config.get("remote", {})
+
+    # rename success
+    assert main(["remote", "rename", remote_name, new_name]) == 0
+    config = dvc.config.load_one("repo")
+    assert remote_name not in config.get("remote", {})
+    assert config["remote"][new_name]["url"] == remote_url
+
+
+def test_remote_duplicated(dvc):
+    remote_name = "drive"
+    remote_url = "gdrive://test/test"
+    used_name = "overlap"
+    another_url = "gdrive://test/test1"
+    # prepare
+    assert main(["remote", "add", remote_name, remote_url]) == 0
+    assert main(["remote", "add", "--local", used_name, another_url]) == 0
+    config = dvc.config.load_one("repo")
+    assert config["remote"][remote_name]["url"] == remote_url
+    local_config = dvc.config.load_one("local")
+    assert local_config["remote"][used_name]["url"] == another_url
+
+    # rename duplicated
+    assert main(["remote", "rename", remote_name, used_name]) == 251
+    config = dvc.config.load_one("repo")
+    assert config["remote"][remote_name]["url"] == remote_url
+    local_config = dvc.config.load_one("local")
+    assert local_config["remote"][used_name]["url"] == another_url
+
+
+def test_remote_default(dvc):
+    remote_name = "drive"
+    remote_url = "gdrive://test/test"
+    new_name = "new"
+    # prepare
+    assert main(["remote", "add", "-d", remote_name, remote_url]) == 0
+    assert main(["remote", "default", "--local", remote_name]) == 0
+    config = dvc.config.load_one("repo")
+    assert config["core"]["remote"] == remote_name
+    assert config["remote"][remote_name]["url"] == remote_url
+    assert new_name not in config.get("remote", {})
+    local_config = dvc.config.load_one("local")
+    assert local_config["core"]["remote"] == remote_name
+
+    # rename success
+    assert main(["remote", "rename", remote_name, new_name]) == 0
+    config = dvc.config.load_one("repo")
+    assert remote_name not in config.get("remote", {})
+    assert config["core"]["remote"] == new_name
+    assert config["remote"][new_name]["url"] == remote_url
+    assert remote_name not in config.get("remote", {})
+    local_config = dvc.config.load_one("local")
+    assert local_config["core"]["remote"] == new_name


Code review comments:
diff_hunk: @@ -108,6 +108,34 @@ def run(self):
         return 0
 
 
+class CmdRemoteRename(CmdRemote):
+    def run(self):
+        conf = self.config.load_one(self.args.level)
+        self._check_exists(conf)
+
+        all_config = self.config.load_config_to_level("all")
+        if self.args.new in all_config.get("remote", {}):
+            raise ConfigError(
+                "Rename failed.Remote name {} already exists.".format(
path: dvc/command/remote.py
text: @user1:
```suggestion
                "Rename failed. Remote name '{}' already exists.".format(
```

This could also be an f-string now instead of using `format()`

diff_hunk: @@ -108,6 +108,34 @@ def run(self):
         return 0
 
 
+class CmdRemoteRename(CmdRemote):
+    def run(self):
+        conf = self.config.load_one(self.args.level)
+        self._check_exists(conf)
path: dvc/command/remote.py
text: @user1:
Nitpicky/minor issue, but we end up loading this conf again when you do
```py
with self.config.edit(self.args.level) as conf:
```

So if we move the `_check_exists` call and the code for checking if new name exists in all/other config levels into the `with` statement, we can get rid of the extra `load_one` call. Something like:

```py
with self.config.edit(self.args.level) as conf:
    self._check_exists(conf)

    # <check if new name exists in all/other levels here>

    conf["remote"][self.args.new] = conf["remote"][self.args.name]
    del conf["remote"][self.args.name]

diff_hunk: @@ -108,6 +108,34 @@ def run(self):
         return 0
 
 
+class CmdRemoteRename(CmdRemote):
+    def run(self):
+        conf = self.config.load_one(self.args.level)
+        self._check_exists(conf)
+
+        all_config = self.config.load_config_to_level("all")
path: dvc/command/remote.py
text: @user1:
Should this be `load_config_to_level()` since `"all"` isn't a defined level? (unless using `"all"` is supposed to actually do something different than calling `load_config_to_level(level=None)`)

@author:
`"all"` is more readable than `None`, this is the reason I used it. Both of them give the same result. As you agreed with `None`, then it is now a better choice for consistency with team code style.