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

Fix cylc clean remote re-invocation bug #5631

Merged
merged 3 commits into from
Jul 25, 2023
Merged

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Jul 18, 2023

Closes #5628

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).
  • No dependency changes
  • Tests are updated
  • Changelog entry included (as a towncrier fragment - see Use towncrier for changelog generation #5634)
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@MetRonnie MetRonnie added bug Something is wrong :( small labels Jul 18, 2023
@MetRonnie MetRonnie added this to the cylc-8.2.1 milestone Jul 18, 2023
@MetRonnie MetRonnie self-assigned this Jul 18, 2023
@MetRonnie MetRonnie marked this pull request as ready for review July 24, 2023 09:26
@MetRonnie MetRonnie requested a review from wxtim July 24, 2023 14:40
@@ -120,10 +121,17 @@ def get_option_parser():
parser.add_option(
'--timeout',
help=("The number of seconds to wait for cleaning to take place on "
"remote hosts before cancelling."),
r"remote hosts before cancelling. Default: %default."),
Copy link
Member

Choose a reason for hiding this comment

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

Nice to document the defaults, I have no idea why optparse does not do this by "default".

We are already defining our own "help formatter", looks like we could override expand_default to add a "Default: %default" bit onto the end of every option where a default is set rather than having to add it to each option manually. (for future work).

Copy link
Member

Choose a reason for hiding this comment

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

Have you turned this into a new issue?

@wxtim wxtim merged commit 9d444b2 into cylc:master Jul 25, 2023
@MetRonnie MetRonnie deleted the cylc-clean branch July 25, 2023 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cylc clean remote re-invocation fails if flow.cylc detected in sub dir
3 participants