From 2992e02b75b7ddd4025169b1a7f2c6fa19ca606b Mon Sep 17 00:00:00 2001 From: Jakub Kaczmarzyk Date: Wed, 18 Jan 2023 16:07:07 -0500 Subject: [PATCH] fix the types in --copy and --entrypoint (#500) * fix the types in --copy and --entrypoint Both of these options use the class OptionEatAll. By default, this class will output a string type, but that would result in a string that looked like a tuple. For example, it is possible to use `ast.literal_eval(value)` on the value to convert it back to a tuple. To fix this, this commit sets the type for --copy and --entrypoint to `tuple` and the subsequent code expects the values for --copy and --entrypoint to be tuples. This commit also adds a type hint to an OptionsEatAll internal to help with debugging. * expand issue 498 tests for --copy and --entrypoint * change assertions to raise click.ClickException This formats the error messages better (and with less clutter). This commit also adds a note to contact the devs in the case of errors that users cannot fix (errors in the implementation...). * raise an error if OptionEatAll is used as a str type The fundamental issue leading to #498 was that the values in OptionEatAll were being coerced to a string type if no type was specified in OptionEatAll. This made a tuple `('foo',)` into a string `'("foo",)'`. This commit raises an error if OptionEatAll is used as a string. Indeed, the 'eat-all' nature of this option suggests that there are multiple options, which suggests the type should be a list or tuple. This commit also adds a test for this. --- neurodocker/cli/generate.py | 24 ++++++++++----- neurodocker/cli/tests/test_cli.py | 50 +++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 10 deletions(-) diff --git a/neurodocker/cli/generate.py b/neurodocker/cli/generate.py index 3fa5ffd6..638c5b41 100644 --- a/neurodocker/cli/generate.py +++ b/neurodocker/cli/generate.py @@ -122,11 +122,16 @@ def __init__(self, *args, **kwargs): nargs = kwargs.pop("nargs", -1) assert nargs == -1, "nargs, if set, must be -1 not {}".format(nargs) super(OptionEatAll, self).__init__(*args, **kwargs) + if self.type is click.STRING: + raise ValueError( + "in the current implementation of OptionEatAll, `type` cannot be a" + " string." + ) self._previous_parser_process = None self._eat_all_parser = None def add_to_parser(self, parser, ctx): - def parser_process(value, state): + def parser_process(value, state: click.parser.ParsingState): # method to hook to the parser.process done = False value = [value] @@ -201,6 +206,7 @@ def _get_common_renderer_params() -> ty.List[click.Parameter]: OptionEatAll( ["--copy"], multiple=True, + type=tuple, help=( "Copy files into the container. Provide at least two paths." " The last path is always the destination path in the container." @@ -215,6 +221,7 @@ def _get_common_renderer_params() -> ty.List[click.Parameter]: OptionEatAll( ["--entrypoint"], multiple=True, + type=tuple, help="Set entrypoint of the container", ), OptionEatAll( @@ -317,12 +324,16 @@ def _get_instruction_for_param( d = {"name": param.name, "kwds": {"base_image": value}} # arg elif param.name == "arg": - assert len(value) == 2, "expected key=value pair for --arg" + if len(value) != 2: + raise click.ClickException("expected key=value pair for --arg") k, v = value d = {"name": param.name, "kwds": {"key": k, "value": v}} # copy elif param.name == "copy": - assert len(value) > 1, "expected at least two values for --copy" + if not isinstance(value, tuple): + raise ValueError("expected this value to be a tuple (contact developers)") + if len(value) < 2: + raise click.ClickException("expected at least two values for --copy") source, destination = list(value[:-1]), value[-1] d = {"name": param.name, "kwds": {"source": source, "destination": destination}} # env @@ -331,10 +342,9 @@ def _get_instruction_for_param( d = {"name": param.name, "kwds": {**value}} # entrypoint elif param.name == "entrypoint": - if isinstance(value, str): - value = [value] - else: - value = list(value) + if not isinstance(value, tuple): + raise ValueError("expected this value to be a tuple (contact developers)") + value = list(value) # convert from tuple to list d = {"name": param.name, "kwds": {"args": value}} # install elif param.name == "install": diff --git a/neurodocker/cli/tests/test_cli.py b/neurodocker/cli/tests/test_cli.py index 1f4b860a..406ca936 100644 --- a/neurodocker/cli/tests/test_cli.py +++ b/neurodocker/cli/tests/test_cli.py @@ -6,6 +6,7 @@ import pytest from neurodocker.cli.cli import generate +from neurodocker.cli.generate import OptionEatAll _cmds = ["docker", "singularity"] @@ -42,8 +43,7 @@ def test_minimal_args(cmd: str, pkg_manager: str): assert result.exit_code == 0, result.output -@pytest.mark.xfail(reason="https://github.com/ReproNim/neurodocker/issues/498") -def test_copy(): +def test_copy_issue_498(): runner = CliRunner() result = runner.invoke( generate, @@ -59,7 +59,51 @@ def test_copy(): "file2", ], ) - assert "file1" in (result.output) + assert "file1" in result.output + assert '\nCOPY ["file1", \\\n "file2"]\n' in result.output + + # Fail if given fewer than two inputs to --copy. + result = runner.invoke( + generate, + [ + "docker", + "--pkg-manager", + "apt", + "--base-image", + "debian", + # copy + "--copy", + "file1", + ], + ) + assert "Error: expected at least two values for --copy" in result.output + assert result.exit_code != 0 + + +# Issue #498 references --copy but the same broken behavior is seen in --entrypoint. +def test_entrypoint_issue_498(): + runner = CliRunner() + result = runner.invoke( + generate, + [ + "docker", + "--pkg-manager", + "apt", + "--base-image", + "debian", + "--entrypoint", + "printf", + "this", + "that", + ], + ) + assert '\nENTRYPOINT ["printf", "this", "that"]\n' in result.output + + +def test_optioneatall_type_issue_498(): + with pytest.raises(ValueError): + OptionEatAll(["--foo"], type=str) + OptionEatAll(["--foo"], type=tuple) @pytest.mark.parametrize("cmd", _cmds)