Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Cylc Clean Support #323
Add Cylc Clean Support #323
Changes from 35 commits
b5b9425
5fbcfd1
cca15e3
670fcd1
17b2b0b
9cb564d
4023a37
5baddf2
185edf0
60355d0
bdbc5b2
cc7ccb6
1accec8
ff85529
b2185cd
9138aaa
3ecb249
ca399dc
9242db1
3a8ab0b
01c5868
31d9097
6696835
deee2a5
11c86ef
6733cab
69332cf
00c39ec
cf7a76b
113183c
f050ab6
1145ede
5ae1703
875f2a8
3299e00
aca76f9
d610994
2c3aafd
b698883
f1d3a6c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor point but this defaults to 120 in cylc flow. Wonder if it would be worth making them consistent, for documentation purposes mainly.
I think 600 may seem like the better timeout than 120, since nfs and file deletions can perhaps take a while?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the CLI 120 is the default, but the user can over-ride it.
In the UI I haven't exposed it to the user, so I think it's reasonable to set it longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to expose it to the user? (Happy to leave this as is for now, but perhaps there should be a question issue for how long the timeouts should be on the CLI vs UI and whether to expose it to user on the UI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember the reason, but I think @oliver-sanders and I agreed not to - I think it ought to be documented for discussion...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it out with
rm
and it failed. This should be a list of strings rather than a single string, I thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - according to the documentation it can be a colon-separated list of strings. I've added something to deal.