cylc reinstall --dry-run #4964

oliver-sanders · 2022-07-05T12:38:01Z

Not 8.0.0 blocking but an easy win.

Add a --dry-run option for cylc reinstall.

Requirements 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.
Appropriate tests are included (unit and/or functional).
Appropriate change log entry included.
Docs (I don't think changes are needed ATM, should improve the reinstall migration page soon)

oliver-sanders · 2022-07-05T12:38:38Z

cylc/flow/scripts/reinstall.py

        source=Path(source),
        named_run=workflow_id,
        rundir=run_dir,
-        dry_run=False  # TODO: ready for dry run implementation


Thanks @datamel, nice future-proofing!

oliver-sanders · 2022-07-05T16:38:17Z

(currently in draft whilst I add the interactive mode stuff...)

hjoliver

Quick premature review. Looks good. Thanks @oliver-sanders (and @datamel for laying the groundwork) - this will ease the pain a little!

Can we explicitly warn, in cylc reinstall command help, that it will delete any non-source-dir files installed or generated at run time, that aren't in share etc. or protected by .cylcignore.

cylc/flow/scripts/clean.py

oliver-sanders · 2022-07-06T07:56:18Z

Can we explicitly warn, in cylc reinstall command help, that it will delete any non-source-dir files installed or generated at run time, that aren't in share etc. or protected by .cylcignore.

I was just quickly adding the --dry-run option, but the interactive mode stuff should be quick to do so I'm currently working on that. Will make the reinstall command document reinstall behaviour in the process.

oliver-sanders · 2022-07-06T08:58:27Z

@hjoliver Have added interactive mode and some, hopefully helpful messages. Need to straighten out the tests with this change.

If interactive ...
Perform dry-run.
If rose-suite.conf exists, add a note to explain why Rose files are marked as deleted.
Add a note about adding files to the .cylcignore file, see --help for more info.
Prompt the user y/n

WDYT?

hjoliver

(Premature review: all good so far 👍 )

cylc/flow/scripts/clean.py

hjoliver · 2022-07-07T02:36:54Z

WDYT?

Excellent, thanks.

hjoliver · 2022-07-07T02:38:13Z

Maybe don't prompt if there are no changes at all. (And then don't bother reinstalling).

oliver-sanders · 2022-07-07T15:02:31Z

Maybe don't prompt if there are no changes at all. (And then don't bother reinstalling).

Done (interactive mode only).

I'm trying not to lean on rsync output too much as I'm not sure how stable it is between implementations so it's only a crude check but should be good enough for most cases.

datamel · 2022-07-08T11:18:54Z

cylc/flow/scripts/cylc.py

