-
Notifications
You must be signed in to change notification settings - Fork 225
add log_path option to container configuration #2491
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.
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
Conversation
Reviewer's GuideIntroduce a new log_path option in container configuration by extending the data model, adding validation logic, updating docs and defaults, and covering the functionality with tests. Class diagram for updated ContainersConfig struct with log_pathclassDiagram
class ContainersConfig {
+string LogDriver
+string LogPath
+int64 LogSizeMax
+Validate() error
+validateLogPath() error
...
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
004460c to
cf7a70a
Compare
Add support for log_path configuration option in containers.conf that allows users to specify a custom directory for container log files. A log path is read from the [containers] section in a containers.conf. For each container a sub-directory within log_path is created where each directory is named after the container id and inside the subdirectory lives the default name for logs which is ctr.log. This option can be overridden when using the `--log-opt` flag. Vendor was modified until containers/common#2491 is merged. Signed-off-by: Joshua Arrevillaga <2004jarrevillaga@gmail.com>
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter is complaining, otherwise change LGTM
cf7a70a to
4e0ac5d
Compare
pkg/config/config_local.go
Outdated
| return fmt.Errorf("log_path must be an absolute path - instead got %q", c.LogPath) | ||
| } | ||
| if strings.ContainsAny(c.LogPath, "\x00") { | ||
| return fmt.Errorf("log_path contains null bytes - instead got %q", c.LogPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can drop the "instead" from the message, the instead seems confusing in this context because the message is log_path contains null bytes - instead got Maybe it is my language barrier but should it be
log_path should not contain null bytes - instead got in order for the instead to make sense?
But I think just having got %q there is likely already enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
This adds support for configuring container log paths via container.conf, matching the functionality of --log-opts path=... at runtime Fixes customer RFE for per-user container.conf log configuration for use with podman-kube systemd services. Signed-off-by: Joshua Arrevillaga <2004jarrevillaga@gmail.com>
4e0ac5d to
bfc2d91
Compare
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2004joshua, Luap99, sourcery-ai[bot] The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
This adds support for configuring container log paths via container.conf, matching the functionality of --log-opts path=... at runtime
Fixes customer RFE for per-user container.conf log configuration for use with podman-kube systemd services.
Summary by Sourcery
Add support for configuring a custom log path for containers via the containers.conf file, including validation, documentation, and tests
New Features:
Enhancements:
Documentation:
Tests: