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 reg tweak. #2816

Merged
merged 3 commits into from Oct 29, 2018
Merged

Cylc reg tweak. #2816

merged 3 commits into from Oct 29, 2018

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Oct 24, 2018

Follow-up to #2759 (sorry) to disallow a confusing edge case.

On master, cylc reg NAME (i.e. PATH arg omitted) does nothing if NAME is already registered to another suite, but it registers a suite in $PWD as NAME [if it isn't already registered].

The former case is not correct. If deriving suite path or name from current directory path, it doesn't make sense to simply "re-register" an existing suite in a different location.

So now, on this branch cylc reg NAME always "tries to" point NAME at $PWD/suite.rc. By "tries to" I mean does so if NAME is not already used, otherwise abort unless --redirect is used (because overwriting an existing run directory is potentially dangerous). This is consistent with cylc reg NAME PATH always trying to register $PWD/suite.rc as NAME.

In fixing this I tweaked the registration function logic again, to make it easier to follow.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Tried master branch first.

kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ cylc reg BRUNO ./etc/examples/tutorial/cycling/two/
REGISTERED BRUNO -> /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/two

kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ ls -la ~/cylc-run/BRUNO/.service/
total 12
drwxrwxr-x 2 kinow kinow 4096 Oct 25 14:09 .
drwxrwxr-x 3 kinow kinow 4096 Oct 25 14:09 ..
lrwxrwxrwx 1 kinow kinow   79 Oct 25 14:09 source -> /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/two

# now re-register without passing the location

kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ cylc reg BRUNO
REGISTERED BRUNO -> /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/two

# it said it registered, but actually there were no changes...

Which was indeed weird. Then tried the change in this pull request.

# Better error handling here, as this is not a suite.rc folder
kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ cylc reg BRUNO
ERROR: no suite.rc in /home/kinow/Development/python/workspace/cylc

# and prevents registering the same name with a different location by accident
kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ cylc reg BRUNO ./etc/examples/tutorial/cycling/five/
ERROR: the name 'BRUNO' already points to /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/two.
Use --redirect to re-use an existing name and run directory.

# of course same name and location is OK
kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ cylc reg BRUNO ./etc/examples/tutorial/cycling/two/
REGISTERED BRUNO -> /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/two

# and clever enough to recognize relative paths! Ha!
kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ cylc reg BRUNO ./etc/examples/tutorial/cycling/two/../five
ERROR: the name 'BRUNO' already points to /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/two.
Use --redirect to re-use an existing name and run directory.

# redirect works OK too
kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ cylc reg BRUNO ./etc/examples/tutorial/cycling/five --redirect
WARNING: the name 'BRUNO' points to /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/two.
It will now be redirected to /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/five.
Files in the existing BRUNO run directory will be overwritten.
REGISTERED BRUNO -> /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/five

kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ ls -lah ~/cylc-run/BRUNO/.service/
total 12K
drwxrwxr-x 2 kinow kinow 4.0K Oct 25 14:12 .
drwxrwxr-x 3 kinow kinow 4.0K Oct 25 14:09 ..
lrwxrwxrwx 1 kinow kinow   80 Oct 25 14:12 source -> /home/kinow/Development/python/workspace/cylc/etc/examples/tutorial/cycling/five

Change looks good, found no issues in manual tests. Reviewing the code also found nothing out of ordinary.

👍 👏 LGTM

@hjoliver hjoliver self-assigned this Oct 25, 2018
@hjoliver hjoliver added this to the next release milestone Oct 25, 2018
@hjoliver
Copy link
Member Author

hjoliver commented Oct 25, 2018

(Updated description above to be clearer).

@hjoliver
Copy link
Member Author

hjoliver commented Oct 25, 2018

(BTW our implicit registration behaviour is doing my head in because the semantics have to be subtly different for cylc register REG and cylc run REG (the former may have a second PATH arg, the latter never does, which has implications... it's quite hard to get all the different combinations of what is possible right, without allowing things that shouldn't be possible). We should probably have been more restrictive: always require a path with the register command, and always require a registered name - no implicit reg - with the run command).

@matthewrmshin
Copy link
Contributor

See also #2818.

@hjoliver
Copy link
Member Author

Two approvals, all tests pass ... merging.

@hjoliver hjoliver merged commit cf6db61 into cylc:master Oct 29, 2018
@hjoliver hjoliver deleted the cylc-reg-tweak branch October 29, 2018 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants