Allow change of templating language when reinstalling #192
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
caedf2f to
111dfa2
Compare
MetRonnie
reviewed
Oct 19, 2022
Comment on lines
-255
to
-258
| 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. | ||
| """ |
There was a problem hiding this comment.
What if you replace jinja2:suite.rc with empy:suite.rc or vice versa? Is that meant to be allowed now?
There was a problem hiding this comment.
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)
- 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.
1ba4456 to
1e8b09b
Compare
MetRonnie
approved these changes
Oct 19, 2022
oliver-sanders
approved these changes
Oct 20, 2022
…guage_change_reinstall_bug
|
Will do once the tests have re-run |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes close #191
in
record_cylc_install_optionsthe functionidentify_templating_sectionis used to check whether the user has set more than one templating section. This is done after the previousrose-suite-cylc-install.confhas been merged with the new one, so that if the user has changed the templating language both will be present in the merged config and the test will always fail.Work done
Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg.