Support setting image_volume_mode in containers.conf #1037
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
| @@ -487,6 +487,16 @@ Default transport method for pulling and pushing images. | |||
| Maximum number of image layers to be copied (pulled/pushed) simultaneously. | |||
| Not setting this field will fall back to containers/image defaults. (6) | |||
|
|
|||
| **image_volume_mode**="bind" | |||
There was a problem hiding this comment.
I think that belongs into the [containers] table.
There was a problem hiding this comment.
No it was added to the [engine] table elsewhere.
There was a problem hiding this comment.
Why is it part of [engine]? To me it feels more on the [containers] side of things as it controls internals of a container but not of the engine.
There was a problem hiding this comment.
I hate that there is a difference, since I am debating all the time myself. Here is how the man page describes them.
## CONTAINERS TABLE
The containers table contains settings to configure and manage the OCI runtime.
# ENGINE TABLE
The `engine` table contains configuration options used to set up container engines such as Podman and Buildah.
Since this option is done by the engine and not the OCI runtime, I think it belongs in the engine.
There was a problem hiding this comment.
I agree, it's really hard to classify. I am OK with keeping it in the engine table
docs/containers.conf.5.md
Outdated
| **image_volume_mode**="bind" | ||
|
|
||
| Tells container engines how to handle the builtin image volumes. | ||
| • bind: An anonymous named volume will be created and mounted |
There was a problem hiding this comment.
The extra whitespace here looks weird. Which of these values is the default?
There was a problem hiding this comment.
image_volume_mode="bind"
Shows the default is "bind".
There was a problem hiding this comment.
@nalind Check it out now, it looks like it renders correctly in markdown and as a man page.
|
LGTM other than @nalind's comment |
rhatdan
force-pushed
the
volumes
branch
4 times, most recently
from
b34f5a4 to
0859235
Compare
There was a problem hiding this comment.
pkg/config.Config has a good number of methods that just return the contents of a field in the structure that's already public, and a number of fields for which there is no corresponding method, so I don't know if this new field is supposed to have one added for it as well.
Otherwise it's mostly nits.
| @@ -46,6 +47,8 @@ const ( | |||
| BoltDBStateStore RuntimeStateStore = iota | |||
| ) | |||
|
|
|||
| var validImageVolumeModes = []string{"bind", "tmpfs", "ignore"} | |||
There was a problem hiding this comment.
Values like CgroupfsCgroupsManager and SystemdCgroupsManager are named constants. Why the inconsistency?
pkg/config/default.go
Outdated
| func (c *Config) Env() []string { | ||
| return c.Containers.Env | ||
| } | ||
|
|
||
| // InitPath returns the default init path to add to containers | ||
| // InitPath returns the default init path to add to containers. |
There was a problem hiding this comment.
I have no idea what this comment is saying.
Begins to fix containers/podman#14230 Signed-off-by: Daniel J Walsh <[email protected]>
rhatdan
force-pushed
the
volumes
branch
2 times, most recently
from
9f4d0cb to
d34ec66
Compare
Signed-off-by: Daniel J Walsh <[email protected]>
Begins to fix containers/podman#14230
Signed-off-by: Daniel J Walsh [email protected]