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 reinstall --dry-run #4964

Merged
merged 6 commits into from
Jul 20, 2022
Merged

cylc reinstall --dry-run #4964

merged 6 commits into from
Jul 20, 2022

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Jul 5, 2022

Closes #4960

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 oliver-sanders added this to the cylc-8.0.0 milestone Jul 5, 2022
@oliver-sanders oliver-sanders requested a review from datamel July 5, 2022 12:38
@oliver-sanders oliver-sanders self-assigned this Jul 5, 2022
@oliver-sanders oliver-sanders marked this pull request as ready for review July 5, 2022 12:38
source=Path(source),
named_run=workflow_id,
rundir=run_dir,
dry_run=False # TODO: ready for dry run implementation
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @datamel, nice future-proofing!

@oliver-sanders
Copy link
Member Author

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

@oliver-sanders oliver-sanders marked this pull request as draft July 5, 2022 16:38
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.

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 Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member Author

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
Copy link
Member Author

oliver-sanders commented Jul 6, 2022

@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?

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.

(Premature review: all good so far 👍 )

cylc/flow/scripts/clean.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member

hjoliver commented Jul 7, 2022

WDYT?

Excellent, thanks.

@hjoliver
Copy link
Member

hjoliver commented Jul 7, 2022

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

@oliver-sanders
Copy link
Member Author

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.

@oliver-sanders oliver-sanders marked this pull request as ready for review July 7, 2022 15:02
commands = [
'clean',
'hold',
'install',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is reinstall important enough to be added here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

(That change has been merged)

Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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
Copy link
Member Author

@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)

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

datamel commented Jul 18, 2022

@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.
reinstall=True,
dry_run=dry_run,
)
reinstall_log.info(cli_format(rsync_cmd))
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

hjoliver commented Jul 20, 2022

(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).

Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

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

cylc/flow/scripts/reinstall.py Show resolved Hide resolved
cylc/flow/scripts/clean.py Show resolved Hide resolved
@datamel datamel merged commit 448e792 into cylc:master Jul 20, 2022
@oliver-sanders oliver-sanders deleted the 4960 branch July 20, 2022 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could be better Not exactly a bug, but not ideal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reinstall: dry run
3 participants