From e488705ee38a6810f240d7546d577c65960457a3 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 22 Nov 2023 12:00:50 +0000 Subject: [PATCH 1/7] Do not allow plugins to act on global.cylc --- cylc/flow/parsec/fileparse.py | 58 +++++++++++++----------- tests/unit/parsec/test_fileparse.py | 39 ++++++++++++++-- tests/unit/plugins/test_pre_configure.py | 8 ++-- 3 files changed, 70 insertions(+), 35 deletions(-) diff --git a/cylc/flow/parsec/fileparse.py b/cylc/flow/parsec/fileparse.py index 42e8a4aa40b..e40f60de9f3 100644 --- a/cylc/flow/parsec/fileparse.py +++ b/cylc/flow/parsec/fileparse.py @@ -30,6 +30,7 @@ value type is known). """ +from copy import deepcopy import os from pathlib import Path import re @@ -51,6 +52,7 @@ if t.TYPE_CHECKING: from optparse import Values + from typing import Dict, Union # heading/sections can contain commas (namespace name lists) and any @@ -105,6 +107,12 @@ _UNCLOSED_MULTILINE = re.compile( r'(?])\[.*\]' ) +TEMPLATING_DETECTED = 'templating_detected' +EXTRA_VARS_TEMPLATE = { + 'env': {}, + 'template_variables': {}, + 'templating_detected': None +} def get_cylc_env_vars() -> t.Dict[str, str]: @@ -242,7 +250,7 @@ def multiline(flines, value, index, maxline): return quot + newvalue + line, index -def process_plugins(fpath, opts): +def process_plugins(fpath: 'Union[str, Path]', opts: 'Values'): """Run a Cylc pre-configuration plugin. Plugins should return a dictionary containing: @@ -263,12 +271,16 @@ def process_plugins(fpath, opts): 'templating_detected': None } """ + fpath = Path(fpath) + # Set out blank dictionary for return: - extra_vars = { - 'env': {}, - 'template_variables': {}, - 'templating_detected': None - } + extra_vars = deepcopy(EXTRA_VARS_TEMPLATE) + + # Don't run this on global configs: + if fpath.name == 'global.cylc': + return extra_vars + + fpath = fpath.parent # Run entry point pre_configure items, trying to merge values with each.: for entry_point in iter_entry_points( @@ -302,26 +314,20 @@ def process_plugins(fpath, opts): ) extra_vars[section].update(section_update) + templating_detected = plugin_result.get(TEMPLATING_DETECTED, None) if ( - 'templating_detected' in plugin_result and - plugin_result['templating_detected'] is not None and - extra_vars['templating_detected'] is not None and - extra_vars['templating_detected'] != - plugin_result['templating_detected'] + templating_detected is not None + and extra_vars[TEMPLATING_DETECTED] is not None + and extra_vars[TEMPLATING_DETECTED] != templating_detected ): # Don't allow subsequent plugins with different templating_detected raise ParsecError( - "Can't merge templating languages " - f"{extra_vars['templating_detected']} and " - f"{plugin_result['templating_detected']}" + "Can't merge templating languages" + f"{extra_vars[TEMPLATING_DETECTED]} and " + f"{templating_detected}" ) - elif ( - 'templating_detected' in plugin_result and - plugin_result['templating_detected'] is not None - ): - extra_vars['templating_detected'] = plugin_result[ - 'templating_detected' - ] + elif plugin_result.get(TEMPLATING_DETECTED, None) is not None: + extra_vars[TEMPLATING_DETECTED] = templating_detected return extra_vars @@ -352,7 +358,7 @@ def merge_template_vars( >>> merge_template_vars(a, b) {'FOO': 42, 'BAZ': 3.14159, 'BAR': 'Hello World'} """ - if plugin_result['templating_detected'] is not None: + if plugin_result[TEMPLATING_DETECTED] is not None: plugin_tvars = plugin_result['template_variables'] will_be_overwritten = ( native_tvars.keys() & @@ -456,7 +462,7 @@ def read_and_proc( do_jinja2 = True do_contin = True - extra_vars = process_plugins(Path(fpath).parent, opts) + extra_vars = process_plugins(fpath, opts) if not template_vars: template_vars = {} @@ -483,12 +489,12 @@ def read_and_proc( # Fail if templating_detected ≠ hashbang process_with = hashbang_and_plugin_templating_clash( - extra_vars['templating_detected'], flines + extra_vars[TEMPLATING_DETECTED], flines ) # process with EmPy if do_empy: if ( - extra_vars['templating_detected'] == 'empy' and + extra_vars[TEMPLATING_DETECTED] == 'empy' and not process_with and process_with != 'empy' ): @@ -508,7 +514,7 @@ def read_and_proc( # process with Jinja2 if do_jinja2: if ( - extra_vars['templating_detected'] == 'jinja2' and + extra_vars[TEMPLATING_DETECTED] == 'jinja2' and not process_with and process_with != 'jinja2' ): diff --git a/tests/unit/parsec/test_fileparse.py b/tests/unit/parsec/test_fileparse.py index c9732e9a315..569632e131d 100644 --- a/tests/unit/parsec/test_fileparse.py +++ b/tests/unit/parsec/test_fileparse.py @@ -32,6 +32,7 @@ ) from cylc.flow.parsec.OrderedDict import OrderedDictWithDefaults from cylc.flow.parsec.fileparse import ( + EXTRA_VARS_TEMPLATE, _prepend_old_templatevars, _get_fpath_for_source, get_cylc_env_vars, @@ -39,6 +40,7 @@ addsect, multiline, parse, + process_plugins, read_and_proc, merge_template_vars ) @@ -741,7 +743,7 @@ def test_get_fpath_for_source(tmp_path): def test_user_has_no_cwd(tmp_path): """Test we can parse a config file even if cwd does not exist.""" - cwd = tmp_path/"cwd" + cwd = tmp_path / "cwd" os.mkdir(cwd) os.chdir(cwd) os.rmdir(cwd) @@ -765,15 +767,42 @@ def test_get_cylc_env_vars(monkeypatch): { "CYLC_VERSION": "betwixt", "CYLC_ENV_NAME": "between", - "CYLC_QUESTION": "que?", - "CYLC_ANSWER": "42", + "CYLC_QUESTION": "que?", + "CYLC_ANSWER": "42", "FOO": "foo" } ) assert ( get_cylc_env_vars() == { - "CYLC_QUESTION": "que?", - "CYLC_ANSWER": "42", + "CYLC_QUESTION": "que?", + "CYLC_ANSWER": "42", } ) + +class EntryPointWrapper: + """Wraps a method to make it look like an entry point.""" + + def __init__(self, fcn): + self.name = fcn.__name__ + self.fcn = fcn + + def load(self): + return self.fcn + + +@EntryPointWrapper +def pre_configure_basic(*_, **__): + """Simple plugin that returns one env var and one template var.""" + return {'env': {'foo': 44}, 'template_variables': {}} + + +def test_plugins_not_called_on_global_config(monkeypatch): + monkeypatch.setattr( + 'cylc.flow.parsec.fileparse.iter_entry_points', + lambda x: [pre_configure_basic] + ) + result = process_plugins('/pennine/way/flow.cylc', {}) + assert result != EXTRA_VARS_TEMPLATE + result = process_plugins('/appalachian/trail/global.cylc', {}) + assert result == EXTRA_VARS_TEMPLATE diff --git a/tests/unit/plugins/test_pre_configure.py b/tests/unit/plugins/test_pre_configure.py index e9f91054e12..5f54571e741 100644 --- a/tests/unit/plugins/test_pre_configure.py +++ b/tests/unit/plugins/test_pre_configure.py @@ -68,7 +68,7 @@ def test_pre_configure(monkeypatch): 'cylc.flow.parsec.fileparse.iter_entry_points', lambda x: [pre_configure_basic] ) - extra_vars = process_plugins(None, None) + extra_vars = process_plugins('/', None) assert extra_vars == { 'env': { 'ANSWER': '42' @@ -90,7 +90,7 @@ def test_pre_configure_duplicate(monkeypatch): ] ) with pytest.raises(ParsecError): - process_plugins(None, None) + process_plugins('/', None) def test_pre_configure_templating_detected(monkeypatch): @@ -103,7 +103,7 @@ def test_pre_configure_templating_detected(monkeypatch): ] ) with pytest.raises(ParsecError): - process_plugins(None, None) + process_plugins('/', None) def test_pre_configure_exception(monkeypatch): @@ -113,7 +113,7 @@ def test_pre_configure_exception(monkeypatch): lambda x: [pre_configure_error] ) with pytest.raises(PluginError) as exc_ctx: - process_plugins(None, None) + process_plugins('/', None) # the context of the original error should be preserved in the raised # exception assert exc_ctx.value.entry_point == 'cylc.pre_configure' From 5a3b1df27c7f7ef2388862f3c6c73f741ba89bf8 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 23 Nov 2023 13:33:29 +0000 Subject: [PATCH 2/7] Update cylc/flow/parsec/fileparse.py --- cylc/flow/parsec/fileparse.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/flow/parsec/fileparse.py b/cylc/flow/parsec/fileparse.py index e40f60de9f3..6556d271b1a 100644 --- a/cylc/flow/parsec/fileparse.py +++ b/cylc/flow/parsec/fileparse.py @@ -326,7 +326,7 @@ def process_plugins(fpath: 'Union[str, Path]', opts: 'Values'): f"{extra_vars[TEMPLATING_DETECTED]} and " f"{templating_detected}" ) - elif plugin_result.get(TEMPLATING_DETECTED, None) is not None: + else: extra_vars[TEMPLATING_DETECTED] = templating_detected return extra_vars From 9400fc9f5e9b78bf2fcf8d65880a537f7421216c Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 28 Nov 2023 13:10:10 +0000 Subject: [PATCH 3/7] Apply suggestions from code review Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- cylc/flow/parsec/fileparse.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cylc/flow/parsec/fileparse.py b/cylc/flow/parsec/fileparse.py index 6556d271b1a..21d616de28f 100644 --- a/cylc/flow/parsec/fileparse.py +++ b/cylc/flow/parsec/fileparse.py @@ -111,7 +111,7 @@ EXTRA_VARS_TEMPLATE = { 'env': {}, 'template_variables': {}, - 'templating_detected': None + TEMPLATING_DETECTED: None } @@ -322,7 +322,7 @@ def process_plugins(fpath: 'Union[str, Path]', opts: 'Values'): ): # Don't allow subsequent plugins with different templating_detected raise ParsecError( - "Can't merge templating languages" + "Can't merge templating languages " f"{extra_vars[TEMPLATING_DETECTED]} and " f"{templating_detected}" ) From 3638eb5b25bbbc0302801e98e89a4ea735a54677 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 28 Nov 2023 13:26:41 +0000 Subject: [PATCH 4/7] response to review --- cylc/flow/parsec/fileparse.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cylc/flow/parsec/fileparse.py b/cylc/flow/parsec/fileparse.py index 21d616de28f..ea0d655a095 100644 --- a/cylc/flow/parsec/fileparse.py +++ b/cylc/flow/parsec/fileparse.py @@ -108,9 +108,10 @@ r'(?])\[.*\]' ) TEMPLATING_DETECTED = 'templating_detected' +TEMPLATE_VARIABLES = 'template_variables' EXTRA_VARS_TEMPLATE = { 'env': {}, - 'template_variables': {}, + TEMPLATE_VARIABLES: {}, TEMPLATING_DETECTED: None } @@ -261,14 +262,15 @@ def process_plugins(fpath: 'Union[str, Path]', opts: 'Values'): or ``empy``. args: - fpath: Directory where the plugin will look for a config. + fpath: Path to a config file. Plugin will look at the parent + directory of this file. opts: Command line options to be passed to the plugin. Returns: Dictionary in the form: extra_vars = { 'env': {}, 'template_variables': {}, - 'templating_detected': None + TEMPLATING_DETECTED: None } """ fpath = Path(fpath) @@ -300,7 +302,7 @@ def process_plugins(fpath: 'Union[str, Path]', opts: 'Values'): entry_point.name, exc ) from None - for section in ['env', 'template_variables']: + for section in ['env', TEMPLATE_VARIABLES]: if section in plugin_result and plugin_result[section] is not None: # Raise error if multiple plugins try to update the same keys. section_update = plugin_result.get(section, {}) @@ -359,7 +361,7 @@ def merge_template_vars( {'FOO': 42, 'BAZ': 3.14159, 'BAR': 'Hello World'} """ if plugin_result[TEMPLATING_DETECTED] is not None: - plugin_tvars = plugin_result['template_variables'] + plugin_tvars = plugin_result[TEMPLATE_VARIABLES] will_be_overwritten = ( native_tvars.keys() & plugin_tvars.keys() From fa6ce1d0a7187ef1f41e26d6041ee8727aaf6631 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 28 Nov 2023 13:37:07 +0000 Subject: [PATCH 5/7] fix --- cylc/flow/parsec/fileparse.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/flow/parsec/fileparse.py b/cylc/flow/parsec/fileparse.py index ea0d655a095..24f6cfcca6e 100644 --- a/cylc/flow/parsec/fileparse.py +++ b/cylc/flow/parsec/fileparse.py @@ -52,7 +52,7 @@ if t.TYPE_CHECKING: from optparse import Values - from typing import Dict, Union + from typing import Union # heading/sections can contain commas (namespace name lists) and any From 59b331d1cecfabd54b1a17abf6b06a2b40e28beb Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Mon, 4 Dec 2023 12:51:48 +0000 Subject: [PATCH 6/7] response to review --- cylc/flow/parsec/fileparse.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cylc/flow/parsec/fileparse.py b/cylc/flow/parsec/fileparse.py index 24f6cfcca6e..236616ce38e 100644 --- a/cylc/flow/parsec/fileparse.py +++ b/cylc/flow/parsec/fileparse.py @@ -282,8 +282,6 @@ def process_plugins(fpath: 'Union[str, Path]', opts: 'Values'): if fpath.name == 'global.cylc': return extra_vars - fpath = fpath.parent - # Run entry point pre_configure items, trying to merge values with each.: for entry_point in iter_entry_points( 'cylc.pre_configure' @@ -292,7 +290,7 @@ def process_plugins(fpath: 'Union[str, Path]', opts: 'Values'): # If you want it to work on sourcedirs you need to get the options # to here. plugin_result = entry_point.load()( - srcdir=fpath, opts=opts + srcdir=fpath.parent, opts=opts ) except Exception as exc: # NOTE: except Exception (purposefully vague) From 6b99618d9a0b0499e36f7e426aa01b021a6114f3 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 8 Dec 2023 11:23:06 +0000 Subject: [PATCH 7/7] fix mypy --- cylc/flow/parsec/fileparse.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/flow/parsec/fileparse.py b/cylc/flow/parsec/fileparse.py index 236616ce38e..4ba55d18180 100644 --- a/cylc/flow/parsec/fileparse.py +++ b/cylc/flow/parsec/fileparse.py @@ -109,7 +109,7 @@ ) TEMPLATING_DETECTED = 'templating_detected' TEMPLATE_VARIABLES = 'template_variables' -EXTRA_VARS_TEMPLATE = { +EXTRA_VARS_TEMPLATE: t.Dict[str, t.Any] = { 'env': {}, TEMPLATE_VARIABLES: {}, TEMPLATING_DETECTED: None