diff --git a/aiida_firecrest/transport.py b/aiida_firecrest/transport.py index 2705e39..df4eb0e 100644 --- a/aiida_firecrest/transport.py +++ b/aiida_firecrest/transport.py @@ -128,17 +128,28 @@ def _validate_temp_directory(ctx: Context, param: InteractiveOption, value: str) def _dynamic_info_firecrest_version( ctx: Context, param: InteractiveOption, value: str ) -> str: - """Find the version of the FirecREST server.""" - # note: right now, unfortunately, the version is not exposed in the API. - # See issue https://github.com/eth-cscs/firecrest/issues/204 - # so here we just develope a workaround to get the version from the server - # basically we check if extract/compress endpoint is available + """ + Find the version of the FirecREST server. + This is a callback function for click, interface. + It's called during the parsing of the command line arguments, during `verdi computer configure` command. + If the user enters "None" as value, this function will connect to the server and get the version of the FirecREST server. + Otherwise, it will check if the version is supported. + """ import click + from packaging.version import InvalidVersion + + if value != "None": + try: + parse(value) + except InvalidVersion as err: + # raise in case the version is not valid, e.g. latest, stable, etc. + raise click.BadParameter(f"Invalid input {value}") from err - if value != "0": if parse(value) < parse("1.15.0"): raise click.BadParameter(f"FirecREST api version {value} is not supported") + # If version is provided by the user, and it's supported, we will just return it. + # No print confirmation is needed, to keep things less verbose. return value firecrest_url = ctx.params["url"] @@ -158,16 +169,32 @@ def _dynamic_info_firecrest_version( small_file_size_mb=0.0, api_version="100.0.0", # version is irrelevant here ) + + parameters = transport._client.parameters() try: - transport.listdir(transport._cwd.joinpath(temp_directory), recursive=True) - _version = "1.16.0" - except Exception: - # all sort of exceptions can be raised here, but we don't care. Since this is just a workaround - _version = "1.15.0" + info = next( + ( + item + for item in parameters["general"] # type: ignore[typeddict-item] + if item["name"] == "FIRECREST_VERSION" + ), + None, + ) + if info is not None: + _version = str(info["value"]) + else: + raise KeyError + except KeyError as err: + click.echo("Could not get the version of the FirecREST server") + raise click.Abort() from err + if parse(_version) < parse("1.15.0"): + click.echo(f"FirecREST api version {_version} is not supported") + raise click.Abort() + + # for the sake of uniformity, we will print the version in style only if dynamically retrieved. click.echo( - click.style("Fireport: ", bold=True, fg="magenta") - + f"Deployed version of FirecREST api: v{_version}" + click.style("Fireport: ", bold=True, fg="magenta") + f"FirecRESTapi: {_version}" ) return _version @@ -308,7 +335,7 @@ class FirecrestTransport(Transport): "api_version", { "type": str, - "default": "0", + "default": "None", "non_interactive_default": True, "prompt": "FirecREST api version [Enter 0 to get this info from server]", "help": "The version of the FirecREST api deployed on the server", diff --git a/pyproject.toml b/pyproject.toml index 401a5e4..22d554d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -54,7 +54,7 @@ exclude = [ [tool.ruff] line-length = 120 -extend-select = ["A", "B", "C4", "ICN", "ISC", "N", "RUF", "SIM"] +lint.extend-select = ["A", "B", "C4", "ICN", "ISC", "N", "RUF", "SIM"] # extend-ignore = [] [tool.isort] diff --git a/tests/conftest.py b/tests/conftest.py index df53355..785aaa7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -324,6 +324,20 @@ def parameters( "value": "Slurm", } ], + "general": [ + { + "description": "FirecREST version.", + "name": "FIRECREST_VERSION", + "unit": "", + "value": "v1.16.1-dev.9", + }, + { + "description": "FirecREST build timestamp.", + "name": "FIRECREST_BUILD", + "unit": "", + "value": "2024-08-16T15:58:47Z", + }, + ], "storage": [ { "description": "Type of object storage, like `swift`, `s3v2` or `s3v4`.", @@ -361,17 +375,13 @@ def parameters( ], "utilities": [ { - "description": "The maximum allowable file size for various operations" - " of the utilities microservice", + "description": "The maximum allowable file size for various operations of the utilities microservice.", "name": "UTILITIES_MAX_FILE_SIZE", "unit": "MB", "value": "5", }, { - "description": ( - "Maximum time duration for executing the commands " - "in the cluster for the utilities microservice." - ), + "description": "Maximum time duration for executing the commands in the cluster for the utilities microservice.", "name": "UTILITIES_TIMEOUT", "unit": "seconds", "value": "5", diff --git a/tests/test_computer.py b/tests/test_computer.py index 6a2041a..e89ff1c 100644 --- a/tests/test_computer.py +++ b/tests/test_computer.py @@ -107,7 +107,7 @@ def test_validate_temp_directory( ).exists() -def test_dynamic_info(firecrest_config, monkeypatch, tmpdir: Path): +def test_dynamic_info_direct_size(firecrest_config, monkeypatch, tmpdir: Path): from aiida_firecrest.transport import _dynamic_info_direct_size monkeypatch.setattr("click.echo", lambda x: None) @@ -130,3 +130,73 @@ def test_dynamic_info(firecrest_config, monkeypatch, tmpdir: Path): # note: user cannot enter negative numbers anyways, click raise as this shoule be float not str result = _dynamic_info_direct_size(ctx, None, 10) assert result == 10 + + +def test_dynamic_info_firecrest_version( + firecrest_config, monkeypatch, tmpdir: Path, capsys +): + + from unittest.mock import patch + + from click import Abort + + from aiida_firecrest.transport import _dynamic_info_firecrest_version + from conftest import MockFirecrest + + ctx = Mock() + ctx.params = { + "url": f"{firecrest_config.url}", + "token_uri": f"{firecrest_config.token_uri}", + "client_id": f"{firecrest_config.client_id}", + "client_secret": f"{firecrest_config.client_secret}", + "compute_resource": f"{firecrest_config.compute_resource}", + "temp_directory": f"{firecrest_config.temp_directory}", + "api_version": f"{firecrest_config.api_version}", + } + + # should catch FIRECREST_VERSION if value is not provided + result = _dynamic_info_firecrest_version(ctx, None, "None") + assert isinstance(result, str) + + # should use the value if provided + result = _dynamic_info_firecrest_version(ctx, None, "10.10.10") + assert result == "10.10.10" + + # raise BadParameter if the version is not supported + with pytest.raises( + BadParameter, match=r".*FirecREST api version 0.0.0 is not supported.*" + ): + _dynamic_info_firecrest_version(ctx, None, "0.0.0") + + # raise BadParameter if the input is nonsense, latest, stable, etc. + with pytest.raises(BadParameter, match=r".*Invalid input.*"): + _dynamic_info_firecrest_version(ctx, None, "latest") + + # in case could not get the version from the server, should abort, as it is a required parameter. + with patch.object(MockFirecrest, "parameters", autospec=True) as mock_parameters: + mock_parameters.return_value = {"there is no version key": "bye bye"} + with pytest.raises(Abort): + result = _dynamic_info_firecrest_version(ctx, None, "None") + capture = capsys.readouterr() + assert "Could not get the version of the FirecREST server" in capture.out + + # in case the version recieved from the server is not supported, should abort, as no magic input can solve this problem. + with patch.object(MockFirecrest, "parameters", autospec=True) as mock_parameters: + unsupported_version = "0.1" + mock_parameters.return_value = { + "general": [ + { + "description": "FirecREST version.", + "name": "FIRECREST_VERSION", + "unit": "", + "value": f"v{unsupported_version}", + }, + ] + } + with pytest.raises(Abort): + result = _dynamic_info_firecrest_version(ctx, None, "None") + capture = capsys.readouterr() + assert ( + f"FirecREST api version v{unsupported_version} is not supported" + in capture.out + )