Skip to content

[Config | Hooks] Add "if supported by the platform" #472

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

Closed
wants to merge 1 commit into from
Closed

[Config | Hooks] Add "if supported by the platform" #472

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 @@ -245,7 +245,7 @@ Presently there are `Prestart`, `Poststart` and `Poststop`.
* [`Poststop`](#poststop) is a list of hooks to be run after the container process exits

Hooks allow one to run code before/after various lifecycle events of the container.
Hooks MUST be called in the listed order.
Hooks MUST be called in the listed order if supported by the platform.
Copy link
Contributor

@mrunalp mrunalp May 31, 2016

Choose a reason for hiding this comment

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

This is fine only if the bundle were to support different platforms else one could just not add hooks to the config at all.

Copy link
Contributor

@wking wking May 31, 2016

Choose a reason for hiding this comment

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

On Tue, May 31, 2016 at 09:33:38AM -0700, Mrunal Patel wrote:

-Hooks MUST be called in the listed order.
+Hooks MUST be called in the listed order if supported by the platform.

This is fine only if the bundle were to support different platforms
else one could just not add hooks to the config at all.

The new text here isn't clear, but based on 1, I think @RobDolinMS
means “Windows-based runtimes will error on configuration files with
meaningful content in ‘hooks’ (e.g. anything more than empty arrays
for the per-event properties)”. So for Windows this PR is aiming for
“you can't have hooks” not “you have a choice to not have hooks”.
The latter is already true, although it is made more explicit by the
in-flight #427.

Copy link
Contributor

@mrunalp mrunalp May 31, 2016

Choose a reason for hiding this comment

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

@wking If that is the intent, then we should clarify further whether the platform will error out or just not call the hooks.

Copy link
Contributor

@wking wking May 31, 2016

Choose a reason for hiding this comment

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

On Tue, May 31, 2016 at 10:34:39AM -0700, Mrunal Patel wrote:

-Hooks MUST be called in the listed order.
+Hooks MUST be called in the listed order if supported by the platform.

@wking If that is the intent, then we should clarify further whether
the platform will error out or just not call the hooks.

I'm pretty sure “just not call the hooks” should not be valid runtime
behavior. This is related to, but maybe not quite covered by, the “…
unable to create the environment specified…” language @cyphar has
in-flight with #397.

Copy link
Member

@tianon tianon Sep 8, 2016

Choose a reason for hiding this comment

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

My preference here is definitely along the same lines (same as #476) -- if the runtime cannot support the functionality, then attempting to use it ought to result in an error (which stops users from scratching their heads wondering why their hooks aren't firing, for example).

The state of the container is passed to the hooks over stdin, so the hooks could get the information they need to do their work.

Hook paths are absolute and are executed from the host's filesystem in the [runtime namespace][runtime-namespace].
Expand Down