Verify logdriver and eventslogger will work together #1025
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 |
|
@edsantiago PTAL |
Fixes:containers/podman#12175 Signed-off-by: Daniel J Walsh <[email protected]>
|
LGTM but I don't feel qualified to slash-lgtm it. Sorry for late response. |
|
@vrothberg @mheon @Luap99 @baude PTAL |
| func LoggingValid(logDriver string, eventsLogger string) error { | ||
|
|
||
| validLogDriver := []string{"file", "k8s-file", "journald", "none"} | ||
| validEventsLogger := []string{"file", "journald", "none"} |
There was a problem hiding this comment.
All of these strings should be constants.
| if !util.StringInSlice(eventsLogger, validEventsLogger) { | ||
| return errors.Errorf("invalid EventsLogger %q must be one of %q", eventsLogger, strings.Join(validEventsLogger, ", ")) | ||
| } | ||
| logErr := errors.Errorf("invalid logging combination LogDriver %q is not allowed with EventsLogger %q", logDriver, eventsLogger) |
There was a problem hiding this comment.
Perhaps it is best to change to a Warnf and be done with it.
There was a problem hiding this comment.
Can you elaborate on why we need this check? It seems legitimate to me to not use the same backend for logs and events but I may be missing something.
There was a problem hiding this comment.
Full context is: containers/podman#12175 (podman-remote, different log drivers).
There was a problem hiding this comment.
Bottom line this is not something we recommend and we should warn against it. I agree that we can not error on this.
I agree this will likely break many installations. Everything except logs will work so we should not fail unless logs is called. |
|
@rhatdan: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes:containers/podman#12175
Signed-off-by: Daniel J Walsh [email protected]