pkg/config: rework testing and add new containers.rootless.conf search location #2499
Conversation
This file contains hidden or 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
Reviewer's GuideThis PR refactors the container config loading pipeline by introducing an explicit ER diagram for config file search order including new rootless locationserDiagram
ETC_CONTAINERS_CONF ||--o{ ETC_CONTAINERS_CONF_D : contains
ETC_CONTAINERS_CONF ||--o| ETC_CONTAINERS_ROOTLESS_CONF : overrides
ETC_CONTAINERS_ROOTLESS_CONF ||--o{ ETC_CONTAINERS_ROOTLESS_CONF_D : contains
ETC_CONTAINERS_ROOTLESS_CONF_D ||--o{ ETC_CONTAINERS_ROOTLESS_CONF_D_UID : contains
HOME_CONTAINERS_CONF ||--o{ HOME_CONTAINERS_CONF_D : contains
ETC_CONTAINERS_CONF {
string path
}
ETC_CONTAINERS_CONF_D {
string path
}
ETC_CONTAINERS_ROOTLESS_CONF {
string path
int uid
}
ETC_CONTAINERS_ROOTLESS_CONF_D {
string path
}
ETC_CONTAINERS_ROOTLESS_CONF_D_UID {
string path
int uid
}
HOME_CONTAINERS_CONF {
string path
}
HOME_CONTAINERS_CONF_D {
string path
}
ER diagram for module search directorieserDiagram
PATHS ||--o{ MODULES_HOME : has
PATHS ||--o{ MODULES_ETC : has
PATHS ||--o{ MODULES_USR : has
PATHS {
string home
string etc
string usr
int uid
}
MODULES_HOME {
string path
}
MODULES_ETC {
string path
}
MODULES_USR {
string path
}
Class diagram for new config loading pipeline and paths abstractionclassDiagram
class Options {
+bool SetDefault
+[]string Modules
+modules(paths *paths) ([]string, error)
}
class paths {
+string usr
+string etc
+string home
+int uid
+defaultPaths() (*paths, error)
}
class Config {
+LoadedModules() []string
+Validate() error
+addCAPPrefix()
}
Options --> paths : uses
Config --> Options : uses
Options : modules(paths *paths)
paths : defaultPaths()
Config : LoadedModules()
Config : Validate()
Config : addCAPPrefix()
Class diagram for updated config file search and module resolutionclassDiagram
class ConfigLoader {
+New(options *Options) (*Config, error)
+Default() (*Config, error)
+newLocked(options *Options, paths *paths) (*Config, error)
+systemConfigs(paths *paths) ([]string, error)
+addConfigs(dirPath string, configs []string) ([]string, error)
}
class ModuleResolver {
+moduleDirectories(paths *paths) []string
+ModuleDirectories() ([]string, error)
}
ConfigLoader --> paths : uses
ConfigLoader --> Options : uses
ModuleResolver --> paths : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Luap99 - I've reviewed your changes - here's some feedback:
- The
newLockedfunction now requires bothOptionsand apathsparameter, which propagates path-resolution concerns throughout the code—consider encapsulating path discovery insideOptionsor using a builder to simplify these function signatures. - There’s overlapping logic in
systemConfigsandmoduleDirectoriesfor assembling the config search hierarchy; extracting the shared path-merging logic into a common helper would reduce duplication and help keep path resolution consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `newLocked` function now requires both `Options` and a `paths` parameter, which propagates path-resolution concerns throughout the code—consider encapsulating path discovery inside `Options` or using a builder to simplify these function signatures.
- There’s overlapping logic in `systemConfigs` and `moduleDirectories` for assembling the config search hierarchy; extracting the shared path-merging logic into a common helper would reduce duplication and help keep path resolution consistent.
## Individual Comments
### Comment 1
<location> `docs/containers.conf.5.md:15` </location>
<code_context>
Container engines read the __/usr/share/containers/containers.conf__,
__/etc/containers/containers.conf__, and __/etc/containers/containers.conf.d/\*.conf__
for global configuration that effects all users.
+For global configuration that only effects rootless users use __/etc/containers/containers.rootless.conf__,
+__/etc/containers/containers.rootless.d/\*.conf__ and __/etc/containers/containers.rootless.d/\$UID/\*.conf__. The UID is is the users uid which podman runs under so it can be used to specify a certain config for a only a single user without having to put the config into the users home directory.
For user specific configuration it reads __\$XDG_CONFIG_HOME/containers/containers.conf__ and
__\$XDG_CONFIG_HOME/containers/containers.conf.d/\*.conf__ files. When `$XDG_CONFIG_HOME` is not set it falls back to using `$HOME/.config` instead.
</code_context>
<issue_to_address>
Replace 'effects' with 'affects' in both new lines.
In this context, 'affects' is correct because it means to influence users.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
for global configuration that effects all users.
For global configuration that only effects rootless users use __/etc/containers/containers.rootless.conf__,
=======
for global configuration that affects all users.
For global configuration that only affects rootless users use __/etc/containers/containers.rootless.conf__,
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `docs/containers.conf.5.md:16` </location>
<code_context>
__/etc/containers/containers.conf__, and __/etc/containers/containers.conf.d/\*.conf__
for global configuration that effects all users.
+For global configuration that only effects rootless users use __/etc/containers/containers.rootless.conf__,
+__/etc/containers/containers.rootless.d/\*.conf__ and __/etc/containers/containers.rootless.d/\$UID/\*.conf__. The UID is is the users uid which podman runs under so it can be used to specify a certain config for a only a single user without having to put the config into the users home directory.
For user specific configuration it reads __\$XDG_CONFIG_HOME/containers/containers.conf__ and
__\$XDG_CONFIG_HOME/containers/containers.conf.d/\*.conf__ files. When `$XDG_CONFIG_HOME` is not set it falls back to using `$HOME/.config` instead.
</code_context>
<issue_to_address>
Remove repeated 'is' in 'The UID is is the users uid...'.
It should read: 'The UID is the user's uid...'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
__/etc/containers/containers.rootless.d/\*.conf__ and __/etc/containers/containers.rootless.d/\$UID/\*.conf__. The UID is is the users uid which podman runs under so it can be used to specify a certain config for a only a single user without having to put the config into the users home directory.
=======
__/etc/containers/containers.rootless.d/\*.conf__ and __/etc/containers/containers.rootless.d/\$UID/\*.conf__. The UID is the user's uid which podman runs under so it can be used to specify a certain config for only a single user without having to put the config into the user's home directory.
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
docs/containers.conf.5.md
Outdated
Comment on lines
14
to
15
| for global configuration that effects all users. | ||
| For global configuration that only effects rootless users use __/etc/containers/containers.rootless.conf__, |
There was a problem hiding this comment.
issue (typo): Replace 'effects' with 'affects' in both new lines.
In this context, 'affects' is correct because it means to influence users.
Suggested change
| for global configuration that effects all users. | |
| For global configuration that only effects rootless users use __/etc/containers/containers.rootless.conf__, | |
| for global configuration that affects all users. | |
| For global configuration that only affects rootless users use __/etc/containers/containers.rootless.conf__, |
docs/containers.conf.5.md
Outdated
| __/etc/containers/containers.conf__, and __/etc/containers/containers.conf.d/\*.conf__ | ||
| for global configuration that effects all users. | ||
| For global configuration that only effects rootless users use __/etc/containers/containers.rootless.conf__, | ||
| __/etc/containers/containers.rootless.d/\*.conf__ and __/etc/containers/containers.rootless.d/\$UID/\*.conf__. The UID is is the users uid which podman runs under so it can be used to specify a certain config for a only a single user without having to put the config into the users home directory. |
There was a problem hiding this comment.
issue (typo): Remove repeated 'is' in 'The UID is is the users uid...'.
It should read: 'The UID is the user's uid...'.
Suggested change
| __/etc/containers/containers.rootless.d/\*.conf__ and __/etc/containers/containers.rootless.d/\$UID/\*.conf__. The UID is is the users uid which podman runs under so it can be used to specify a certain config for a only a single user without having to put the config into the users home directory. | |
| __/etc/containers/containers.rootless.d/\*.conf__ and __/etc/containers/containers.rootless.d/\$UID/\*.conf__. The UID is the user's uid which podman runs under so it can be used to specify a certain config for only a single user without having to put the config into the user's home directory. |
There is no reason to use filepath.Walk() as we do not want to recurse at all as such this just makes the code harder to follow. We can just use os.ReadDir() which also sorts the result already so there is no need to sort ourselves again. Signed-off-by: Paul Holzinger <[email protected]>
They should be no need for external consumers to directly access these. Signed-off-by: Paul Holzinger <[email protected]>
Add three new search location when running rootless for the config file: - /etc/containers/containers.rootless.conf - /etc/containers/containers.rootless.conf.d - /etc/containers/containers.rootless.conf.d/<UID>/ The idea is that this can be set by system administrators to define default config for all rootless users without setting the options for the rootful podman. Today /etc/containers/containers.conf is already used as rootless but its options are also read when podman is used as root which made it not useable for some users. Signed-off-by: Paul Holzinger <[email protected]>
Define a helper struct which contains the search paths. That way unit tests can mock the paths by setting them to something else so we don't actually end up reading the paths under /usr and /etc and $HOME. Signed-off-by: Paul Holzinger <[email protected]>
This allows us to fully control the test paths so it doesn't read global system paths which can break unit tests. Signed-off-by: Paul Holzinger <[email protected]>
module tests are currently actually broken as rootless as the home dir is resolved only once and does not take the test override into account. Instead rewrite modules to use the new paths overwrite struct used by th emain containers.conf tests already. With that we can mock all paths and even root/rootless cases while running as the same user. Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
It is mainly used for tetsing without adding much value. We can check the loaded modules via the LoadedModules() call on the Config. Signed-off-by: Paul Holzinger <[email protected]>
Until now there didn't seem to exists a proper that that actually verifies the containers.conf loading order so fix that. The test got a bit complicated but I coud not find something better. Signed-off-by: Paul Holzinger <[email protected]>
mheon
reviewed
Aug 6, 2025
mheon
reviewed
Aug 6, 2025
| for global configuration that effects all users. | ||
| for global configuration that affects all users. | ||
| For global configuration that only affects rootless users use __/etc/containers/containers.rootless.conf__, | ||
| __/etc/containers/containers.rootless.d/\*.conf__ and __/etc/containers/containers.rootless.d/\$UID/\*.conf__. The UID is the user's uid which podman runs under so it can be used to specify a certain config for only a single user without having to put the config into the user's home directory. |
Member
There was a problem hiding this comment.
I think you should split $UID out into a separate sentence. containers.rootless.conf and containers.rootless.conf.d are global configuration. the $UID bit is not, and should be discussed separately.
Member
Author
There was a problem hiding this comment.
Mhh I read globally different here. Globally to me == system directories in this context.
How should this be worded instead then? tbf I think the entire paragraph is just pointless noise and hard to parse. I think just listing each location in the right order like I have in the design docs is likely the most logical.
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, Luap99, sourcery-ai[bot] 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 |
Luap99
changed the title
Member
|
/lgtm |
openshift-merge-bot
bot
merged commit 55585de
into
containers:main
15 of 16 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
see commits
Summary by Sourcery
Centralize and refactor config file path management into a reusable paths struct, add new rootless-specific config search locations, simplify file scanning logic, rename constants to unexported, update module loading to respect the new structure, and adjust tests and documentation accordingly.
New Features:
Enhancements:
Documentation:
Tests: