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.

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

VR: Check workflow stopped if updating template vars from plugin #5966

Closed
wxtim opened this issue Feb 12, 2024 · 2 comments
Closed

VR: Check workflow stopped if updating template vars from plugin #5966

wxtim opened this issue Feb 12, 2024 · 2 comments

Comments

@wxtim
Copy link
Member

wxtim commented Feb 12, 2024

Ref: https://cylc.discourse.group/t/jinja2-variable-differences-in-config-files-new-ones-are-not-being-respected/885
Ref: #5297

Description

VR check whether template variables have been changed using the CLI and raises an error if the workflow is running.

If you change the template variables using a plugin, however, this check is not carried out.

The check is currently carried out here:

def check_tvars_and_workflow_stopped(

Reproducible Example

#!jinja2
# flow.cylc
[scheduler]
    allow implicit tasks = True
[scheduling]
    initial cycle point = 1000
    [[graph]]
        R1 = {{task}} => bar
cylc vip workflow --pause -S 'task="foo"'
cylc vr workflow --pause -S 'task="bar"'

# Witnesss that change hasn't happend after the reload.
grep "R1 =" ~/cylc-run/basic.stone/runN/log/config/*

cylc stop workflow

cylc vr workflow --pause -S 'task="bar"'

# OK now
grep "R1 =" ~/cylc-run/basic.stone/runN/log/config/*
@wxtim wxtim added the bug Something is wrong :( label Feb 12, 2024
@wxtim
Copy link
Member Author

wxtim commented Feb 13, 2024

I'm actaully not sure that this is correct. Re-install takes -S but not -s.

The real question is whether it all behaves as expected and there are no nasty surprises for the user. I'm going to develop a couple of tests which will attempt to quantify what is going on.

@oliver-sanders
Copy link
Member

oliver-sanders commented Feb 14, 2024

I'm actaully not sure that this is correct. Re-install takes -S but not -s.

Yes, reload doesn't support -s and we should check for that, but -S is a reinstall feature so cylc vr should support it fine.


The -S option applies to (re)install, so you should be able to use it for both cases.

I.E. I think this is a bug not a lack of CLI validation. So closing this as superseded by #5968

Please reopen if I've got the wrong end of the stick.

@oliver-sanders oliver-sanders closed this as not planned Won't fix, can't repro, duplicate, stale Feb 14, 2024
@oliver-sanders oliver-sanders removed the bug Something is wrong :( label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants