Clarify the meaning of hook elements #255
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -66,6 +66,7 @@ If a hook returns a non-zero exit code, then an error is logged and the remainin | |||
|
|
|||
| `path` is required for a hook. | |||
| `args` and `env` are optional. | |||
| The meanings are the same as `Path`, `Args` and `Env` in [golang Cmd](https://golang.org/pkg/os/exec/#Cmd). | |||
Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>
hqhq
force-pushed
the
hq_clarify_hooks
branch
from
5ecf6e9 to
48049d2
Compare
|
@mrunalp Updated, thanks. |
|
On Tue, Nov 24, 2015 at 03:13:14AM -0800, Qiang Huang wrote:
Previous discussion in #118 and 1. My current preference is for |
|
We keep flip-flopping on this one and it is a total bikeshed. I don't think it matters at the end of the day but are there real systems out there where people specify the arg0 and don't use a symlink like /bin/busybox -> /bin/ls?! |
|
@wking Thanks for your links. After briefly reading #118 and opencontainers/runc#160 , I think using the Go style is intended. I'm OK with Go style or Python style, to be honest, Python style seems a bit more reasonable but I prefer using Go style, just don't want to mix too many rules, and Go style works fine. @philips Since this PR didn't change any semantics but clarify it, I don't think we are flip-flopping here. I don't know why specs gave examples that way, but that really cause confusing as opencontainers/runc#413. |
|
On Wed, Nov 25, 2015 at 05:47:49AM -0800, Brandon Philips wrote:
Of the potential APIs listed in 1, I think (c) is unambiguously a. ‘path’ is required, and ‘[path] + args…’ is the executed argv This provides no way to override ‘argv[0]’. It's where b. ‘path’ is required, and ‘args…’ is the executed argv. If ‘args’ This is where opencontainers/runc#160 ended up, and it's what Go c. ‘path’ is optional, ‘args…’ is the executed argv, and ‘path’ takes This is how Python does it 4, and its lovely. You only need to d. ‘args…’ is thet executed argv, and ‘args[0]’ is used when searching This is like (c), except without the ability to override ‘argv[0]’. I haven't heard any pushback against (c) (other than a “variadic” |
|
Since this is just clarifying the way it works currently, it LGTM. |
|
LGTM |
vbatts
added a commit
that referenced
this pull request
Dec 9, 2015
Clarify the meaning of hook elements
wking
added a commit
to wking/ccon
that referenced
this pull request
Dec 17, 2015
ccon doesn't believe in container IDs, so this still needs some work before it will feed OCI-specified state JSON into the hook's stdin. The hook syntax (with argv[0] in args) doesn't quite match the v0.1.0 docs [1,2], but it does match the phrasing from opencontainers/runtime-spec#255 which was claimed to "clarify, but not change" the semantics [3,4]. I think that the v0.1.0 phrasing wasn't clear enough to decide if #255 was a change or not, but since it wasn't clear, there's no more harm in just going with Qiang and Mrunal's interpretation. [1]: https://github.com/opencontainers/specs/blob/v0.1.1/runtime.md#post-stop [2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/luaaorsya10 Subject: Explicit args[0] for hooks? Date: Sat, 15 Aug 2015 12:15:23 -0700 Message-ID: <20150815191523.GK21585@odin.tremily.us> [3]: opencontainers/runtime-spec#255 (comment) [4]: opencontainers/runtime-spec#255 (comment)
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
May 20, 2016
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of punting to platforms with POSIX links for POSIX systems [3].
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
May 20, 2016
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of punting to platforms with POSIX links for POSIX systems [3].
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
May 20, 2016
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of punting to platforms with POSIX links for POSIX systems [3].
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
May 20, 2016
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of punting to platforms with POSIX links [3]. Rob Dolin had
suggested "platform-appropriate" wording [4], but it seems like Visual
Studio 2015 supports execv [5], and providing an explicit
"platform-appropriate" wiggle seems like it's adding useless
complication.
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-52
[4]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
[5]: https://msdn.microsoft.com/en-us/library/886kc0as.aspx
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
May 20, 2016
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of POSIX links [3]. Rob Dolin had suggested
"platform-appropriate" wording [4], but it seems like Visual Studio
2015 supports execv [5], and providing an explicit
"platform-appropriate" wiggle seems like it's adding useless
complication.
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-52
[4]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
[5]: https://msdn.microsoft.com/en-us/library/886kc0as.aspx
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
Aug 26, 2016
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of POSIX links [3]. Rob Dolin had suggested
"platform-appropriate" wording [4], but it seems like Visual Studio
2015 supports execv [5], and providing an explicit
"platform-appropriate" wiggle seems like it's adding useless
complication.
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-52
[4]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
[5]: https://msdn.microsoft.com/en-us/library/886kc0as.aspx
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
Aug 30, 2016
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of POSIX links [3]. Rob Dolin had suggested
"platform-appropriate" wording [4], but it seems like Visual Studio
2015 supports execv [5], and providing an explicit
"platform-appropriate" wiggle seems like it's adding useless
complication.
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-52
[4]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
[5]: https://msdn.microsoft.com/en-us/library/886kc0as.aspx
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
Sep 21, 2016
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of POSIX links [3]. Rob Dolin had suggested
"platform-appropriate" wording [4], but it seems like Visual Studio
2015 supports execv [5], and providing an explicit
"platform-appropriate" wiggle seems like it's adding useless
complication.
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-52
[4]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
[5]: https://msdn.microsoft.com/en-us/library/886kc0as.aspx
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
Oct 15, 2016
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of POSIX links [3]. Rob Dolin had suggested
"platform-appropriate" wording [4], but it seems like Visual Studio
2015 supports execv [5], and providing an explicit
"platform-appropriate" wiggle seems like it's adding useless
complication.
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-52
[4]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
[5]: https://msdn.microsoft.com/en-us/library/886kc0as.aspx
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
Jan 4, 2017
And make it omitempty, otherwise:
$ ocitools generate --template <(echo '{}')
$ cat config.json | jq -S .
{
"hooks": {},
...
}
To provide space for the type information and 'optional', I've
shuffled the hook docs to follow our usual:
* **`{property}`** ({type}, {when-needed}) {notes}
format. I've kept the separate event-trigger sections (e.g. "###
Prestart") since they go into more detail on the timing, purpose, and
exit handling for the different events (and that seemed like too much
information to put into the nested lists).
I've replaced the Go reference from 48049d2 (Clarify the semantics of
hook elements, 2015-11-25, opencontainers#255) with POSIX references (following the
new process docs) to address pushback against referencing Go [1,2] in
favor of POSIX links [3]. Rob Dolin had suggested
"platform-appropriate" wording [4], but it seems like Visual Studio
2015 supports execv [5], and providing an explicit
"platform-appropriate" wiggle seems like it's adding useless
complication.
[1]: opencontainers#427 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-46
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-52
[4]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-18-17.01.log.html#l-54
[5]: https://msdn.microsoft.com/en-us/library/886kc0as.aspx
Signed-off-by: W. Trevor King <wking@tremily.us>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm not sure if this is the intention here, but I think it'll be better
to keep consistency with golang packages.
Signed-off-by: Qiang Huang h.huangqiang@huawei.com