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 unregister: skip suites with port files #1802

Merged
merged 1 commit into from
Apr 21, 2016

Conversation

matthewrmshin
Copy link
Contributor

Also ensure that cylc unregister bar does not unregister a suite
called foobar.

Close #1796.

@matthewrmshin matthewrmshin self-assigned this Apr 19, 2016
@matthewrmshin matthewrmshin added the bug Something is wrong :( label Apr 19, 2016
@matthewrmshin matthewrmshin added this to the next-release milestone Apr 19, 2016
@matthewrmshin
Copy link
Contributor Author

@hjoliver @arjclark please review.

@hjoliver
Copy link
Member

Branch needs updating.

try:
os.rmdir(tmp)
except OSError:
break
else:
Copy link
Member

Choose a reason for hiding this comment

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

This block (in master too) is a bit too simplistic to do what it's supposed to - i.e. remove all empty directories left by unregistering and deleting a bunch of suites. To see this, try cylc import examples $TMPDIR then cylc unreg -d cylc-.* and look in $TMPDIR. However, I doubt that anyone stores suites in directory trees that reflect the suite name hierarchy as I originally intended (e.g. nwp.test1 and nwp.test2 stored in nwp/test1/ and nwp/test2) - certainly Rose users don't - and unregisters them en-masse ... so maybe we should just remove this feature.

Also ensure that `cylc unregister bar` does not unregister a suite
called `foobar`.
@matthewrmshin
Copy link
Contributor Author

All comments addressed. Branch re-based.

@hjoliver
Copy link
Member

Review 1 - good, test battery passing.

@hjoliver hjoliver assigned arjclark and unassigned hjoliver Apr 20, 2016
@arjclark
Copy link
Contributor

Looks OK to me. No problems from the test-battery in my environment.

@arjclark arjclark merged commit b6a7886 into cylc:master Apr 21, 2016
@matthewrmshin matthewrmshin deleted the fix-unregister branch April 21, 2016 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants