Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow change of templating language when reinstalling #192

Merged
merged 3 commits into from
Oct 31, 2022
Merged

Allow change of templating language when reinstalling #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e8b09b
Allow change of templating language when reinstalling
wxtim Oct 18, 2022
08db958
re-add check re
wxtim Oct 31, 2022
e9e7086
Merge branch '1.1.x' into 20221018T0943--1.1.x--template_variable_lan…
wxtim Oct 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
6 changes: 5 additions & 1 deletion cylc/rose/entry_points.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
)
Expand All @@ -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:
Expand All @@ -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
Expand Down
18 changes: 17 additions & 1 deletion cylc/rose/utilities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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.
Expand All @@ -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}'
Expand All @@ -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


Expand Down
35 changes: 0 additions & 35 deletions tests/functional/test_reinstall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
"""
Comment on lines -255 to -258
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you replace jinja2:suite.rc with empy:suite.rc or vice versa? Is that meant to be allowed now?

Copy link
Member Author

@wxtim wxtim Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not recommended since both are deprecated. You will get a deprecation warning (from elsewhere in the code). However, I don't think that we should actively prevent people doing this - I think it's a legit, if strange, thing to do.

It might be reasonable to prevent people changing template variables to either, but I hadn't done that, because it was a side effect of the original feature not the intention.

(It'll also mess your workflow up without sig re-writing, but that's the user's problem IMO)

(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))
29 changes: 21 additions & 8 deletions tests/unit/test_config_tree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import os
import pytest
from pytest import param

from types import SimpleNamespace
from io import StringIO
Expand All @@ -41,7 +42,6 @@
from metomi.rose.config import ConfigLoader


param = pytest.param
HOST = get_host()


Expand Down Expand Up @@ -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'
)
]
)
Expand Down