From f649df56b8797f8c24e99d9d3b0f30c4e50e21a2 Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Wed, 16 Dec 2020 19:21:01 -0500 Subject: [PATCH 1/5] Allow YAML list format for config path options --- lib/galaxy/config/__init__.py | 8 ++++++-- test/unit/config/test_path_resolves_to.py | 9 +++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 777ee0755b4..5f78f2b17cd 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -274,13 +274,17 @@ class BaseAppConfiguration: datatype = self.schema.app_schema[key].get('type') # check for `not None` explicitly (value can be falsy) if value is not None and datatype in type_converters: - return type_converters[datatype](value) + # convert value or each item in value to type `datatype` + if type(value) is list: + return [type_converters[datatype](item) for item in value] + else: + return type_converters[datatype](value) return value def strip_deprecated_dir(key, value): resolves_to = self.schema.paths_to_resolve.get(key) if resolves_to: # value contains paths that will be resolved - paths = [path.strip() for path in value.split(',')] + paths = listify(value, do_strip=True) for i, path in enumerate(paths): first_dir = path.split(os.sep)[0] # get first directory component if first_dir == self.deprecated_dirs.get(resolves_to): # first_dir is deprecated for this option diff --git a/test/unit/config/test_path_resolves_to.py b/test/unit/config/test_path_resolves_to.py index 6706a525cc2..7782b127f76 100644 --- a/test/unit/config/test_path_resolves_to.py +++ b/test/unit/config/test_path_resolves_to.py @@ -209,6 +209,15 @@ def test_kwargs_listify(mock_init, monkeypatch): assert config.path4 == ['my-config/new1', 'my-config/new2'] +def test_kwargs_as_list_listify(mock_init, monkeypatch): + # Expected: use values from kwargs; each value resolved and listified + new_path4 = ['new1', 'new2'] + config = BaseAppConfiguration(path4=new_path4) + + assert config._raw_config['path4'] == 'new1,new2' + assert config.path4 == ['my-config/new1', 'my-config/new2'] + + @pytest.fixture def mock_check_against_root(mock_init, monkeypatch): From 8b921fc87d9600c63f262d869d53f5f7880178cc Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Wed, 16 Dec 2020 20:15:36 -0500 Subject: [PATCH 2/5] Enable list format for admin_users option --- lib/galaxy/config/__init__.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 5f78f2b17cd..7e4d06cfe07 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -413,10 +413,7 @@ class CommonConfigurationMixin: @admin_users.setter def admin_users(self, value): self._admin_users = value - if value: - self.admin_users_list = [u.strip() for u in value.split(',') if u] - else: # provide empty list for convenience (check membership, etc.) - self.admin_users_list = [] + self.admin_users_list = listify(value) def is_admin_user(self, user): """Determine if the provided user is listed in `admin_users`.""" From 85d0a09f4767bb1140a19092b3163a36884eec96 Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Thu, 17 Dec 2020 09:52:03 -0500 Subject: [PATCH 3/5] Use isinstance() instead of type() Co-authored-by: Nicola Soranzo --- lib/galaxy/config/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 7e4d06cfe07..ce7f92a9a99 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -275,7 +275,7 @@ class BaseAppConfiguration: # check for `not None` explicitly (value can be falsy) if value is not None and datatype in type_converters: # convert value or each item in value to type `datatype` - if type(value) is list: + if isinstance(value, list): return [type_converters[datatype](item) for item in value] else: return type_converters[datatype](value) From 07784b70cd703122da54e8231a739357014cc6cc Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Thu, 17 Dec 2020 10:03:55 -0500 Subject: [PATCH 4/5] Improve readability as per code review --- lib/galaxy/config/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index ce7f92a9a99..3436d35dfd5 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -275,10 +275,11 @@ class BaseAppConfiguration: # check for `not None` explicitly (value can be falsy) if value is not None and datatype in type_converters: # convert value or each item in value to type `datatype` + f = type_converters[datatype] if isinstance(value, list): - return [type_converters[datatype](item) for item in value] + return [f(item) for item in value] else: - return type_converters[datatype](value) + return f(value) return value def strip_deprecated_dir(key, value): From a001f445ff33dc236d732b83c4dcbec28e38b385 Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Thu, 17 Dec 2020 23:27:52 -0500 Subject: [PATCH 5/5] Do not change list value to csv string Remove side effect from strip_deprecated_dir function. For context, see code review. --- lib/galaxy/config/__init__.py | 4 ++++ test/unit/config/test_path_resolves_to.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 3436d35dfd5..8fdef452fdc 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -295,6 +295,10 @@ class BaseAppConfiguration: "to suppress this warning: %s", key, resolves_to, ignore, path ) paths[i] = path[len(ignore):] + + # return list or string, depending on type of `value` + if isinstance(value, list): + return paths return ','.join(paths) return value diff --git a/test/unit/config/test_path_resolves_to.py b/test/unit/config/test_path_resolves_to.py index 7782b127f76..abbf3f22e57 100644 --- a/test/unit/config/test_path_resolves_to.py +++ b/test/unit/config/test_path_resolves_to.py @@ -214,7 +214,7 @@ def test_kwargs_as_list_listify(mock_init, monkeypatch): new_path4 = ['new1', 'new2'] config = BaseAppConfiguration(path4=new_path4) - assert config._raw_config['path4'] == 'new1,new2' + assert config._raw_config['path4'] == ['new1', 'new2'] assert config.path4 == ['my-config/new1', 'my-config/new2']