+    commands = [
+        'clean',
+        'hold',
+        'install',


Is reinstall important enough to be added here?

Fair point, subjective, I didn't add reload which was silly, if we are having reinstall we need reload too.

Hillary is making changes to this on another branch so I've reverted these changes.

(That change has been merged)

datamel

I've had a really good play around with this. One thing I have noticed is that in "--debug" mode, the rsync command is printed to the terminal but not the log. I think (if it is desirable for the command to be printed to the log, which I think it is) the log level of the log needs dropping to debug, when --debug in cli opts, and the command is currently written to LOG, which would need to be changed to reinstall_log? It looks a bit like a conscious choice so happy to be overruled if you think otherwise, I am possibly not thinking of a reason for this.
Really nice cli interface, with the tip.

oliver-sanders · 2022-07-08T12:46:00Z

The reason I added the rsync command to debug mode was to make it a little clearer what was going on under the surface for users in --dry-run mode (if they are interested).

I think it's less important for the log, but then again, why not put it in? Might be helpful later on.

oliver-sanders · 2022-07-11T15:51:49Z

@datamel Were you suggesting logging the rsync command to the reinstall log or to the scheduler log?

(The LOG.debug I added doesn't go to either ATM, only to stderr)

datamel · 2022-07-18T12:02:52Z

@datamel Were you suggesting logging the rsync command to the reinstall log or to the scheduler log?

(The LOG.debug I added doesn't go to either ATM, only to stderr)

I was suggesting the reinstall log. Just somewhere for us to be able to check the command in the case of debugging problems.

* Shorten/simplify the first line of a couple of command descriptions. * Add `cylc clean` and `cylc gui` to the list of "selected sub-commands" which are listed by default in the `cylc help` page. * Ensure there is one newline after each description (fixes spacing issue with the auto-added ID help).

* Makes the CLI --help easier to skim by making section headings more obvious.

* New behaviour: * In interactive mode: * Perform dry run. * Display proposed changes to the user. * Prompt for permission to continue. * Provide explanatory docs in the --help. * In non-interactive mode (unlikely use case for reinstall): * Just do it. * Misc changes: * Use the globalcfg configured rsync command for `cylc install` (was using hardcoded `rsync`). * Improved reinstall docs.

oliver-sanders · 2022-07-19T13:32:23Z

cylc/flow/workflow_files.py

+        reinstall=True,
+        dry_run=dry_run,
+    )
+    reinstall_log.info(cli_format(rsync_cmd))


I was suggesting the reinstall log. Just somewhere for us to be able to check the command in the case of debugging problems.

Now done here.

hjoliver · 2022-07-20T01:42:47Z

(Pushed a style fix to allow tests to pass, because I thought @datamel had approved this already ... but it seems not, so not merging just yet).

datamel

All good. Just a small change I think to make on the help docs.

cylc/flow/scripts/reinstall.py

cylc/flow/scripts/clean.py

oliver-sanders added the small label Jul 5, 2022

oliver-sanders added this to the cylc-8.0.0 milestone Jul 5, 2022

oliver-sanders requested a review from datamel July 5, 2022 12:38

oliver-sanders self-assigned this Jul 5, 2022

oliver-sanders marked this pull request as ready for review July 5, 2022 12:38

oliver-sanders commented Jul 5, 2022

View reviewed changes

oliver-sanders marked this pull request as draft July 5, 2022 16:38

hjoliver reviewed Jul 5, 2022

View reviewed changes

cylc/flow/scripts/clean.py Outdated Show resolved Hide resolved

oliver-sanders force-pushed the 4960 branch from 0ee2a5a to 8be9957 Compare July 6, 2022 08:56

hjoliver approved these changes Jul 7, 2022

View reviewed changes

cylc/flow/scripts/clean.py Outdated Show resolved Hide resolved

oliver-sanders force-pushed the 4960 branch from 8be9957 to 56a5068 Compare July 7, 2022 15:01

oliver-sanders marked this pull request as ready for review July 7, 2022 15:02

oliver-sanders removed the small label Jul 7, 2022

datamel reviewed Jul 8, 2022

View reviewed changes

oliver-sanders force-pushed the 4960 branch from e9a8e38 to 5465367 Compare July 11, 2022 15:52

oliver-sanders added the could be better Not exactly a bug, but not ideal. label Jul 14, 2022

oliver-sanders force-pushed the 4960 branch from 5465367 to d5c5239 Compare July 19, 2022 12:59

oliver-sanders added 3 commits July 19, 2022 14:29

reinstall: add --dry-run option

8fd81ab

cli: put headings in help text in bold

94e6d1b

* Makes the CLI --help easier to skim by making section headings more obvious.

oliver-sanders added 2 commits July 19, 2022 14:30

changelog

2f7a6ec

oliver-sanders force-pushed the 4960 branch from d5c5239 to 2f7a6ec Compare July 19, 2022 13:31

oliver-sanders commented Jul 19, 2022

View reviewed changes

Style fix.

1d251bf

datamel reviewed Jul 20, 2022

View reviewed changes

cylc/flow/scripts/reinstall.py Show resolved Hide resolved

cylc/flow/scripts/clean.py Show resolved Hide resolved

datamel approved these changes Jul 20, 2022

View reviewed changes

datamel merged commit 448e792 into cylc:master Jul 20, 2022

oliver-sanders deleted the 4960 branch July 20, 2022 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cylc reinstall --dry-run #4964

cylc reinstall --dry-run #4964

oliver-sanders commented Jul 5, 2022 •

edited

Loading

oliver-sanders Jul 5, 2022

oliver-sanders commented Jul 5, 2022

hjoliver left a comment •

edited

Loading

oliver-sanders commented Jul 6, 2022

oliver-sanders commented Jul 6, 2022 •

edited

Loading

hjoliver left a comment

hjoliver commented Jul 7, 2022

hjoliver commented Jul 7, 2022 •

edited

Loading

oliver-sanders commented Jul 7, 2022

datamel Jul 8, 2022

oliver-sanders Jul 8, 2022

oliver-sanders Jul 11, 2022

hjoliver Jul 19, 2022

datamel left a comment

oliver-sanders commented Jul 8, 2022

oliver-sanders commented Jul 11, 2022

datamel commented Jul 18, 2022

oliver-sanders Jul 19, 2022

hjoliver commented Jul 20, 2022 •

edited

Loading

datamel left a comment

cylc reinstall --dry-run #4964

cylc reinstall --dry-run #4964

Conversation

oliver-sanders commented Jul 5, 2022 • edited Loading

oliver-sanders Jul 5, 2022

Choose a reason for hiding this comment

oliver-sanders commented Jul 5, 2022

hjoliver left a comment • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Jul 6, 2022

oliver-sanders commented Jul 6, 2022 • edited Loading

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver commented Jul 7, 2022

hjoliver commented Jul 7, 2022 • edited Loading

oliver-sanders commented Jul 7, 2022

datamel Jul 8, 2022

Choose a reason for hiding this comment

oliver-sanders Jul 8, 2022

Choose a reason for hiding this comment

oliver-sanders Jul 11, 2022

Choose a reason for hiding this comment

hjoliver Jul 19, 2022

Choose a reason for hiding this comment

datamel left a comment

Choose a reason for hiding this comment

oliver-sanders commented Jul 8, 2022

oliver-sanders commented Jul 11, 2022

datamel commented Jul 18, 2022

oliver-sanders Jul 19, 2022

Choose a reason for hiding this comment

hjoliver commented Jul 20, 2022 • edited Loading

datamel left a comment

Choose a reason for hiding this comment

oliver-sanders commented Jul 5, 2022 •

edited

Loading

hjoliver left a comment •

edited

Loading

oliver-sanders commented Jul 6, 2022 •

edited

Loading

hjoliver commented Jul 7, 2022 •

edited

Loading

hjoliver commented Jul 20, 2022 •

edited

Loading