CNF-22825: Consolidate around klog/v2#314
CNF-22825: Consolidate around klog/v2#314sebrandon1 wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughLogging infrastructure migrated across the istio-csr controller from a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. DetailsNeeds 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.
|
/retest |
sebrandon1
force-pushed
the
remove_extra_loggers
branch
from
c3d8740 to
d663ec0
Compare
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)
24-27: Incomplete migration causes build failure.Line 25 still uses
r.log.Error()but thelogfield does not exist in theReconcilerstruct. This causes a build error.Other error logging statements in this same file have already been migrated to
klog.ErrorS(), but this one was missed.🐛 Proposed fix
if err := r.createOrApplyNetworkPolicies(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { - r.log.Error(err, "failed to reconcile network policy resources") + klog.ErrorS(err, "failed to reconcile network policy resources") return err }Note: The same issue exists in
networkpolicies.go(lines 17, 64, 80, 86) which usesr.log.V().Info()and also needs migration to theklogequivalent.
🧹 Nitpick comments (1)
pkg/controller/istiocsr/rbacs.go (1)
142-143: Consider simplifying error logging pattern.Creating inline errors with
fmt.Errorf()solely for logging is unconventional. Consider logging without wrapping in an error:- klog.ErrorS(fmt.Errorf("error updating clusterrole name in status"), "status update error", "istiocsr", istiocsr.GetNamespace()) + klog.V(1).InfoS("unable to determine clusterrole name for status update", "istiocsr", istiocsr.GetNamespace())However, since this preserves existing behavior and works correctly, this is optional.
Also applies to: 232-233
Remove the logr.Logger field from the istiocsr Reconciler struct and replace all r.log calls with direct klog/v2 structured logging calls. This eliminates the go-logr/logr/testr dependency from the istiocsr package and consolidates logging on the existing klog/v2 dependency.
sebrandon1
force-pushed
the
remove_extra_loggers
branch
from
d663ec0 to
9f039a8
Compare
sebrandon1
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sebrandon1: This pull request references CNF-22825 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest ci/prow/e2e-operator |
|
/retest |
|
@sebrandon1: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
3 participants
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: