diff --git a/CHANGES.md b/CHANGES.md index 1230b175cb1..eb0fda8e34f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -21,6 +21,10 @@ issue which could cause jobs to fail if this variable became too long. ### Enhancements +-[#5605](https://github.com/cylc/cylc-flow/pull/5605) - A shorthand for defining +-a list of strings - Before: `cylc command -s "X=['a', 'bc', 'd']"` - After: +-`cylc command -z X=a,bc,d`. + [#5537](https://github.com/cylc/cylc-flow/pull/5537) - Allow parameters in family names to be split, e.g. `FAM`. diff --git a/cylc/flow/option_parsers.py b/cylc/flow/option_parsers.py index d8037e0dddc..cb21f5e8bcf 100644 --- a/cylc/flow/option_parsers.py +++ b/cylc/flow/option_parsers.py @@ -22,7 +22,7 @@ OptionParser, Values, Option, - IndentedHelpFormatter + IndentedHelpFormatter, ) import os import re @@ -363,6 +363,15 @@ class CylcOptionParser(OptionParser): See `cylc help id` for more details. ''')) + CAN_BE_USED_MULTIPLE = ( + " This option can be used multiple times on the command line.") + + NOTE_PERSIST_ACROSS_RESTARTS = ( + " NOTE: these settings persist across workflow restarts," + " but can be set again on the \"cylc play\"" + " command line if they need to be overridden." + ) + STD_OPTIONS = [ OptionSettings( ['-q', '--quiet'], help='Decrease verbosity.', @@ -403,13 +412,27 @@ class CylcOptionParser(OptionParser): " Values should be valid Python literals so strings" " must be quoted" " e.g. 'STR=\"string\"', INT=43, BOOL=True." - " This option can be used multiple " - " times on the command line." - " NOTE: these settings persist across workflow restarts," - " but can be set again on the \"cylc play\"" - " command line if they need to be overridden." + + CAN_BE_USED_MULTIPLE + + NOTE_PERSIST_ACROSS_RESTARTS ), - action='append', default=[], dest='templatevars', useif='jset'), + action='append', default=[], dest='templatevars', useif='jset' + ), + OptionSettings( + ['-z', '--set-list', '--template-list'], + metavar='NAME=VALUE1,VALUE2,...', + help=( + 'Set the value of a Jinja2 template variable in the' + ' workflow definition as a comma separated' + ' list of Python strings.' + ' Values containing commas must be quoted.' + " e.g. '+s STR=a,b,c' => ['a', 'b', 'c']" + " or '+ s STR=a,\"b,c\"' => ['a', 'b,c']" + + CAN_BE_USED_MULTIPLE + + NOTE_PERSIST_ACROSS_RESTARTS + ), + action='append', default=[], dest='templatevars_lists', + useif='jset' + ), OptionSettings( ['--set-file'], metavar='FILE', help=( @@ -418,9 +441,7 @@ class CylcOptionParser(OptionParser): " pairs (one per line)." " As with --set values should be valid Python literals " " so strings must be quoted e.g. STR='string'." - " NOTE: these settings persist across workflow restarts," - " but can be set again on the \"cylc play\"" - " command line if they need to be overridden." + + NOTE_PERSIST_ACROSS_RESTARTS ), action='store', default=None, dest='templatevars_file', useif='jset' diff --git a/cylc/flow/scheduler.py b/cylc/flow/scheduler.py index dd4b8efae74..e192185c388 100644 --- a/cylc/flow/scheduler.py +++ b/cylc/flow/scheduler.py @@ -140,7 +140,7 @@ TASK_STATUS_RUNNING, TASK_STATUS_WAITING, TASK_STATUS_FAILED) -from cylc.flow.templatevars import load_template_vars +from cylc.flow.templatevars import get_template_vars from cylc.flow.util import cli_format from cylc.flow.wallclock import ( get_current_time_string, @@ -278,10 +278,7 @@ def __init__(self, reg: str, options: Values) -> None: self.id = self.tokens.id self.uuid_str = str(uuid4()) self.options = options - self.template_vars = load_template_vars( - self.options.templatevars, - self.options.templatevars_file - ) + self.template_vars = get_template_vars(self.options) # mutable defaults self._profile_amounts = {} diff --git a/cylc/flow/scripts/check_versions.py b/cylc/flow/scripts/check_versions.py index 2aefca4be3e..996eecd0f1f 100755 --- a/cylc/flow/scripts/check_versions.py +++ b/cylc/flow/scripts/check_versions.py @@ -48,7 +48,7 @@ from cylc.flow.id_cli import parse_id from cylc.flow.platforms import get_platform, get_host_from_platform from cylc.flow.remote import construct_ssh_cmd -from cylc.flow.templatevars import load_template_vars +from cylc.flow.templatevars import get_template_vars from cylc.flow.terminal import cli_function if TYPE_CHECKING: @@ -87,7 +87,7 @@ def main(_, options: 'Values', *ids) -> None: workflow_id, flow_file, options, - load_template_vars(options.templatevars, options.templatevars_file)) + get_template_vars(options)) platforms = { config.get_config(['runtime', name, 'platform']) diff --git a/cylc/flow/scripts/diff.py b/cylc/flow/scripts/diff.py index e2512942359..09a541900a5 100755 --- a/cylc/flow/scripts/diff.py +++ b/cylc/flow/scripts/diff.py @@ -39,7 +39,7 @@ icp_option, ) from cylc.flow.config import WorkflowConfig -from cylc.flow.templatevars import load_template_vars +from cylc.flow.templatevars import get_template_vars from cylc.flow.terminal import cli_function if TYPE_CHECKING: @@ -155,9 +155,7 @@ def main(parser: COP, options: 'Values', workflow_id1: str, workflow_id2: str): if workflow_file_1_ == workflow_file_2_: parser.error("You can't diff a single workflow.") print(f"Parsing {workflow_id_1} ({workflow_file_1_})") - template_vars = load_template_vars( - options.templatevars, options.templatevars_file - ) + template_vars = get_template_vars(options) config1 = WorkflowConfig( workflow_id_1, workflow_file_1_, options, template_vars ).cfg diff --git a/cylc/flow/scripts/view.py b/cylc/flow/scripts/view.py index 423480cc20f..495af07a412 100644 --- a/cylc/flow/scripts/view.py +++ b/cylc/flow/scripts/view.py @@ -35,7 +35,7 @@ CylcOptionParser as COP, ) from cylc.flow.parsec.fileparse import read_and_proc -from cylc.flow.templatevars import load_template_vars +from cylc.flow.templatevars import get_template_vars from cylc.flow.terminal import cli_function if TYPE_CHECKING: @@ -114,7 +114,6 @@ async def _main(options: 'Values', workflow_id: str) -> None: src=True, constraint='workflows', ) - # read in the flow.cylc file viewcfg = { 'mark': options.mark, @@ -128,7 +127,7 @@ async def _main(options: 'Values', workflow_id: str) -> None: } for line in read_and_proc( flow_file, - load_template_vars(options.templatevars, options.templatevars_file), + get_template_vars(options), viewcfg=viewcfg, opts=options, ): diff --git a/cylc/flow/templatevars.py b/cylc/flow/templatevars.py index 063acb40bb0..7474761e226 100644 --- a/cylc/flow/templatevars.py +++ b/cylc/flow/templatevars.py @@ -17,15 +17,21 @@ from ast import literal_eval from optparse import Values -from typing import TYPE_CHECKING, Any, Dict +from typing import Any, Dict, List, Optional, Set, Union +from cylc.flow import LOG from cylc.flow.exceptions import InputError from cylc.flow.rundb import CylcWorkflowDAO from cylc.flow.workflow_db_mgr import WorkflowDatabaseManager from cylc.flow.workflow_files import WorkflowFiles -if TYPE_CHECKING: - from pathlib import Path +from pathlib import Path + + +OVERWRITE_WARNING = ( + 'Template variable {} redefined:' + ' the previous value will be overwritten.' +) def get_template_vars_from_db(run_dir: 'Path') -> dict: @@ -77,22 +83,102 @@ def eval_var(var): ) from None -def load_template_vars(template_vars=None, template_vars_file=None): +def parse_string_list(stringlist: str) -> List: + """Parse a comma separated string list into a Python string list. + + Examples: + >>> parse_string_list('a,b,c') + ['a', 'b', 'c'] + >>> parse_string_list('a,"b,b",c') + ['a', 'b,b', 'c'] + >>> parse_string_list("a,'b,b','c'") + ['a', 'b,b', 'c'] + """ + list_ = [] + in_quotes = False + buffer = '' + for char in stringlist: + if char in {'"', "'"}: + in_quotes = not in_quotes + elif not in_quotes and char == ',': + list_.append(buffer) + buffer = '' + else: + buffer += char + list_.append(buffer) + return list_ + + +def load_template_vars( + template_vars: Optional[List[str]] = None, + template_vars_file: Union[Path, str, None] = None, + templatevars_lists: Optional[List[str]] = None +) -> Dict[str, Any]: """Load template variables from key=value strings.""" - res = {} + keys: Set[str] = set() + invalid_vars: str = '' + + # Parse Template vars set by file (-S) + file_tvars: Dict[str, Any] = {} if template_vars_file: with open(template_vars_file, 'r') as handle: for line in handle: line = line.strip().split("#", 1)[0] if not line: continue - key, val = line.split("=", 1) - res[key.strip()] = eval_var(val.strip()) + try: + key, val = line.split("=", 1) + except ValueError: + invalid_vars += f'\n * {line}' + continue + file_tvars[key.strip()] = eval_var(val.strip()) + keys.add(key.strip()) + + cli_tvars: Dict[str, Any] = {} + tvars_lists: Dict[str, str] = {} + for input_, result, func in ( + (template_vars, cli_tvars, eval_var), + (templatevars_lists, tvars_lists, parse_string_list) + ): + for pair in input_ or []: + # Handle ValueError + try: + key, val = pair.split("=", 1) + except ValueError: + invalid_vars += f'\n * {pair}' + continue + key, val = key.strip(), val.strip() + if key in result: + LOG.warning(OVERWRITE_WARNING.format(key)) + result[key] = func(val) + keys.add(key) + + # Raise an error if there are any args without the form key=value.: + if invalid_vars: + raise InputError( + 'Template variables must be set with key=value(s):' + + invalid_vars + ) + + # Explicitly record which source of tvars over-rides which. + res = {} + badkeys = '' + for key in keys: + if key in cli_tvars and key in tvars_lists: + badkeys += ( + f"\n * {key}={cli_tvars[key]} and {key}={tvars_lists[key]}") + else: + res[key] = cli_tvars.get( + key, tvars_lists.get( + key, file_tvars.get(key))) + + # Raise an error if there are any key-clashes between tvars and tvars_list + if badkeys: + raise InputError( + "You can't set the same template variable with both -s and -z:" + + badkeys + ) - if template_vars: - for pair in template_vars: - key, val = pair.split("=", 1) - res[key.strip()] = eval_var(val.strip()) return res @@ -106,4 +192,8 @@ def get_template_vars(options: Values) -> Dict[str, Any]: Returns: template_vars: Template variables to give to a Cylc config. """ - return load_template_vars(options.templatevars, options.templatevars_file) + return load_template_vars( + options.templatevars, + options.templatevars_file, + options.templatevars_lists + ) diff --git a/tests/integration/test_template_variables_cli.py b/tests/integration/test_template_variables_cli.py new file mode 100644 index 00000000000..05620815c0e --- /dev/null +++ b/tests/integration/test_template_variables_cli.py @@ -0,0 +1,39 @@ +#!/usr/bin/env python3 + +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) NIWA & British Crown (Met Office) & Contributors. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +from cylc.flow.scripts.view import get_option_parser, _main as view +from cylc.flow.option_parsers import Options + + +async def test_list_tvars(tmp_path, capsys): + """View shows that lists of comma separated args are converted into + strings: + """ + (tmp_path / 'flow.cylc').write_text( + '#!jinja2\n' + '{% for i in FOO %}\n' + '# {{i}} is string: {{i is string}}\n' + '{% endfor %}\n' + ) + options = Options(get_option_parser())() + options.jinja2 = True + options.templatevars_lists = ['FOO="w,x",y,z'] + await view(options, str(tmp_path)) + result = capsys.readouterr().out.split('\n') + for string in ['w,x', 'y', 'z']: + assert f'# {string} is string: True' in result diff --git a/tests/unit/test_templatevars.py b/tests/unit/test_templatevars.py index 4d414d40647..05ea140e6a4 100644 --- a/tests/unit/test_templatevars.py +++ b/tests/unit/test_templatevars.py @@ -16,11 +16,12 @@ from pathlib import Path import pytest +from pytest import param import tempfile import unittest from cylc.flow import __version__ as cylc_version -from cylc.flow.exceptions import ServiceFileError +from cylc.flow.exceptions import ServiceFileError, InputError from cylc.flow.rundb import CylcWorkflowDAO from cylc.flow.templatevars import ( get_template_vars_from_db, @@ -161,3 +162,61 @@ def test_get_old_tvars_fails_if_cylc_7_db(tmp_path): dbfile.touch() with pytest.raises(ServiceFileError, match='database is incompatible'): get_template_vars_from_db(tmp_path) + + +@pytest.mark.parametrize( + 'tvars_lists, expect', + ( + param( + ['FOO=a,b,c'], {'FOO': ['a', 'b', 'c']}, + id='basic' + ), + param( + ['FOO=a,"b,b",c'], {'FOO': ['a', 'b,b', 'c']}, + id='contains-quotes' + ), + ) +) +def test_load_var_lists(tvars_lists, expect): + """ + load_template_vars should parse template vars lists and dump these + into template vars. + """ + result = load_template_vars(templatevars_lists=tvars_lists) + assert result == expect + + +def test_load_var_lists_warns_overwriting(caplog): + """It wars user if same tvar name used twice.""" + load_template_vars(template_vars=["BAZ='HEY'", "BAZ='THERE'"]) + assert 'BAZ' in caplog.records[-1].message + load_template_vars(templatevars_lists=['FOO=a,b', 'FOO=b,c']) + assert 'FOO' in caplog.records[-1].message + + +def test_load_var_lists_fails(): + """ + load_template_vars should fail if the same var is set using -s and -z + """ + errmsg = r"FOO=a and FOO=\['a', 'b'\]" + with pytest.raises(InputError, match=errmsg): + load_template_vars( + template_vars=['FOO="a"'], + templatevars_lists=['FOO=a,b'] + ) + + +def test_load_vars_invalid_value_pairs(): + with pytest.raises(InputError, match="FOO\n.*qux\n.*BAZ\n.*QUX"): + load_template_vars( + template_vars=['FOO', 'qux'], + templatevars_lists=['BAZ', 'QUX'] + ) + + +def test_load_template_vars_ValueError_fm_file(monkeypatch): + from io import StringIO + handle = StringIO('Hello') + monkeypatch.setattr('builtins.open', lambda *_: handle) + with pytest.raises(InputError, match='Hello'): + load_template_vars(template_vars_file='foo')