From 06052a8f6a28936ef94f76275af09e4c2eafc56b Mon Sep 17 00:00:00 2001 From: Dmitrii Pukhov Date: Sat, 30 Mar 2019 14:48:07 +0900 Subject: [PATCH 1/3] cli: fix overwriting remotes in `dvc remote add` Now dvc remote add requires user confirmation in case of overwriting of an already existing remote. Fixes #1760 --- dvc/command/remote.py | 23 +++++++++++++++++++++-- dvc/exceptions.py | 4 ++-- tests/test_remote.py | 9 +++++++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index fb03c536f..a2c4cfb32 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -5,12 +5,17 @@ import os import re import dvc.logger as logger -from dvc.command.base import fix_subparsers, append_doc_link -from dvc.config import Config +from dvc.command.base import append_doc_link, fix_subparsers from dvc.command.config import CmdConfig +from dvc.config import Config +from dvc.exceptions import ConfirmRemoveError, DvcException class CmdRemoteAdd(CmdConfig): + def _prevent_config_overwriting(self, section): + if (section in self.configobj.keys()) and not self.args.force: + raise ConfirmRemoveError(section) + @staticmethod def resolve_path(path, config_file): """Resolve path relative to config file location. @@ -39,6 +44,13 @@ class CmdRemoteAdd(CmdConfig): ) section = Config.SECTION_REMOTE_FMT.format(self.args.name) + + try: + self._prevent_config_overwriting(section) + except DvcException as exc: + logger.error(exc) + return 1 + ret = self._set(section, Config.SECTION_REMOTE_URL, self.args.url) if ret != 0: return ret @@ -156,6 +168,13 @@ def add_parser(subparsers, parent_parser): default=False, help="Set as default remote.", ) + remote_add_parser.add_argument( + "-f", + "--force", + action="store_true", + default=False, + help="Force overwriting existing configs", + ) remote_add_parser.set_defaults(func=CmdRemoteAdd) REMOTE_DEFAULT_HELP = "Set/unset default remote." diff --git a/dvc/exceptions.py b/dvc/exceptions.py index 92bdb9169..25a7f4a65 100644 --- a/dvc/exceptions.py +++ b/dvc/exceptions.py @@ -179,8 +179,8 @@ class CyclicGraphError(DvcException): class ConfirmRemoveError(DvcException): def __init__(self, path): super(ConfirmRemoveError, self).__init__( - "unable to remove '{}' without a confirmation from the user. Use " - "'-f' to force.".format(path) + "unable to remove or overwrite '{}' without a confirmation " + "from the user. Use '-f' to force.".format(path) ) diff --git a/tests/test_remote.py b/tests/test_remote.py index c2e611732..67a5e8cdc 100644 --- a/tests/test_remote.py +++ b/tests/test_remote.py @@ -57,6 +57,15 @@ class TestRemote(TestDvc): config = configobj.ConfigObj(self.dvc.config.config_file) self.assertEqual(config['remote "mylocal"']["url"], rel) + def test_overwrite(self): + remote_name = "a" + remote_url = "s3://bucket/name" + main(["remote", "add", remote_name, remote_url]) + self.assertEqual(main(["remote", "add", remote_name, remote_url]), 1) + self.assertEqual( + main(["remote", "add", "-f", remote_name, remote_url]), 0 + ) + class TestRemoteRemoveDefault(TestDvc): def test(self): From 180b844f4605fd765e06391606f1c8058ccfd6a9 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sat, 30 Mar 2019 21:21:37 +0900 Subject: [PATCH 2/3] Update tests/test_remote.py test_overwrite: fix case of none-zero result on initial stage Co-Authored-By: puhoshville --- tests/test_remote.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_remote.py b/tests/test_remote.py index 67a5e8cdc..b298c092e 100644 --- a/tests/test_remote.py +++ b/tests/test_remote.py @@ -60,7 +60,7 @@ class TestRemote(TestDvc): def test_overwrite(self): remote_name = "a" remote_url = "s3://bucket/name" - main(["remote", "add", remote_name, remote_url]) + self.assertEqual(main(["remote", "add", remote_name, remote_url]), 0) self.assertEqual(main(["remote", "add", remote_name, remote_url]), 1) self.assertEqual( main(["remote", "add", "-f", remote_name, remote_url]), 0 From 67f4c9329e7419df56b088b31b5daf41617dd310 Mon Sep 17 00:00:00 2001 From: Dmitrii Pukhov Date: Sat, 30 Mar 2019 21:38:47 +0900 Subject: [PATCH 3/3] Fix unnecessary raise, ConfirmRemoveError recover --- dvc/command/remote.py | 16 ++++++---------- dvc/exceptions.py | 4 ++-- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index a2c4cfb32..9bb487639 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -8,14 +8,9 @@ import dvc.logger as logger from dvc.command.base import append_doc_link, fix_subparsers from dvc.command.config import CmdConfig from dvc.config import Config -from dvc.exceptions import ConfirmRemoveError, DvcException class CmdRemoteAdd(CmdConfig): - def _prevent_config_overwriting(self, section): - if (section in self.configobj.keys()) and not self.args.force: - raise ConfirmRemoveError(section) - @staticmethod def resolve_path(path, config_file): """Resolve path relative to config file location. @@ -44,11 +39,12 @@ class CmdRemoteAdd(CmdConfig): ) section = Config.SECTION_REMOTE_FMT.format(self.args.name) - - try: - self._prevent_config_overwriting(section) - except DvcException as exc: - logger.error(exc) + if (section in self.configobj.keys()) and not self.args.force: + logger.error( + "Remote with name {} already exists. " + "Use -f (--force) to overwrite remote " + "with new value".format(self.args.name) + ) return 1 ret = self._set(section, Config.SECTION_REMOTE_URL, self.args.url) diff --git a/dvc/exceptions.py b/dvc/exceptions.py index 25a7f4a65..92bdb9169 100644 --- a/dvc/exceptions.py +++ b/dvc/exceptions.py @@ -179,8 +179,8 @@ class CyclicGraphError(DvcException): class ConfirmRemoveError(DvcException): def __init__(self, path): super(ConfirmRemoveError, self).__init__( - "unable to remove or overwrite '{}' without a confirmation " - "from the user. Use '-f' to force.".format(path) + "unable to remove '{}' without a confirmation from the user. Use " + "'-f' to force.".format(path) )