Skip to content
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

Use klog for logging #166

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dhaiducek
Copy link
Member

There are probably better options, but this works for now (it logs the filename without a path).

Also cleans up go.mod.

There are probably better options, but this works
for now (it logs the filename without a path).

Signed-off-by: Dale Haiducek <[email protected]>
Copy link

openshift-ci bot commented Jul 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jul 3, 2024
Signed-off-by: Dale Haiducek <[email protected]>
Comment on lines +141 to +146
replace (
// Replaced for compatibility with k8s.io/[email protected]
// Above v0.29.5, we'd need to upgrade Go to v1.22, which isn't compatible
github.com/prometheus/client_golang => github.com/prometheus/client_golang v1.18.0
github.com/prometheus/common => github.com/prometheus/common v0.47.0
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trades out replace statements, but I'm assuming the kubernetes-dependency-watch version bump is desireable, so hopefully the next round we can get rid of these entirely.

@mprahl
Copy link
Member

mprahl commented Jul 5, 2024

@dhaiducek what was the reason for moving to klog?

@dhaiducek
Copy link
Member Author

@dhaiducek what was the reason for moving to klog?

There were other logging statements using klog and when I was doing some testing I ran into a panic because SetLogger() had never been set up with the way we're trying to log currently. klog was the low bar that worked rather than trying to figure out how to wire up controller-runtime with logger, especially since it looked like it was already being used elsewhere, for instance:

klog.Infof("Deleted the compliance history API Route of %s", RouteName)

Of course, I could try to implement the same logging flows as our other controllers. I'm not actually sure why that hadn't been done previously though...

@openshift-merge-robot
Copy link

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants