Allow change of templating language when reinstalling (#192)

* Allow change of templating language when reinstalling
- Move checks for multiple templating sections
  to before old and new configs are merged
- Algorithm merging previous and current config
  renames old template variables section if
  new install has changes it.
- Update tests for merge algorithm.
- Remove test that it's impossible to change
  templating languages on reinstall - it
  should not be.
* re-add check re

CHANGES.md

@@ -10,6 +10,8 @@ ones in. -->
 
 ### Fixes
 
+[#192](https://github.com/cylc/cylc-rose/pull/192) - Fix bug where Cylc Rose would prevent change to template language on reinstall.
+
 [#180](https://github.com/cylc/cylc-rose/pull/180) - Rose stem gets stem
 suite's basename to use as workflow name when not otherwise set.
 

cylc/rose/entry_points.py

@@ -180,6 +180,8 @@ def record_cylc_install_options(
     # Leave now if there is nothing to do:
     if not cli_config:
         return False
+    # raise error if CLI config has multiple templating sections
+    identify_templating_section(cli_config)
 
     # Construct path objects representing our target files.
     (Path(rundir) / 'opt').mkdir(exist_ok=True)
@@ -195,6 +197,8 @@ def record_cylc_install_options(
             conf_filepath.unlink()
         else:
             oldconfig = loader.load(str(conf_filepath))
+            # Check old config for clashing template variables sections.
+            identify_templating_section(oldconfig)
             cli_config = merge_rose_cylc_suite_install_conf(
                 oldconfig, cli_config
             )
@@ -211,7 +215,6 @@ def record_cylc_install_options(
             ]
 
     cli_config.comments = [' This file records CLI Options.']
-    identify_templating_section(cli_config)
     dumper.dump(cli_config, str(conf_filepath))
 
     # Merge the opts section of the rose-suite.conf with those set by CLI:
@@ -222,6 +225,7 @@ def record_cylc_install_options(
         rose_suite_conf, cli_config
     )
     identify_templating_section(rose_suite_conf)
+
     dumper(rose_suite_conf, rose_conf_filepath)
 
     return cli_config, rose_suite_conf

cylc/rose/utilities.py

@@ -16,7 +16,7 @@
 """Cylc support for reading and interpreting ``rose-suite.conf`` workflow
 configuration files.
 """
-
+import itertools
 import os
 from pathlib import Path
 import re
@@ -179,6 +179,11 @@ def get_rose_vars_from_config_node(config, config_node, environ):
 
 
 def identify_templating_section(config_node):
+    """Get the name of the templating section.
+
+    Raises MultipleTemplatingEnginesError if multiple
+    templating sections exist.
+    """
     defined_sections = SECTIONS.intersection(set(config_node.value.keys()))
     if len(defined_sections) > 1:
         raise MultipleTemplatingEnginesError(
@@ -280,6 +285,8 @@ def merge_rose_cylc_suite_install_conf(old, new):
     Opts are merged separately to allow special behaviour.
     The rest is merged using ConfigNodeDiff.
 
+    If the template language has changed, use the new templating language.
+
     Args:
         old, new (ConfigNode):
             Old and new nodes.
@@ -294,6 +301,14 @@ def merge_rose_cylc_suite_install_conf(old, new):
         >>> merge_rose_cylc_suite_install_conf(old, new)['opts']
         {'value': 'a b c d e', 'state': '', 'comments': []}
     """
+    # remove jinja2/empy:suite.rc from old if template variables in new
+    for before, after in itertools.permutations(SECTIONS, 2):
+        if new.value.get(after, '') and old.value.get(before, ''):
+            # Choosing not to warn if user downgrades here because
+            # other checks warn of old sections.
+            old.value[after] = old.value[before]
+            old.value.pop(before)
+
     # Special treatement of opts key:
     if 'opts' in old and 'opts' in new:
         new_opts_str = f'{old["opts"].value} {new["opts"].value}'
@@ -306,6 +321,7 @@ def merge_rose_cylc_suite_install_conf(old, new):
     diff.set_from_configs(old, new)
     diff.delete_removed()
     old.add(diff)
+
     return old
 
 

tests/functional/test_reinstall.py

@@ -250,38 +250,3 @@ def test_cylc_reinstall_run2(fixture_reinstall_flow2):
 def test_cylc_reinstall_files2(fixture_reinstall_flow2, file_, expect):
     fpath = fixture_reinstall_flow2['fixture_provide_flow']['flowpath']
     assert (fpath / file_).read_text() == expect
-
-
-def test_cylc_reinstall_fail_on_clashing_template_vars(tmp_path, request):
-    """If you re-install with a different templating engine in suite.rc
-    reinstall should fail.
-    """
-    (tmp_path / 'rose-suite.conf').write_text(
-        '[jinja2:suite.rc]\n'
-        'Primrose=\'Primula Vulgaris\'\n'
-    )
-    (tmp_path / 'flow.cylc').touch()
-    test_flow_name = f'cylc-rose-test-{str(uuid4())[:8]}'
-    install = subprocess.run(
-        [
-            'cylc', 'install', str(tmp_path), '--workflow-name',
-            test_flow_name, '--no-run-name'
-        ]
-    )
-    assert install.returncode == 0
-    (tmp_path / 'rose-suite.conf').write_text(
-        '[empy:suite.rc]\n'
-        'Primrose=\'Primula Vulgaris\'\n'
-    )
-    reinstall = subprocess.run(
-        ['cylc', 'reinstall', test_flow_name],
-        capture_output=True
-    )
-    assert reinstall.returncode != 0
-    assert (
-        'You should not define more than one templating section'
-        in reinstall.stderr.decode()
-    )
-    # Clean up run dir:
-    if not request.session.testsfailed:
-        shutil.rmtree(get_workflow_run_dir(test_flow_name))

tests/unit/test_config_tree.py

@@ -22,6 +22,7 @@
 
 import os
 import pytest
+from pytest import param
 
 from types import SimpleNamespace
 from io import StringIO
@@ -41,7 +42,6 @@
 from metomi.rose.config import ConfigLoader
 
 
-param = pytest.param
 HOST = get_host()
 
 
@@ -534,24 +534,37 @@ def test_get_cli_opts_node(opt_confs, defines, rose_template_vars, expect):
     'old, new, expect',
     [
         # An example with only opts:
-        ('opts=a b c', 'opts=c d e', '\nopts=a b c d e\n'),
+        param(
+            'opts=a b c', 'opts=c d e', '\nopts=a b c d e\n',
+            id='only opts'
+        ),
         # An example with lots of options:
-        (
+        param(
             'opts=A B\n[env]\nFOO=Whipton\n[jinja2:suite.rc]\nBAR=Heavitree',
             'opts=B C\n[env]\nFOO=Pinhoe\n[jinja2:suite.rc]\nBAR=Broadclyst',
-            'opts=A B C\n[env]\nFOO=Pinhoe\n[jinja2:suite.rc]\nBAR=Broadclyst'
+            'opts=A B C\n[env]\nFOO=Pinhoe\n[jinja2:suite.rc]\nBAR=Broadclyst',
+            id='lots of options'
+        ),
+        # An example with updated template variables:
+        param(
+            'opts=A B\n[env]\nFOO=Whipton\n[jinja2:suite.rc]\nBAR=Ottery',
+            'opts=B C\n[env]\nFOO=Pinhoe\n[template variables]\nBAR=Whipton',
+            'opts=A B C\n[env]\nFOO=Pinhoe\n[template variables]\nBAR=Whipton',
+            id='changed template vars'
         ),
         # An example with no old opts:
-        (
+        param(
             '',
             'opts=A B\n[env]\nFOO=Whipton\n[jinja2:suite.rc]\nBAR=Heavitree',
-            'opts=A B\n[env]\nFOO=Whipton\n[jinja2:suite.rc]\nBAR=Heavitree'
+            'opts=A B\n[env]\nFOO=Whipton\n[jinja2:suite.rc]\nBAR=Heavitree',
+            id='no old options'
         ),
         # An example with no new opts:
-        (
+        param(
             'opts=A B\n[env]\nFOO=Whipton\n[jinja2:suite.rc]\nBAR=Heavitree',
             '',
-            'opts=A B\n[env]\nFOO=Whipton\n[jinja2:suite.rc]\nBAR=Heavitree'
+            'opts=A B\n[env]\nFOO=Whipton\n[jinja2:suite.rc]\nBAR=Heavitree',
+            id='no new opts'
         )
     ]
 )