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

cylc play: add suggestion for --upgrade in non-interactive terminal #5535

Merged
merged 3 commits into from
May 18, 2023

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 12, 2023

Companion to cylc/cylc-uiserver#455

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). One line message for Cylc GUI to pick up.
  • CHANGES.md entry included if this is a change that can affect users Covered by CHANGES.md in Handle trying to play workflows which require upgrade cylc-uiserver#455
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX. N/A
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@MetRonnie MetRonnie added this to the cylc-8.1.5 milestone May 12, 2023
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from oliver-sanders May 15, 2023 09:15
@@ -523,6 +523,7 @@ def _version_check(
process=str.lower,
)
# we are in non-interactive mode, abort abort abort
print('Use "--upgrade" to upgrade the workflow.', file=sys.stderr)
Copy link
Member

@MetRonnie MetRonnie May 15, 2023

Choose a reason for hiding this comment

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

There is a bit of a mismatch printing this to stderr when just above

This workflow was previously run with <yellow>{last_run_version}</yellow>.
This version of Cylc is <green>{__version__}</green>.

was printed to stdout.

Would there be anything wrong with printing that earlier message to stderr also? Would solve this problem #5381 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

@oliver-sanders - Any reason not to print the earlier msg to stderr?

Copy link
Member

Choose a reason for hiding this comment

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

There is a bit of a mismatch printing this to stderr when just above

Eh, not really, those messages are informational, this message conveys an actual error.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately the GUI and TUI are only regurgitating stderr. We should find a way for them to include the useful info about which versions are flying about IMHO

Copy link
Member

Choose a reason for hiding this comment

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

Fair point, but a widespread issue not related to this PR.

I don't think stdout/err are a great interface for Tui/GUI, but short of a Python API (that allows Cylc version changing) I can't think of a better option that isn't tons of work.

In this case, the version compat issue should also be handled better in at the UI end as we intend to implement upgrade logic there.

Copy link
Member

Choose a reason for hiding this comment

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

Moved to #5554

@MetRonnie MetRonnie changed the title Enable upgrade cylc play: add suggestion for --upgrade in non-interactive terminal May 15, 2023
@oliver-sanders oliver-sanders merged commit 31b6916 into cylc:8.1.x May 18, 2023
@wxtim wxtim deleted the enable_upgrade branch May 18, 2023 12:09
wxtim added a commit to wxtim/cylc that referenced this pull request May 19, 2023
…github.com:wxtim/cylc into fix.sort_lint_listing--correct_number_for_line_len

* 'fix.sort_lint_listing--correct_number_for_line_len' of github.com:wxtim/cylc:
  `cylc play`: add suggestion for `--upgrade` in non-interactive terminal (cylc#5535)
  centralize number used for line length check.
  Respond to review
  Fix cylc lint commented-out Jinja2 bug
  Logging: say command actioned instead of succeeded
  cylc lint non zero code from warnings (cylc#5546)
  Fix S011 lint (cylc#5536)
@hjoliver hjoliver modified the milestones: cylc-8.1.5, cylc-8.2.0 Jun 12, 2023
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

Successfully merging this pull request may close these issues.

4 participants