pkg/config: add CONTAINERS_CONF_OVERRIDE #1378
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
Accompanying Podman PR: containers/podman#17895 |
|
@edsantiago @rhatdan @mheon @baude this is how imagine it to look like. It will ultimately solve the shortcomings in testing and should also simplify gating tests which can now quite easily test the various of flavors of network and database backends etc. |
|
Bikeshed, CONTAINERS_CONF_OVERRIDE environment name seems to match what CONTAINERS_CONF currently does. |
I first had _ADD then I had _EXTRA but in the end I gravitated toward _OVERRIDE as I found it to best describe what it does: it overrides any other config, whether it's a system or user config or even CONTAINERS_CONF. But I am not married to _OVERRIDE. |
|
I am fine with sticking with _OVERRIDE, since this is mainly for testing and is well documented. |
vrothberg
force-pushed
the
containers_conf_extra
branch
2 times, most recently
from
b44e9c6 to
0df99c5
Compare
| defer os.Unsetenv("CONTAINERS_CONF_OVERRIDE") | ||
| config, err := NewConfig("") | ||
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | ||
| gomega.Expect(config.Containers.ApparmorProfile).To(gomega.Equal("overridden-default")) |
There was a problem hiding this comment.
Let's pretend that the OVERRIDE code breaks, and becomes equivalent to $CONTAINERS_CONF (i.e. ignoring existing conf). This test would happily pass.
Suggestion: include a test for a known-good setting, something that containers_override.conf does not explicitly set. (I don't know what environment this test runs in. It might be necessary to add this new check after line 929 below, something that is set in containers_default.conf). Left as exercise for the reader.
There was a problem hiding this comment.
Great catch! I added a check for a value that only the default.conf sets 👍
Add yet another environment variable for loading containers.conf. When CONTAINERS_CONF_OVERRIDE is set, the specified config file will be loaded last - even when CONTAINERS_CONF is set. This mechanism is needed to preserve system settings and other environment variables. Setting CONTAINERS_CONF will load only the specified config file and ignore all system and user paths. That makes testing hard as many Podman tests use CONTAINERS_CONF for testing. The intended use of CONTAINERS_CONF_OVERRIDE is to set it during tests and point it to a specific configuration of Podman (e.g., netavark with sqlite backend). Similar needs have popped up talking to users in the automotive and high-performance computing space. In a way, such a setting allows for specifying a specific "flavor" of Podman while preserving all existing settings on the system. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
vrothberg
force-pushed
the
containers_conf_extra
branch
from
0df99c5 to
1665db7
Compare
|
Good to go from my end. @rhatdan @edsantiago feel free to merge. |
|
/lgtm |
Add yet another environment variable for loading containers.conf. When CONTAINERS_CONF_OVERRIDE is set, the specified config file will be loaded last - even when CONTAINERS_CONF is set.
This mechanism is needed to preserve system settings and other environment variables. Setting CONTAINERS_CONF will load only the specified config file and ignore all system and user paths. That makes testing hard as many Podman tests use CONTAINERS_CONF for testing.
The intended use of CONTAINERS_CONF_OVERRIDE is to set it during tests and point it to a specific configuration of Podman (e.g., netavark with sqlite backend).
Similar needs have popped up talking to users in the automotive and high-performance computing space. In a way, such a setting allows for specifying a specific "flavor" of Podman while preserving all existing settings on the system.