-
Notifications
You must be signed in to change notification settings - Fork 93
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
Default to localhost in srv files mgr. #2344
Conversation
(The change looks good enough, but I'll need to test it to see if it really works.) |
Adding proper tests seems difficult in this case... |
Agreed. |
lib/cylc/cfgspec/globalcfg.py
Outdated
value = value.replace(os.environ['HOME'], '$HOME') | ||
elif owner != USER: | ||
# replace USER with owner for direct access via local filesys | ||
value = value.replace('/%s/' % USER, '/%s/' % owner) |
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.
This will only work if we have /home/hilary/
, /home/matt
, etc. It will not work if we have something like /home/disk1/hilary/
, /home/disk2/matt/
, etc.
os.path.expanduser
will work with strings with ~
or ~user
prefixes, but we'll still have issues if the value is not under HOME.
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.
Good point (you mean if different users have different home locations, right?).
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.
we'll still have issues if the value is not under HOME
I presume os.path.expanduser("~user")
works regardless, but do you mean if the user has configured a non-standard run directory location? (I wonder if we're assuming the standard location anywhere else at this point...)
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.
That as well.
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.
(Just to clarify.) While ~matt/
is normally /home/matt/
, it can be anything as well, e.g. /net/home/disk2/matt/
or worse.
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.
(yes, but os.path.expanduser(~user)
should return /net/home/disk2/matt
in that case, no?)
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 which case we could assume ~user/cylc-run/
rather than replacing /me/
with /you/
in the path - that would work so long as ~you
has not configured a non-standard cylc-run location.)
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.
os.path.expanduser(~user)
should work for all instances of ~user/cylc-run/
, but will not work for non-standard locations, which is probably good enough.
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.
Yep, that's the best we can do without attempt to parse other users global config files. I'll implement os.path.expanduser()
to finish off this PR...
Code looks ok so far, I am just wondering how to test it in my environment (don't think I can create extra users on here...) |
85bdd97
to
14f7893
Compare
Rebased, and now uses |
3rd-to-last commit restores command transcripts to the user guide (lost in the recent bin/cylc refactor). Should be good to go now if tests pass... |
Close #2340
Close #526
--user
specified but not--host
)