diff --git a/swh/objstorage/api/server.py b/swh/objstorage/api/server.py --- a/swh/objstorage/api/server.py +++ b/swh/objstorage/api/server.py @@ -175,8 +175,7 @@ # retro compatibility configuration settings app["config"] = config - _cfg = config["objstorage"] - app["objstorage"] = get_objstorage(_cfg["cls"], _cfg["args"]) + app["objstorage"] = get_objstorage(**config["objstorage"]) app.router.add_route("GET", "/", index) app.router.add_route("POST", "/check_config", check_config) @@ -237,20 +236,14 @@ missing_keys = [] vcfg = cfg["objstorage"] - for key in ("cls", "args"): - v = vcfg.get(key) - if v is None: - missing_keys.append(key) - - if missing_keys: - raise KeyError( - "Invalid configuration; missing %s config entry" - % (", ".join(missing_keys),) - ) - - cls = vcfg.get("cls") + if "cls" not in vcfg: + raise KeyError("Invalid configuration; missing cls config entry") + + cls = vcfg["cls"] if cls == "pathslicing": - args = vcfg["args"] + # Backwards-compatibility: either get the deprecated `args` from the + # objstorage config, or use the full config itself to check for keys + args = vcfg.get("args", vcfg) for key in ("root", "slicing"): v = args.get(key) if v is None: @@ -258,7 +251,7 @@ if missing_keys: raise KeyError( - "Invalid configuration; missing args.%s config entry" + "Invalid configuration; missing %s config entry" % (", ".join(missing_keys),) ) diff --git a/swh/objstorage/factory.py b/swh/objstorage/factory.py --- a/swh/objstorage/factory.py +++ b/swh/objstorage/factory.py @@ -4,6 +4,7 @@ # See top-level LICENSE file for more information from typing import Callable, Dict, Union +import warnings from swh.objstorage.api.client import RemoteObjStorage from swh.objstorage.backends.generator import RandomGeneratorObjStorage @@ -59,15 +60,15 @@ _STORAGE_CLASSES_MISSING["swift"] = e.args[0] -def get_objstorage(cls, args): +def get_objstorage(cls: str, args=None, **kwargs): """ Create an ObjStorage using the given implementation class. Args: - cls (str): objstorage class unique key contained in the + cls: objstorage class unique key contained in the _STORAGE_CLASSES dict. - args (dict): arguments for the required class of objstorage - that must match exactly the one in the `__init__` method of the - class. + kwargs: arguments for the required class of objstorage + that must match exactly the one in the `__init__` method of the + class. Returns: subclass of ObjStorage that match the given `storage_class` argument. Raises: @@ -75,7 +76,17 @@ key. """ if cls in _STORAGE_CLASSES: - return _STORAGE_CLASSES[cls](**args) + if args is not None: + warnings.warn( + 'Explicit "args" key is deprecated for objstorage initialization, ' + "use class arguments keys directly instead.", + DeprecationWarning, + ) + # TODO: when removing this, drop the "args" backwards compatibility + # from swh.objstorage.api.server configuration checker + kwargs = args + + return _STORAGE_CLASSES[cls](**kwargs) else: raise ValueError( "Storage class {} is not available: {}".format( diff --git a/swh/objstorage/tests/test_multiplexer_filter.py b/swh/objstorage/tests/test_multiplexer_filter.py --- a/swh/objstorage/tests/test_multiplexer_filter.py +++ b/swh/objstorage/tests/test_multiplexer_filter.py @@ -28,12 +28,13 @@ self.tmpdir = tempfile.mkdtemp() pstorage = { "cls": "pathslicing", - "args": {"root": self.tmpdir, "slicing": "0:5"}, + "root": self.tmpdir, + "slicing": "0:5", } base_storage = get_objstorage(**pstorage) base_storage.id = compute_hash self.storage = get_objstorage( - "filtered", {"storage_conf": pstorage, "filters_conf": [read_only()]} + "filtered", storage_conf=pstorage, filters_conf=[read_only()] ) self.valid_content = b"pre-existing content" self.invalid_content = b"invalid_content" diff --git a/swh/objstorage/tests/test_objstorage_api.py b/swh/objstorage/tests/test_objstorage_api.py --- a/swh/objstorage/tests/test_objstorage_api.py +++ b/swh/objstorage/tests/test_objstorage_api.py @@ -26,11 +26,9 @@ self.config = { "objstorage": { "cls": "pathslicing", - "args": { - "root": self.tmpdir, - "slicing": "0:1/0:5", - "allow_delete": True, - }, + "root": self.tmpdir, + "slicing": "0:1/0:5", + "allow_delete": True, }, "client_max_size": 8 * 1024 * 1024, } diff --git a/swh/objstorage/tests/test_server.py b/swh/objstorage/tests/test_server.py --- a/swh/objstorage/tests/test_server.py +++ b/swh/objstorage/tests/test_server.py @@ -35,41 +35,29 @@ def test_load_and_check_config_no_configuration(): """Inexistent configuration files raises""" - with pytest.raises(EnvironmentError) as e: + with pytest.raises(EnvironmentError, match="Configuration file must be defined"): load_and_check_config(None) - assert e.value.args[0] == "Configuration file must be defined" - config_path = "/indexer/inexistent/config.yml" - with pytest.raises(FileNotFoundError) as e: + with pytest.raises(FileNotFoundError, match=f"{config_path} does not exist"): load_and_check_config(config_path) - assert e.value.args[0] == "Configuration file %s does not exist" % (config_path,) - def test_load_and_check_config_invalid_configuration_toplevel(tmpdir): """Invalid configuration raises""" config = {"something": "useless"} config_path = prepare_config_file(tmpdir, content=config) - with pytest.raises(KeyError) as e: + with pytest.raises(KeyError, match="missing objstorage config entry"): load_and_check_config(config_path) - assert e.value.args[0] == "Invalid configuration; missing objstorage config entry" - def test_load_and_check_config_invalid_configuration(tmpdir): """Invalid configuration raises""" - for data, missing_keys in [ - ({"objstorage": {"something": "useless"}}, ["cls", "args"]), - ({"objstorage": {"cls": "something"}}, ["args"]), - ]: - config_path = prepare_config_file(tmpdir, content=data) - with pytest.raises(KeyError) as e: - load_and_check_config(config_path) - - assert e.value.args[0] == "Invalid configuration; missing %s config entry" % ( - ", ".join(missing_keys), - ) + config_path = prepare_config_file( + tmpdir, content={"objstorage": {"something": "useless"}} + ) + with pytest.raises(KeyError, match="missing cls config entry"): + load_and_check_config(config_path) def test_load_and_check_config_invalid_configuration_level2(tmpdir): @@ -85,31 +73,43 @@ c = copy.deepcopy(config) c["objstorage"]["args"].pop(key) config_path = prepare_config_file(tmpdir, c) - with pytest.raises(KeyError) as e: + with pytest.raises(KeyError, match=f"missing {key} config entry"): load_and_check_config(config_path) - assert ( - e.value.args[0] - == "Invalid configuration; missing args.%s config entry" % key - ) - -def test_load_and_check_config_fine(tmpdir): +@pytest.mark.parametrize( + "config", + [ + pytest.param( + { + "objstorage": { + "cls": "pathslicing", + "args": {"root": "root", "slicing": "slicing"}, + } + }, + id="pathslicing-bw-compat", + ), + pytest.param( + { + "objstorage": { + "cls": "pathslicing", + "root": "root", + "slicing": "slicing", + } + }, + id="pathslicing", + ), + pytest.param( + {"client_max_size": "10", "objstorage": {"cls": "memory", "args": {}}}, + id="empty-args-bw-compat", + ), + pytest.param( + {"client_max_size": "10", "objstorage": {"cls": "memory"}}, id="empty-args" + ), + ], +) +def test_load_and_check_config(tmpdir, config): """pathslicing configuration fine loads ok""" - config = { - "objstorage": { - "cls": "pathslicing", - "args": {"root": "root", "slicing": "slicing",}, - } - } - - config_path = prepare_config_file(tmpdir, config) - cfg = load_and_check_config(config_path) - assert cfg == config - - -def test_load_and_check_config_fine2(tmpdir): - config = {"client_max_size": "10", "objstorage": {"cls": "remote", "args": {}}} config_path = prepare_config_file(tmpdir, config) cfg = load_and_check_config(config_path) assert cfg == config