Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 16, 2016
Merged

Make cwd an abs path to avoid ambiguity #312

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ The runtime MUST mount entries in the listed order.
## Process configuration

* **`terminal`** (bool, optional) specifies whether you want a terminal attached to that process. Defaults to false.
* **`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.
Copy link
Contributor

@jonboulle jonboulle Jan 15, 2016

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

Copy link
Contributor Author

@duglin duglin Jan 15, 2016

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.

Copy link
Contributor

@mrunalp mrunalp Jan 15, 2016

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).

Copy link
Contributor Author

@duglin duglin Jan 15, 2016

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.

Copy link
Contributor

@wking wking Jan 15, 2016

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 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.

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.”

Copy link
Contributor Author

@duglin duglin Jan 15, 2016

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.

Copy link
Contributor

@wking wking Jan 15, 2016

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.

* **`env`** (array of strings, optional) contains a list of variables that will be set in the process's environment prior to execution. Elements in the array are specified as Strings in the form "KEY=value". The left hand side must consist solely of letters, digits, and underscores `_` as outlined in [IEEE Std 1003.1-2001](http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html).
* **`args`** (string, required) executable to launch and any flags as an array. The executable is the first element and must be available at the given path inside of the rootfs. If the executable path is not an absolute path then the search $PATH is interpreted to find the executable.

Expand Down