-
Notifications
You must be signed in to change notification settings - Fork 593
Make cwd an abs path to avoid ambiguity #312
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
Conversation
Signed-off-by: Doug Davis <[email protected]>
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.
might want to consider behaviour if that path doesn't exist
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.
hmm good point - I'll try to cover that in my change to the definition of the ops (in particular 'start') since that's when it matters.
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.
Hmm, not sure if we should cover details for every misconfigured property. Does a catch all like "runtime will return an error if the config is invalid for properties that couldn't be validated statically" work? (Not here, but maybe in ops as @duglin suggested).
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 didn't view this as a misconfiguraton issue - per se. I viewed @jonboulle's comment as:
1 - make sure we say that during a start the process must be run from the cwd (which I forgot to mention in my ops PR)
2 - make sure impls generate an error instead of creating the missing cwd - which some may think is a valid thing to do. Both are valid options but we need consistency and in this case I think erroring out is the better choice.
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.
On Fri, Jan 15, 2016 at 10:00:44AM -0800, Doug Davis wrote:
I didn't view this as a misconfiguraton issue - per se. I viewed
@jonboulle's comment as:
1 - make sure we say that during astartthe process must be run
from the cwd (which I forgot to mention in my ops PR)
2 - make sure impls generate an error instead of creating the
missing cwd - which some may think is a valid thing to do. Both
are valid options but we need consistency and in this case I
think erroring out is the better choice.
These both sound valid to me, but seem orthogonal to whether cwd is
absolute/relative or required/optional.
For (2), I'd just say “If the cwd doesn't exist, the runtime MUST log
an error and jump to the cleanup step ($LIFECYLE_LINK). The runtime
MUST NOT create the missing directory.”
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.
Whether cwd is required or not is a different issue/PR so its out of scope of this PR.
Given its required, I think abs path is the only valid choice - relative (w/o defining the starting point, which is what making cwd required was trying to avoid) makes no sense from an interop perspective.
The error stuff (in 2) is for the ops PR I'm still tweaking.
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.
On Fri, Jan 15, 2016 at 10:26:51AM -0800, Doug Davis wrote:
-*
cwd(string, required) is the working directory that will be set for the executable.
+*cwd(string, required) is the working directory that will be set for the executable. This value MUST be an absolute path.Whether cwd is required or not is a different issue/PR so its out of scope of this PR.
Given its required, I think abs path is the only valid choice…
I agree on both points. But since #307 is only two days old (from
proposal to merge), I thought I'd give folks another chance to revisit
before too much time was sunk in implementation details (like this PR)
for a feature I don't think we need (#307). If #307 is already locked
in, then yeah, I think this PR is the only sensible way forward.
|
This seems like a bunch of spec/runtime hoop jumping to force config |
|
@wking I see your point and agree about being consistent one way or the other. |
|
LGTM |
1 similar comment
|
LGTM |
Make cwd an abs path to avoid ambiguity
Signed-off-by: Doug Davis [email protected]