cylc-flow icon indicating copy to clipboard operation
cylc-flow copied to clipboard

Cylc VR: Validate should not change options

Open wxtim opened this issue 1 year ago • 2 comments

Closes #6262

Validate may alter the content of options objects passed to it: In this case by replacing icp = now with icp = <actual time> which is an invalid restart option.

Fix

Pass validate a copy of the options object to prevent changes made to the options by validate affecting subsequent steps.

Whilst in principle we probably shouldn't let the options object be mutated by the cylc validate this shouldn't affect us anywhere except in cylc vr.

Check List

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

wxtim avatar Aug 22 '24 09:08 wxtim

Partially closes https://github.com/cylc/cylc-flow/issues/6262 (we should check for other places where the options object might be changed unwisely.)

Please remove the "partially" or create a follow-on issue to cover this.

oliver-sanders avatar Aug 27 '24 15:08 oliver-sanders

MacOS failures caused by different functional tests being introduced into the chunk due to the added functional test. Addressed by https://github.com/wxtim/cylc/pull/63

MetRonnie avatar Aug 29 '24 11:08 MetRonnie

MacOS failures caused by different functional tests being introduced into the chunk due to the added functional test. Addressed by wxtim#63

Still awaiting

MetRonnie avatar Sep 23 '24 16:09 MetRonnie