-
Notifications
You must be signed in to change notification settings - Fork 246
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
refactor(log): ingore error log when journal is disable #1955
Conversation
Friendly ping @jlebon |
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.
Hello! sorry for the delay and thank you for adding this change, and 100% that makes sense. Could you make two small changes.
- include in your commit message the distribution that you are referring to
i.e somthing like: "the distro x has no journal. Add conditional to avoid failure due to logging method not being available. " - add a item to the release notes under changes sub-header
2a3eacf
to
34978b9
Compare
@prestist Done |
@BlackHole1 sorry I still am seeing some spelling/grammar issues with your commit message wdyt about this? exec/engine: log to journal only when available Alpine Linux does not include systemd’s journal, which causes Ignition to emit warnings when attempting to log. This PR introduces a check to determine if the journal is available on the current distribution, and skips logging to the journal when it is not present. Signed-off-by: Kevin Cui [email protected] Also can you please add an comment to the release notes? talking about this under changes for the unreleased version. |
6b57636
to
f98f1ef
Compare
@prestist Thank you for your review suggestions! |
@BlackHole1 I took a closer look at the code and the symptoms, have you tested this code change? I suspect you will still get that warning. |
@prestist Sorry I'm late. I just did some testing locally. The issue you're worried about won't happen. package main
import (
"fmt"
"github.com/coreos/go-systemd/v22/journal"
)
func main() {
fmt.Println("====> test journal.Enabled()")
for i := 0; i < 10; i++ {
fmt.Println(journal.Enabled())
}
fmt.Println("====> test journal.Send()")
for i := 0; i < 10; i++ {
if err := journal.Send("MESSAGE=Hello, World!", journal.PriInfo, nil); err != nil {
fmt.Println("Send Error: ", err)
}
}
} |
Friendly ping @prestist :) |
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.
One nit, but looks sane to me
Alpine Linux does not include systemd’s journal, which causes Ignition to emit warnings when attempting to log. This PR introduces a check to determine if the journal is available on the current distribution, and skips logging to the journal when it is not present. Signed-off-by: Kevin Cui <[email protected]>
f98f1ef
to
0b340f0
Compare
@jlebon Done |
There is no systemd and journal in our distribution, and when using ignition, many warning logs are printed. The changes in this PR will check if there is a journal in the current distribution, and if not, it will not pass the logs to the journal.
Notes: The distro Alpine has no journal. Add conditional to avoid failure due to logging method not being available.