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

clean the options with information for cylc rose after dumping thier … #308

Closed
wants to merge 2 commits into from

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Apr 8, 2024

…content in rose-suite-cylc-install.conf

Partner of cylc/cylc-flow#6060

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

Comment on lines +360 to +374
def test_CLI_opts_cleared(monkeypatch):
"""The post install entry point should clear the three rose opts"""
path = 'cylc.rose.entry_points.'
monkeypatch.setattr(
f'{path}record_cylc_install_options', lambda *_, **__: False)
monkeypatch.setattr(f'{path}copy_config_file', lambda *_, **__: False)
monkeypatch.setattr(f'{path}rose_config_exists', lambda *_, **__: True)

opts = SimpleNamespace(
rose_template_vars=['gee', 'whiz'],
defines=['foo', 'bar'],
opt_conf_keys=['thomas', 'percy'])

post_install('', '', opts)
assert any(opts.__dict__.values()) is False
Copy link
Member

Choose a reason for hiding this comment

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

For a functional test, how about something installs and then reinstalls a workflow using the same opts object e.g:

def test_something(cylc_install_cl):
    opts = SimpleNamespace(
        rose_template_vars=['gee', 'whiz'],
        defines=['foo', 'bar'],
        opt_conf_keys=['thomas', 'percy']
    )

    # install
    run_dir = cylc_install_cli(..., opts)

    # check installed rose config
    RoseConfigTree(run_dir).get(('template variables', 'gee')) == 'whatever'

    # reinstall using same opts object (just like cylc-flow does)
    run_dir = cylc_install_cli(..., opts)

     # check the installed rose config again
    RoseConfigTree(run_dir).get(('template variables', 'gee')) == 'whatever'

Copy link
Member Author

@wxtim wxtim Apr 9, 2024

Choose a reason for hiding this comment

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

I don't actually think that this change makes sense, since the original bug surfaces in cylc play (which never runs the post install plugin) and not just cylc vip. I think that these two PR's may both need to be scrapped and the 8.2.x solution used after all (cylc/cylc-flow#6059).

@wxtim wxtim closed this Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants