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

Jump to bottom

play: check Cylc version on restart #5074

Merged
merged 2 commits into from
Oct 25, 2022
Merged

play: check Cylc version on restart #5074

merged 2 commits into from
Oct 25, 2022

Conversation

oliver-sanders
Copy link
Member

  • Closes handle version change on restart (resume) #4039
  • When restarting a workflow, check the version it previously ran under
    against the current version in order to determine if the change is
    sensible.
    • Restart normally if the restart would:
      • Not change the version.
      • Increase the maintenance number.
    • Prompt (override with --yes) for confirmation if the restart would:
      • Reduce the maintenance number.
      • Increase the minor number.
    • Exit with nasty error (override with --force or --yes --yes?) if
      the restart would:
      • Decrease the minor number.
      • Change the major number.
  • This also moves the pre-existing workflow DB check from post-detatch
    to pre-detach.

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.
  • 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, PRs raised to both master and the relevant maintenance branch.

@oliver-sanders oliver-sanders added this to the cylc-8.1.0 milestone Aug 16, 2022
@oliver-sanders oliver-sanders self-assigned this Aug 16, 2022
hjoliver
hjoliver requested changes Aug 17, 2022
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Prompt format should be of the form:

Question blah? [a](b/c)_
# or
Question blah [a](b/c)?_

(ends with a blank space; options in parentheses; no colon ?:)

cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved
cylc/flow/terminal.py Outdated Show resolved Hide resolved
cylc/flow/scheduler_cli.py Show resolved Hide resolved
cylc/flow/scheduler_cli.py Show resolved Hide resolved
cylc/flow/scheduler_cli.py Show resolved Hide resolved
cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved
cylc/flow/terminal.py Show resolved Hide resolved
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Aug 17, 2022 •
edited
Loading

(I failed to git add the unit tests for this, will push up tomorrow) - done

@hjoliver
Copy link
Member

hjoliver commented Aug 18, 2022 •
edited
Loading

Looks like the prompt is messing with some functional tests:

This workflow was previously run with Cylc 8.0rc1.dev0.
This is Cylc 8.1.0.dev.
Are you sure you want to upgrade from Cylc 8.0rc1.dev0 to 8.1.0.dev: y,n

(I guess we have faked DBs with specific Cylc versions recorded)

@oliver-sanders
Copy link
Member Author

Makes sense, in which case we need to have a CLI override to handle this.

  • --force Permit downgrade [implemented].
  • --yes, -y Answer "yes" to prompts (permits upgrade) [suggested].

Thoughts?

@hjoliver
Copy link
Member

--yes, -y Answer "yes" to prompts (permits upgrade) [suggested].
Thoughts?

What's the reasoning behind --force vs --yes for upgrade vs downgrade? Presumably we could have --yes handle both cases, which would be slightly more convenient than --force which requires re-running the command. But maybe I'm missing something?

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Aug 18, 2022 •
edited
Loading

But maybe I'm missing something?

Upgrading is safe (so we prompt), downgrading is dangerous (so we error without prompting). Implementing the two via the same option conflates the two and makes situations where cylc play is called in scripts awkward.

@oliver-sanders oliver-sanders force-pushed the upperism branch 2 times, most recently from f52e7b1 to a88390a Compare August 18, 2022 14:17
@hjoliver
Copy link
Member

Makes sense. In that case, I guess we do need --yes as well.

@oliver-sanders
Copy link
Member Author

Alternatively we could make --yes the default behaviour if the command is run non-interactively?

@oliver-sanders oliver-sanders marked this pull request as draft August 25, 2022 16:35
@oliver-sanders oliver-sanders marked this pull request as ready for review September 12, 2022 14:55
@oliver-sanders
Copy link
Member Author

Added --upgrade (safe'ish) and --downgrade (nasty) options.

  • In interactive mode user's are prompted for permission to upgrade.
  • Otherwise use --upgrade and --downgrade.

wxtim
wxtim requested changes Oct 5, 2022
View reviewed changes
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Checked out and tried.
  • Read code.

I've made a couple of suggestions about variable naming - I'm particularly unhappy about this and that which I find very unhelpful.

cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved
cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved
cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved
cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved
cylc/flow/terminal.py Outdated Show resolved Hide resolved
tests/unit/test_terminal.py
assert prompt('whatever', ['x'], default='x') == 'x'

# test a prompt with an input pre-process method thinggy
stdinput('YES')
Copy link
Member

Choose a reason for hiding this comment

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

You missed "What is the airspeed velocity of an unladen sparrow?"

@wxtim wxtim self-requested a review October 6, 2022 09:06
wxtim
wxtim approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

You have answered all my earlier review comments.

@oliver-sanders
play: check Cylc version on restart
c57368d 
* Closes cylc#4039
* When restarting a workflow, check the version it previously ran under
  against the current version in order to determine if the change is
  sensible.
  * Restart normally if the restart would:
    * Not change the version.
    * Increase the maintenance number.
  * Prompt (override with --yes) for confirmation if the restart would:
    * Reduce the maintenance number.
    * Increase the minor number.
  * Exit with nasty error (override with --force or --yes --yes?) if
    the restart would:
    * Decrease the minor number.
    * Change the major number.
* This also moves the pre-existing workflow DB check from post-detatch
  to pre-detach.
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Oct 20, 2022 •
edited
Loading

The DB upgrader test is now broken due to the upgrading being performed in the cylc play script rather than via the Scheduler object. Needs resolving.

Done.

Also filled in some coverage holes.

@oliver-sanders
play: --upgrade and --downgrade options
f7f6144 
* Add upgrade/downgrade options for non-interactive purposes
hjoliver
hjoliver approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

👍

@hjoliver hjoliver merged commit b0ff549 into cylc:master Oct 25, 2022
@oliver-sanders oliver-sanders deleted the upperism branch October 26, 2022 08:50
@MetRonnie MetRonnie mentioned this pull request Nov 11, 2022
Merged
8 tasks
@MetRonnie MetRonnie mentioned this pull request Jul 30, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxtim wxtim wxtim approved these changes

@hjoliver hjoliver hjoliver approved these changes

Assignees

@oliver-sanders oliver-sanders

Labels
None yet
Projects
None yet
Milestone
cylc-8.1.0
Development

Successfully merging this pull request may close these issues.

handle version change on restart (resume)
3 participants
@oliver-sanders @hjoliver @wxtim