From a9f640a3d399b1590c63268ea7b6d021b456a4cc Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Sun, 12 Apr 2020 21:32:41 +0800 Subject: [PATCH 1/6] remote config validation Fixes #3552 --- dvc/config.py | 1 + tests/func/test_remote.py | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/dvc/config.py b/dvc/config.py index 6e1ccd7f5f..4b33ef68d1 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -342,6 +342,7 @@ def edit(self, level="repo"): yield conf conf = self._save_paths(conf, self.files[level]) + self.validate(conf) _save_config(self.files[level], conf) self.load() diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index f1ebd35068..cd941efbeb 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -49,6 +49,29 @@ def test_relative_path(self): config = configobj.ConfigObj(self.dvc.config.files["repo"]) self.assertEqual(config['remote "mylocal"']["url"], rel) + def test_remote_modify_validation(self): + remote_name = "drive" + unsupported_config = "unsupported_config" + self.assertEqual( + main(["remote", "add", "-d", remote_name, "gdrive://test/test"]), 0 + ) + self.assertEqual( + main( + [ + "remote", + "modify", + remote_name, + unsupported_config, + "something", + ] + ), + 251, + ) + config = configobj.ConfigObj(self.dvc.config.files["repo"]) + self.assertFalse( + unsupported_config in config['remote "{}"'.format(remote_name)] + ) + def test_overwrite(self): remote_name = "a" remote_url = "s3://bucket/name" From 5553be299a4bf61e069d95119adb8d5324a85bd4 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Mon, 13 Apr 2020 07:54:56 +0800 Subject: [PATCH 2/6] solving config test fail --- tests/func/test_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/func/test_config.py b/tests/func/test_config.py index 1ae0118b77..3dc9a2c558 100644 --- a/tests/func/test_config.py +++ b/tests/func/test_config.py @@ -95,7 +95,7 @@ def test_merging_two_levels(dvc): conf["remote"]["test"] = {"url": "ssh://example.com"} with dvc.config.edit("local") as conf: - conf["remote"]["test"] = {"password": "1"} + conf["remote"]["test"] = {"url": "ssh://example.com", "password": "1"} assert dvc.config["remote"]["test"] == { "url": "ssh://example.com", From a3031cb812dbb3b80c70875c6104eeb4e3be79e3 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Tue, 14 Apr 2020 22:23:34 +0800 Subject: [PATCH 3/6] Validation problem in config merging sloving validation problem in config merging from different levels. Only those config in lower levels can affect the validation process. --- dvc/config.py | 11 ++++++++++- tests/func/test_config.py | 8 +++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index 4b33ef68d1..5b52cbd823 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -342,7 +342,16 @@ def edit(self, level="repo"): yield conf conf = self._save_paths(conf, self.files[level]) - self.validate(conf) + + merged_conf = {} + for merge_level in self.LEVELS: + if merge_level == level: + _merge(merged_conf, conf) + break + elif merge_level in self.files: + _merge(merged_conf, self.load_one(merge_level)) + self.validate(merged_conf) + _save_config(self.files[level], conf) self.load() diff --git a/tests/func/test_config.py b/tests/func/test_config.py index 3dc9a2c558..9f0293dc33 100644 --- a/tests/func/test_config.py +++ b/tests/func/test_config.py @@ -94,8 +94,14 @@ def test_merging_two_levels(dvc): with dvc.config.edit() as conf: conf["remote"]["test"] = {"url": "ssh://example.com"} + with pytest.raises( + ConfigError, match=r"expected 'url' for dictionary value" + ): + with dvc.config.edit("global") as conf: + conf["remote"]["test"] = {"password": "1"} + with dvc.config.edit("local") as conf: - conf["remote"]["test"] = {"url": "ssh://example.com", "password": "1"} + conf["remote"]["test"] = {"password": "1"} assert dvc.config["remote"]["test"] == { "url": "ssh://example.com", From 5fa3cd70b78d5963c4a36af7a9957fb1f4912f4e Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Tue, 14 Apr 2020 22:39:22 +0800 Subject: [PATCH 4/6] reduce Cognitive Complexity --- dvc/config.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index 5b52cbd823..7767b96c6c 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -278,10 +278,7 @@ def load(self, validate=True): Raises: ConfigError: thrown if config has an invalid format. """ - conf = {} - for level in self.LEVELS: - if level in self.files: - _merge(conf, self.load_one(level)) + conf = self._load_config_to_level("all_levels") if validate: conf = self.validate(conf) @@ -333,6 +330,15 @@ 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): + merged_conf = {} + for merge_level in self.LEVELS: + if merge_level in self.files: + _merge(merged_conf, self.load_one(merge_level)) + if merge_level == level: + break + return merged_conf + @contextmanager def edit(self, level="repo"): if level in {"repo", "local"} and self.dvc_dir is None: @@ -343,13 +349,8 @@ def edit(self, level="repo"): conf = self._save_paths(conf, self.files[level]) - merged_conf = {} - for merge_level in self.LEVELS: - if merge_level == level: - _merge(merged_conf, conf) - break - elif merge_level in self.files: - _merge(merged_conf, self.load_one(merge_level)) + merged_conf = self._load_config_to_level(level) + _merge(merged_conf, conf) self.validate(merged_conf) _save_config(self.files[level], conf) From ed14ea883477279feab99ee887191ae8676afb04 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Wed, 15 Apr 2020 21:16:13 +0800 Subject: [PATCH 5/6] Two request changes 1. Duplicate load current level of config 2. rewrite test to pytest style --- dvc/config.py | 4 ++-- tests/func/test_remote.py | 39 ++++++++++++++++----------------------- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index 7767b96c6c..ff55e42802 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -333,10 +333,10 @@ def _map_dirs(conf, func): def _load_config_to_level(self, level): merged_conf = {} for merge_level in self.LEVELS: - if merge_level in self.files: - _merge(merged_conf, self.load_one(merge_level)) if merge_level == level: break + if merge_level in self.files: + _merge(merged_conf, self.load_one(merge_level)) return merged_conf @contextmanager diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index cd941efbeb..1032f82099 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -49,29 +49,6 @@ def test_relative_path(self): config = configobj.ConfigObj(self.dvc.config.files["repo"]) self.assertEqual(config['remote "mylocal"']["url"], rel) - def test_remote_modify_validation(self): - remote_name = "drive" - unsupported_config = "unsupported_config" - self.assertEqual( - main(["remote", "add", "-d", remote_name, "gdrive://test/test"]), 0 - ) - self.assertEqual( - main( - [ - "remote", - "modify", - remote_name, - unsupported_config, - "something", - ] - ), - 251, - ) - config = configobj.ConfigObj(self.dvc.config.files["repo"]) - self.assertFalse( - unsupported_config in config['remote "{}"'.format(remote_name)] - ) - def test_overwrite(self): remote_name = "a" remote_url = "s3://bucket/name" @@ -259,3 +236,19 @@ def test_external_dir_resource_on_no_cache(tmp_dir, dvc, tmp_path_factory): dvc.cache.local = None with pytest.raises(RemoteCacheRequiredError): dvc.run(deps=[fspath(external_dir)]) + + +def test_remote_modify_validation(dvc): + remote_name = "drive" + unsupported_config = "unsupported_config" + assert ( + main(["remote", "add", "-d", remote_name, "gdrive://test/test"]) == 0 + ) + assert ( + main( + ["remote", "modify", remote_name, unsupported_config, "something"] + ) + == 251 + ) + config = configobj.ConfigObj(dvc.config.files["repo"]) + assert unsupported_config not in config['remote "{}"'.format(remote_name)] From acf8112e7ca7f36f1fe5e3bb2a15d89e43d64137 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 15 Apr 2020 13:21:22 +0000 Subject: [PATCH 6/6] Restyled by black --- tests/func/test_remote.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index a4d2c56a5e..4369203dd7 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -240,8 +240,8 @@ def test_external_dir_resource_on_no_cache(tmp_dir, dvc, tmp_path_factory): dvc.cache.local = None with pytest.raises(RemoteCacheRequiredError): dvc.run(deps=[fspath(external_dir)]) - - + + def test_push_order(tmp_dir, dvc, tmp_path_factory, mocker): url = fspath(tmp_path_factory.mktemp("upstream")) dvc.config["remote"]["upstream"] = {"url": url} @@ -270,5 +270,3 @@ def test_remote_modify_validation(dvc): ) config = configobj.ConfigObj(dvc.config.files["repo"]) assert unsupported_config not in config['remote "{}"'.format(remote_name)] - -