Consolidate around klog/v2 #314
Conversation
WalkthroughReplaces direct use of github.com/go-logr/logr with k8s.io/klog/v2 logging across the Istio CSR controller, removes the Reconciler.log field, updates tests to drop testr usage, adjusts controller-runtime logger initialization, and moves Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
vendor/github.com/go-logr/logr/testr/testr.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
go.mod(1 hunks)pkg/controller/istiocsr/controller.go(2 hunks)pkg/controller/istiocsr/test_utils.go(2 hunks)pkg/operator/setup_manager.go(2 hunks)
🔇 Additional comments (2)
go.mod (1)
93-93: Incorrect — github.com/go-logr/logr does not need to be a direct dependency.No source file imports github.com/go-logr/logr; ctrllog.SetLogger(klog.Background()) is used in pkg/operator/setup_manager.go:36 and a klog.Logger field is used in pkg/controller/istiocsr/controller.go:51, so logr is pulled in transitively and go mod tidy correctly leaves it marked
// indirect.Likely an incorrect or invalid review comment.
pkg/operator/setup_manager.go (1)
34-37:ctrllog.SetLogger(klog.Background())won’t compile. Use klogr.New().
SetLoggerexpects alogr.Logger.klog.Background()returnsklog.Logger. Use the klog-backed logr.- // Use klog/v2 as the global logr.Logger for controller-runtime. - // Prefer klog.Background() over deprecated klogr. - ctrllog.SetLogger(klog.Background()) + // Use klogr (klog-backed logr.Logger) for controller-runtime. + ctrllog.SetLogger(klogr.New())Likely an incorrect or invalid review comment.
| "sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| klog "k8s.io/klog/v2" |
There was a problem hiding this comment.
Type mismatch: Reconciler.log must be logr.Logger (not klog.Logger).
ctrl.Log.WithName(...) returns a logr.Logger. Storing it in a klog.Logger field won’t compile. Keep the field as logr.Logger and drop the direct klog import here.
- klog "k8s.io/klog/v2"
+ "github.com/go-logr/logr"
@@
- log klog.Logger
+ log logr.LoggerAlso applies to: 51-51
🤖 Prompt for AI Agents
In pkg/controller/istiocsr/controller.go around lines 28 and 51, the code
imports klog and stores ctrl.Log.WithName(...) into a klog.Logger field causing
a type mismatch; change the field type to logr.Logger, remove the direct klog
import, and assign ctrl.Log.WithName(...) (a logr.Logger) to that field. Also
update any references/usages of the field to use logr.Logger methods (or convert
to klog only where needed), and ensure imports and variable declarations compile
after replacing klog.Logger with logr.Logger.
| "k8s.io/client-go/tools/record" | ||
|
|
||
| "github.com/go-logr/logr/testr" | ||
| klog "k8s.io/klog/v2" |
There was a problem hiding this comment.
Use klogr adapter; klog.Background() returns klog.Logger, not logr.Logger.
Tests should provide a logr.Logger (backed by klog) to match controller-runtime. Switch to klogr.
Apply:
- klog "k8s.io/klog/v2"
+ klogr "k8s.io/klog/v2/klogr"
@@
- log: klog.Background(),
+ log: klogr.New(),Also applies to: 41-41
🤖 Prompt for AI Agents
In pkg/controller/istiocsr/test_utils.go around lines 15 and 41, the code
imports klog and uses klog.Background() which returns a klog.Logger rather than
a logr.Logger expected by controller-runtime tests; replace the usage with the
klogr adapter: add import "github.com/go-logr/klogr", create a logr.Logger via
klogr.New() (optionally .WithName(...) if needed) and pass that into tests/code
paths instead of klog.Background(); remove or stop using klog.Background() where
a logr.Logger is required.
sebrandon1
force-pushed
the
remove_extra_loggers
branch
from
1508734 to
c3d8740
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/istiocsr/install_istiocsr.go (1)
28-30: Incomplete logging migration on line 28.This line still uses
r.log.Errorwhile all other logging calls in this function (and across the PR) have been migrated to klog. This inconsistency should be addressed.Apply this diff to complete the migration:
- r.log.Error(err, "failed to reconcile network policy resources") + klog.ErrorS(err, "failed to reconcile network policy resources")
♻️ Duplicate comments (1)
pkg/controller/istiocsr/controller.go (1)
21-21: Past review comment no longer applicable - approach has changed.The previous review flagged a type mismatch with storing
ctrl.Log.WithName(...)in aklog.Loggerfield. The current implementation resolves this by removing thelogfield entirely and using directklogcalls throughout the controller. This approach aligns with the PR objective to consolidate around klog/v2 and remove the logr dependency.Note: This means controller-scoped logging context (previously provided by
WithName) is no longer captured, but this appears intentional for the consolidation effort.Also applies to: 47-53
🧹 Nitpick comments (1)
pkg/controller/istiocsr/controller.go (1)
146-149: Avoid creating synthetic errors solely for logging.Creating
fmt.Errorf("invalid label format")just to satisfyklog.ErrorS's signature is awkward. Since this is a validation failure rather than an operational error, consider usingklog.V(2).InfoSwith a warning message instead, or passnilas the error parameter if you prefer ErrorS.Apply this diff to use structured info logging:
- klog.ErrorS(fmt.Errorf("invalid label format"), "label value not in expected format", + klog.V(2).InfoS("label value not in expected format", "labelName", IstiocsrResourceWatchLabelName, "labelValue", value, "resource", obj.GetName())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
vendor/github.com/go-logr/logr/testr/testr.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (11)
go.mod(1 hunks)pkg/controller/istiocsr/certificates.go(3 hunks)pkg/controller/istiocsr/controller.go(11 hunks)pkg/controller/istiocsr/deployments.go(4 hunks)pkg/controller/istiocsr/install_istiocsr.go(3 hunks)pkg/controller/istiocsr/rbacs.go(16 hunks)pkg/controller/istiocsr/serviceaccounts.go(3 hunks)pkg/controller/istiocsr/services.go(3 hunks)pkg/controller/istiocsr/test_utils.go(0 hunks)pkg/controller/istiocsr/utils.go(3 hunks)pkg/operator/setup_manager.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/controller/istiocsr/test_utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🔇 Additional comments (11)
pkg/operator/setup_manager.go (1)
14-14: LGTM! Past review comment addressed correctly.The import has been updated to use the klogr adapter, and the logger initialization correctly supplies a klog/v2-backed logr.Logger to controller-runtime via
klogr.New(). The explanatory comment is also helpful.Also applies to: 33-34
pkg/controller/istiocsr/rbacs.go (1)
8-8: LGTM! Consistent migration to klog/v2 structured logging.The logging calls have been properly migrated from
r.logto klog's structured logging API (ErrorS/InfoS with key-value pairs), with appropriate verbosity levels.Also applies to: 24-24, 66-66
pkg/controller/istiocsr/serviceaccounts.go (1)
7-7: LGTM! Logging migration aligns with the PR objective.The file properly imports klog/v2 and uses structured logging with appropriate verbosity levels.
Also applies to: 18-18, 29-29
pkg/controller/istiocsr/deployments.go (1)
18-18: LGTM! Logging migration is complete and consistent.The deployment reconciliation logging has been properly migrated to klog/v2 structured logging with appropriate verbosity levels.
Also applies to: 42-42, 53-53, 559-559
pkg/controller/istiocsr/certificates.go (1)
8-8: LGTM! Certificate reconciliation logging properly migrated.The logging calls correctly use klog/v2 structured logging with appropriate verbosity levels.
Also applies to: 25-25, 36-36
pkg/controller/istiocsr/install_istiocsr.go (1)
33-33: LGTM! These logging calls have been properly migrated.The migration to klog.ErrorS and klog.V().InfoS is consistent with the PR objective.
Also applies to: 38-38, 43-43, 48-48, 53-53, 63-63
pkg/controller/istiocsr/services.go (1)
7-7: LGTM! Service reconciliation logging properly migrated.The logging migration to klog/v2 is consistent and uses appropriate structured logging patterns.
Also applies to: 37-37, 48-48
pkg/controller/istiocsr/utils.go (1)
17-17: LGTM! Utility function logging properly migrated.The logging calls in status updates and instance validation have been correctly migrated to klog/v2 structured logging.
Also applies to: 53-53, 410-410
pkg/controller/istiocsr/controller.go (3)
108-119: LGTM!The constructor correctly reflects the removal of the log field and properly initializes the remaining Reconciler fields.
217-245: LGTM!The structured logging calls in the Reconcile function are well-formed, use appropriate verbosity levels, and provide clear context for reconciliation events.
260-322: LGTM!The logging throughout
processReconcileRequestproperly uses structured logging, appropriate verbosity levels, and provides comprehensive observability for reconciliation state changes and error conditions.
|
@sebrandon1: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
1 participant
I was messing around into some other repos and saw there was a
logrbeing included here that wasn't widely used. To remove an extra dependency, I just consolidated the calls tologrwith calls to the existingklog/v2.Similar to: