pkg/config: lookup InitPath in HelperBinariesDir #1698
Conversation
| @@ -540,11 +542,6 @@ func (c *Config) Env() []string { | |||
| return c.Containers.Env | |||
| } | |||
|
|
|||
| // InitPath returns location where init program added to containers when users specify the --init flag. | |||
There was a problem hiding this comment.
This would cause a breaking change, why not still return the init-path?
There was a problem hiding this comment.
Because this is a broken API, nobody should call that, the correct logic is in FindInintBinary(). Returning a single path just causes confusion for users who think this function does return something correct.
We do not guarantee a stable API in c/common so I can just nuke it here and update the only caller in podman.
docs/containers.conf.5.md
Outdated
| @@ -205,6 +205,8 @@ Run an init inside the container that forwards signals and reaps processes. | |||
|
|
|||
| **init_path**="/usr/libexec/podman/catatonit" | |||
|
|
|||
| Deprecated: It is recommend to place catatonit in a directory listed under the **helper_binaries_dir** option. | |||
There was a problem hiding this comment.
What does deprecation mean in this context? Is it a NOP from now on or will it still be used if set? (Didn't read the Podman/full PR yet but I think users may have the same question)
There was a problem hiding this comment.
It still works fine, same for the other undocumented field under the engine section, see FindInintBinary().
With deprecated I mean that users should instead of setting this field place it under one of the helper binaries directories instead. This field will continue to work and I have no intention of removing it as this will break backwards compat.
There was a problem hiding this comment.
Could we rephrase "deprecated" to "We recommend using $the_other_thing instead"? I feel that "deprecated" raises more questions than answers in this specific case.
There was a problem hiding this comment.
I prefer to remove deprecated stuff from the man page totally, If the user has a containers.conf with this field, it will be used but we don't need to menetion it in man page or containers.conf.
There was a problem hiding this comment.
Sure, I went with deprecated because that leaves the door open to remove it at some point. But I guess both fields are simple enough to keep them supported even for future major versions.
Forcing a single upstream default for the init path is bad as some distro use different install locations for various reasons. To fix this use the existing helper_binaries_dir field to lookup in all directories. To keep backwards compatibility we keep using the old default and both Containers.InitPath and Engine.InitPath. Yes that is right, somehow we ended up with the same config field under the containers and engine section and they are both used in podman! Thus we need to keep supporting both, only the field under the container section was documented and now recommends the use of helper_binaries_dir. To make the docs more clear also document what binaries are currently looked up in helper_binaries_dir. Note this needs further integration in podman. Fixes containers#1110 Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I would have removed the description in containers.conf, but this is fine. /lgtm |
Forcing a single upstream default for the init path is bad as some distro use different install locations for various reasons.
To fix this use the existing helper_binaries_dir field to lookup in all directories. To keep backwards compatibility we keep using the old default and both Containers.InitPath and Engine.InitPath. Yes that is right, somehow we ended up with the same config field under the containers and engine section and they are both used in podman! Thus we need to keep supporting both, only the field under the container section was documented and it will now be marked as deprecated.
To make the docs more clear also document what binaries are currently looked up in helper_binaries_dir.
Note this needs further integration in podman.
Fixes #1110