diff --git a/changes.d/5933.fix.md b/changes.d/5933.fix.md new file mode 100644 index 00000000000..8ba0546c760 --- /dev/null +++ b/changes.d/5933.fix.md @@ -0,0 +1 @@ +Fixed bug in `cylc broadcast` (and the GUI Edit Runtime command) where everything after a `#` character in a setting would be stripped out. diff --git a/cylc/flow/parsec/validate.py b/cylc/flow/parsec/validate.py index 4ef66e19862..ecd481a86bb 100644 --- a/cylc/flow/parsec/validate.py +++ b/cylc/flow/parsec/validate.py @@ -26,7 +26,7 @@ import shlex from collections import deque from textwrap import dedent -from typing import List, Dict, Any, Tuple +from typing import List, Dict, Any, Optional, Tuple from metomi.isodatetime.data import Duration, TimePoint from metomi.isodatetime.dumpers import TimePointDumper @@ -373,7 +373,7 @@ def coerce_range(cls, value, keys): return Range((int(min_), int(max_))) @classmethod - def coerce_str(cls, value, keys): + def coerce_str(cls, value, keys) -> str: """Coerce value to a string. Examples: @@ -385,7 +385,7 @@ def coerce_str(cls, value, keys): """ if isinstance(value, list): # handle graph string merging - vraw = [] + vraw: List[str] = [] vals = [value] while vals: val = vals.pop() @@ -512,16 +512,32 @@ def parse_int_range(cls, value): return None @classmethod - def strip_and_unquote(cls, keys, value): + def _unquote(cls, keys: List[str], value: str) -> Optional[str]: + """Unquote value.""" + for substr, rec in ( + ("'''", cls._REC_MULTI_LINE_SINGLE), + ('"""', cls._REC_MULTI_LINE_DOUBLE), + ('"', cls._REC_DQ_VALUE), + ("'", cls._REC_SQ_VALUE) + ): + if value.startswith(substr): + match = rec.match(value) + if not match: + raise IllegalValueError("string", keys, value) + return match[1] + return None + + @classmethod + def strip_and_unquote(cls, keys: List[str], value: str) -> str: """Remove leading and trailing spaces and unquote value. Args: - keys (list): + keys: Keys in nested dict that represents the raw configuration. - value (str): + value: String value in raw configuration. - Return (str): + Return: Processed value. Examples: @@ -529,24 +545,13 @@ def strip_and_unquote(cls, keys, value): 'foo' """ - for substr, rec in [ - ["'''", cls._REC_MULTI_LINE_SINGLE], - ['"""', cls._REC_MULTI_LINE_DOUBLE], - ['"', cls._REC_DQ_VALUE], - ["'", cls._REC_SQ_VALUE]]: - if value.startswith(substr): - match = rec.match(value) - if not match: - raise IllegalValueError("string", keys, value) - value = match.groups()[0] - break - else: - # unquoted - value = value.split(r'#', 1)[0] + val = cls._unquote(keys, value) + if val is None: + val = value.split(r'#', 1)[0] # Note strip() removes leading and trailing whitespace, including # initial newlines on a multiline string: - return dedent(value).strip() + return dedent(val).strip() @classmethod def strip_and_unquote_list(cls, keys, value): @@ -1136,23 +1141,25 @@ def _coerce_type(cls, value): return val -# BACK COMPAT: BroadcastConfigValidator -# The DB at 8.0.x stores Interval values as neither ISO8601 duration -# string or DurationFloat. This has been fixed at 8.1.0, and -# the following class acts as a bridge between fixed and broken. -# url: -# https://github.com/cylc/cylc-flow/pull/5138 -# from: -# 8.0.x -# to: -# 8.1.x -# remove at: -# 8.x class BroadcastConfigValidator(CylcConfigValidator): """Validate and Coerce DB loaded broadcast config to internal objects.""" def __init__(self): CylcConfigValidator.__init__(self) + @classmethod + def coerce_str(cls, value, keys) -> str: + """Coerce value to a string. Unquotes & strips lead/trail whitespace. + + Prevents ParsecValidator from assuming '#' means comments; + '#' has valid uses in shell script such as parameter substitution. + + Examples: + >>> BroadcastConfigValidator.coerce_str('echo "${FOO#*bar}"', None) + 'echo "${FOO#*bar}"' + """ + val = ParsecValidator._unquote(keys, value) or value + return dedent(val).strip() + @classmethod def strip_and_unquote_list(cls, keys, value): """Remove leading and trailing spaces and unquote list value. @@ -1177,6 +1184,18 @@ def strip_and_unquote_list(cls, keys, value): value = value.lstrip('[').rstrip(']') return ParsecValidator.strip_and_unquote_list(keys, value) + # BACK COMPAT: BroadcastConfigValidator.coerce_interval + # The DB at 8.0.x stores Interval values as neither ISO8601 duration + # string or DurationFloat. This has been fixed at 8.1.0, and + # the following method acts as a bridge between fixed and broken. + # url: + # https://github.com/cylc/cylc-flow/pull/5138 + # from: + # 8.0.x + # to: + # 8.1.x + # remove at: + # 8.x @classmethod def coerce_interval(cls, value, keys): """Coerce an ISO 8601 interval into seconds. diff --git a/cylc/flow/scripts/broadcast.py b/cylc/flow/scripts/broadcast.py index d1a62d708da..28bd3bae4e3 100755 --- a/cylc/flow/scripts/broadcast.py +++ b/cylc/flow/scripts/broadcast.py @@ -82,6 +82,7 @@ from ansimarkup import parse as cparse from copy import deepcopy from functools import partial +import os.path import re import sys from tempfile import NamedTemporaryFile @@ -203,7 +204,7 @@ def files_to_settings(settings, setting_files, cancel_mode=False): handle.seek(0, 0) cfg.loadcfg(handle.name) else: - cfg.loadcfg(setting_file) + cfg.loadcfg(os.path.abspath(setting_file)) stack = [([], cfg.get(sparse=True))] while stack: keys, item = stack.pop() diff --git a/tests/functional/broadcast/10-file-1/broadcast.cylc b/tests/functional/broadcast/10-file-1/broadcast.cylc index f429f8a2740..243502ed547 100644 --- a/tests/functional/broadcast/10-file-1/broadcast.cylc +++ b/tests/functional/broadcast/10-file-1/broadcast.cylc @@ -1,6 +1,9 @@ script=""" printenv CYLC_FOOBAR +# This hash char should not cause the rest of the script to be stripped out +# - https://github.com/cylc/cylc-flow/pull/5933 + if (($CYLC_TASK_TRY_NUMBER < 2 )); then false fi diff --git a/tests/unit/parsec/test_validate.py b/tests/unit/parsec/test_validate.py index 8c73b840eb3..3e24bcf6365 100644 --- a/tests/unit/parsec/test_validate.py +++ b/tests/unit/parsec/test_validate.py @@ -18,12 +18,13 @@ from typing import List import pytest -from pytest import approx +from pytest import approx, param from cylc.flow.parsec.config import ConfigNode as Conf from cylc.flow.parsec.OrderedDict import OrderedDictWithDefaults from cylc.flow.parsec.exceptions import IllegalValueError from cylc.flow.parsec.validate import ( + BroadcastConfigValidator, CylcConfigValidator as VDR, DurationFloat, ListValueError, @@ -456,11 +457,9 @@ def test_coerce_int_list(): validator.coerce_int_list(value, ['whatever']) -def test_coerce_str(): - """Test coerce_str.""" - validator = ParsecValidator() - # The good - for value, result in [ +@pytest.mark.parametrize( + 'value, expected', + [ ('', ''), ('Hello World!', 'Hello World!'), ('"Hello World!"', 'Hello World!'), @@ -474,9 +473,15 @@ def test_coerce_str(): 'Hello:\n foo\nGreet\n baz'), ('False', 'False'), ('None', 'None'), - (['a', 'b'], 'a\nb') - ]: - assert validator.coerce_str(value, ['whatever']) == result + (['a', 'b'], 'a\nb'), + ('abc#def', 'abc'), + ] +) +def test_coerce_str(value: str, expected: str): + """Test coerce_str.""" + validator = ParsecValidator() + # The good + assert validator.coerce_str(value, ['whatever']) == expected def test_coerce_str_list(): @@ -498,9 +503,51 @@ def test_coerce_str_list(): assert validator.coerce_str_list(value, ['whatever']) == results -def test_strip_and_unquote(): +@pytest.mark.parametrize('value, expected', [ + param( + "'a'", + 'a', + id="single quotes" + ), + param( + '"\'a\'"', + "'a'", + id="single quotes inside double quotes" + ), + param( + '" a b" # comment', + ' a b', + id="comment outside" + ), + param( + '"""bene\ngesserit"""', + 'bene\ngesserit', + id="multiline double quotes" + ), + param( + "'''kwisatz\n haderach'''", + 'kwisatz\n haderach', + id="multiline single quotes" + ), + param( + '"""a\nb""" # comment', + 'a\nb', + id="multiline with comment outside" + ), +]) +def test_unquote(value: str, expected: str): + """Test strip_and_unquote.""" + assert ParsecValidator._unquote(['a'], value) == expected + + +@pytest.mark.parametrize('value', [ + '"""', + "'''", + "'don't do this'", +]) +def test_strip_and_unquote__bad(value: str): with pytest.raises(IllegalValueError): - ParsecValidator.strip_and_unquote(['a'], '"""') + ParsecValidator.strip_and_unquote(['a'], value) def test_strip_and_unquote_list_parsec(): @@ -692,3 +739,23 @@ def test_type_help_examples(): except Exception: raise Exception( f'Example "{example}" failed for type "{vdr}"') + + +@pytest.mark.parametrize('value, expected', [ + param( + """ + a="don't have a cow" + a=${a#*have} + echo "$a" # let's see what happens + """, + "a=\"don't have a cow\"\na=${a#*have}\necho \"$a\" # let's see what happens", + id="multiline" + ), + param( + '"sleep 30 # ja!" ', + 'sleep 30 # ja!', + id="quoted" + ), +]) +def test_broadcast_coerce_str(value: str, expected: str): + assert BroadcastConfigValidator.coerce_str(value, ['whatever']) == expected