_dynamic_info_firecrest_version updated with new development (#60)

* _dynamic_info_firecrest_version updated
taking use of `/status/parameters` now.

---------

Co-authored-by: Alexander Goscinski <[email protected]>

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",

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]

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",

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
+        )