From 55e1201445f35c7679394c68df8e85fb4e1095bc Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Wed, 29 Apr 2020 22:14:25 +0800 Subject: [PATCH 1/8] fix #3470 Add validation to restrict default remote repo in list of remote repos. --- dvc/command/remote.py | 7 +++---- dvc/config.py | 8 ++++++++ tests/func/test_remote.py | 10 ++++++++++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index 0580537b87..c655fa488f 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -48,16 +48,15 @@ class CmdRemoteRemove(CmdRemote): def run(self): 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 + if level == self.args.level: + del conf["remote"][self.args.name] + break return 0 diff --git a/dvc/config.py b/dvc/config.py index 1fc63acbb3..d06efcad28 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -359,6 +359,14 @@ def edit(self, level="repo"): @staticmethod def validate(data): try: + if ( + "remote" in data["core"] + and data["core"]["remote"] not in data["remote"] + ): + print(data) + print(data["core"]["remote"]) + print(data["remote"]) + raise ConfigError("") return COMPILED_SCHEMA(data) except Invalid as exc: raise ConfigError(str(exc)) from None diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 4369203dd7..3cd6039170 100644 --- 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,12 @@ 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_default_modify(dvc): + remote_name = "my_remote" + wrong_remote_name = "anything" + assert main(["remote", "add", remote_name, "s3://bucket/name"]) == 0 + + assert main(["remote", "default", remote_name]) == 0 + assert main(["remote", "default", wrong_remote_name]) == 251 From 194ed2dfe4243234000235e76b32d3890f907958 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Fri, 1 May 2020 15:44:14 +0800 Subject: [PATCH 2/8] fix #3470 1. add two tests 2. add one validation 3. modified remote remove to satithe validation change --- dvc/command/remote.py | 4 ++-- dvc/config.py | 5 +---- tests/func/test_config.py | 32 +++++++++++++++++++++++++++++++- tests/func/test_remote.py | 21 +++++++++++++++------ 4 files changed, 49 insertions(+), 13 deletions(-) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index c655fa488f..0e8b4a5fe5 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -46,8 +46,8 @@ def run(self): class CmdRemoteRemove(CmdRemote): def run(self): - with self.config.edit(self.args.level) as conf: - self._check_exists(conf) + conf = self.config.load_one(self.args.level) + self._check_exists(conf) # Remove core.remote refs to this remote in any shadowing configs for level in reversed(self.config.LEVELS): diff --git a/dvc/config.py b/dvc/config.py index d06efcad28..e13c46670f 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -363,10 +363,7 @@ def validate(data): "remote" in data["core"] and data["core"]["remote"] not in data["remote"] ): - print(data) - print(data["core"]["remote"]) - print(data["remote"]) - raise ConfigError("") + raise ConfigError("Default remote not in the remote list.") return COMPILED_SCHEMA(data) except Invalid as exc: raise ConfigError(str(exc)) from None diff --git a/tests/func/test_config.py b/tests/func/test_config.py index 9f0293dc33..339732b5f0 100644 --- a/tests/func/test_config.py +++ b/tests/func/test_config.py @@ -78,7 +78,7 @@ def test_non_existing(self): self.assertEqual(ret, 251) ret = main(["config", "core.remote", "myremote"]) - self.assertEqual(ret, 0) + self.assertEqual(ret, 251) ret = main(["config", "core.non_existing_field", "-u"]) self.assertEqual(ret, 251) @@ -109,6 +109,36 @@ def test_merging_two_levels(dvc): } +def test_remote_default_set(dvc): + local_level = "local_remote" + repo_level = "repo_remote" + with dvc.config.edit() as conf: + conf["remote"][repo_level] = {"url": "ssh://example.com"} + with dvc.config.edit() as conf: + conf["remote"][local_level] = {"url": "ssh://example.com"} + + # repo-level remote repo cannot set default in global level + with pytest.raises(ConfigError): + with dvc.config.edit("global") as conf: + conf["core"]["remote"] = repo_level + assert "remote" not in dvc.config["core"] + + # remote default must in remote list + with pytest.raises(ConfigError): + with dvc.config.edit("local") as conf: + conf["core"]["remote"] = "not_in_list" + assert "remote" not in dvc.config["core"] + + with dvc.config.edit() as conf: + conf["core"]["remote"] = repo_level + assert dvc.config["core"]["remote"] == repo_level + + # repo-level remote repo can set default in local level + with dvc.config.edit("local") as conf: + conf["core"]["remote"] = local_level + assert dvc.config["core"]["remote"] == local_level + + def test_config_loads_without_error_for_non_dvc_repo(tmp_dir): # regression testing for https://github.com/iterative/dvc/issues/3328 Config(validate=True) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 3cd6039170..ba122846fc 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -273,10 +273,19 @@ def test_remote_modify_validation(dvc): assert unsupported_config not in config['remote "{}"'.format(remote_name)] -def test_remote_default_modify(dvc): - remote_name = "my_remote" - wrong_remote_name = "anything" - assert main(["remote", "add", remote_name, "s3://bucket/name"]) == 0 +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 main(["remote", "default", remote_name]) == 0 - assert main(["remote", "default", wrong_remote_name]) == 251 + assert repo_config["core"]["remote"] == remote_repo + assert local_config["core"]["remote"] == remote_local From b254d104df65295e0a1156f3979b365be9d38a3d Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Fri, 1 May 2020 16:20:46 +0800 Subject: [PATCH 3/8] Pass DeepSource --- dvc/config.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index e13c46670f..37a7318ab0 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -219,7 +219,8 @@ class Config(dict): CONFIG = "config" CONFIG_LOCAL = "config.local" - def __init__(self, dvc_dir=None, validate=True): + def __init__(self, dvc_dir=None, validate=True, **kwargs): + super().__init__(**kwargs) self.dvc_dir = dvc_dir if not dvc_dir: @@ -238,12 +239,14 @@ def __init__(self, dvc_dir=None, validate=True): def get_dir(cls, level): from appdirs import user_config_dir, site_config_dir - assert level in ("global", "system") - if level == "global": return user_config_dir(cls.APPNAME, cls.APPAUTHOR) - if level == "system": + elif level == "system": return site_config_dir(cls.APPNAME, cls.APPAUTHOR) + else: + raise ConfigError( + "This method only used in global or system level." + ) @cached_property def files(self): From 1ae458c34f49e8d7b35c39c1e6c592b423493a97 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Fri, 1 May 2020 16:24:29 +0800 Subject: [PATCH 4/8] deepsource required --- dvc/config.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index 37a7318ab0..1a5b89ab41 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -241,12 +241,9 @@ def get_dir(cls, level): if level == "global": return user_config_dir(cls.APPNAME, cls.APPAUTHOR) - elif level == "system": + if level == "system": return site_config_dir(cls.APPNAME, cls.APPAUTHOR) - else: - raise ConfigError( - "This method only used in global or system level." - ) + raise ConfigError("This method only used in global or system level.") @cached_property def files(self): From e76e7fdcdd53df288e3fe94d341516524e23fff7 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Fri, 1 May 2020 17:59:27 +0800 Subject: [PATCH 5/8] Redesigned. --- dvc/command/remote.py | 24 ++++++++++++++++++------ dvc/config.py | 11 +++-------- tests/func/test_config.py | 32 +------------------------------- 3 files changed, 22 insertions(+), 45 deletions(-) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index 0e8b4a5fe5..e630354c42 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -46,17 +46,18 @@ def run(self): class CmdRemoteRemove(CmdRemote): def run(self): - conf = self.config.load_one(self.args.level) - self._check_exists(conf) + 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: - del conf["remote"][self.args.name] - break + + if level == self.args.level: + break return 0 @@ -84,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 in remote list." + ) return 0 diff --git a/dvc/config.py b/dvc/config.py index 1a5b89ab41..febfe63611 100644 --- 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) @@ -359,11 +359,6 @@ def edit(self, level="repo"): @staticmethod def validate(data): try: - if ( - "remote" in data["core"] - and data["core"]["remote"] not in data["remote"] - ): - raise ConfigError("Default remote not in the remote list.") return COMPILED_SCHEMA(data) except Invalid as exc: raise ConfigError(str(exc)) from None diff --git a/tests/func/test_config.py b/tests/func/test_config.py index 339732b5f0..9f0293dc33 100644 --- a/tests/func/test_config.py +++ b/tests/func/test_config.py @@ -78,7 +78,7 @@ def test_non_existing(self): self.assertEqual(ret, 251) ret = main(["config", "core.remote", "myremote"]) - self.assertEqual(ret, 251) + self.assertEqual(ret, 0) ret = main(["config", "core.non_existing_field", "-u"]) self.assertEqual(ret, 251) @@ -109,36 +109,6 @@ def test_merging_two_levels(dvc): } -def test_remote_default_set(dvc): - local_level = "local_remote" - repo_level = "repo_remote" - with dvc.config.edit() as conf: - conf["remote"][repo_level] = {"url": "ssh://example.com"} - with dvc.config.edit() as conf: - conf["remote"][local_level] = {"url": "ssh://example.com"} - - # repo-level remote repo cannot set default in global level - with pytest.raises(ConfigError): - with dvc.config.edit("global") as conf: - conf["core"]["remote"] = repo_level - assert "remote" not in dvc.config["core"] - - # remote default must in remote list - with pytest.raises(ConfigError): - with dvc.config.edit("local") as conf: - conf["core"]["remote"] = "not_in_list" - assert "remote" not in dvc.config["core"] - - with dvc.config.edit() as conf: - conf["core"]["remote"] = repo_level - assert dvc.config["core"]["remote"] == repo_level - - # repo-level remote repo can set default in local level - with dvc.config.edit("local") as conf: - conf["core"]["remote"] = local_level - assert dvc.config["core"]["remote"] == local_level - - def test_config_loads_without_error_for_non_dvc_repo(tmp_dir): # regression testing for https://github.com/iterative/dvc/issues/3328 Config(validate=True) From a2eec4dd412f972f51eeef4673d7c85b7c6db1a8 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sat, 9 May 2020 20:24:36 +0300 Subject: [PATCH 6/8] fix typo --- dvc/command/remote.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index e630354c42..fb562ad8c8 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -95,7 +95,7 @@ def run(self): conf["core"]["remote"] = self.args.name else: raise ConfigError( - "default remote must in remote list." + "default remote must be present in remote list." ) return 0 From f66a70fa2e239add3ab005dd0701038463ba0dbf Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sat, 9 May 2020 20:26:18 +0300 Subject: [PATCH 7/8] Revert "deepsource required" This reverts commit 1ae458c34f49e8d7b35c39c1e6c592b423493a97. --- dvc/config.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index febfe63611..ba75c3522b 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -241,9 +241,12 @@ def get_dir(cls, level): if level == "global": return user_config_dir(cls.APPNAME, cls.APPAUTHOR) - if level == "system": + elif level == "system": return site_config_dir(cls.APPNAME, cls.APPAUTHOR) - raise ConfigError("This method only used in global or system level.") + else: + raise ConfigError( + "This method only used in global or system level." + ) @cached_property def files(self): From d31c738ff2b95ef79bb4e2d46307a7953a5a6fa3 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sat, 9 May 2020 20:26:26 +0300 Subject: [PATCH 8/8] Revert "Pass DeepSource" This reverts commit b254d104df65295e0a1156f3979b365be9d38a3d. --- dvc/config.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index ba75c3522b..e18dbe44db 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -219,8 +219,7 @@ class Config(dict): CONFIG = "config" CONFIG_LOCAL = "config.local" - def __init__(self, dvc_dir=None, validate=True, **kwargs): - super().__init__(**kwargs) + def __init__(self, dvc_dir=None, validate=True): self.dvc_dir = dvc_dir if not dvc_dir: @@ -239,14 +238,12 @@ def __init__(self, dvc_dir=None, validate=True, **kwargs): def get_dir(cls, level): from appdirs import user_config_dir, site_config_dir + assert level in ("global", "system") + if level == "global": return user_config_dir(cls.APPNAME, cls.APPAUTHOR) - elif level == "system": + if level == "system": return site_config_dir(cls.APPNAME, cls.APPAUTHOR) - else: - raise ConfigError( - "This method only used in global or system level." - ) @cached_property def files(self):