CNF-22825: Consolidate around klog/v2#314
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughLogging in the istio-csr controller was changed from a stored ChangesIstio CSR logging migration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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.
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
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
force-pushed
the
remove_extra_loggers
branch
from
9f039a8 to
7caf980
Compare
|
@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 "5.0.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 |
sebrandon1
force-pushed
the
remove_extra_loggers
branch
from
7caf980 to
1f36ca8
Compare
sebrandon1
force-pushed
the
remove_extra_loggers
branch
from
1f36ca8 to
e2d1fa2
Compare
sebrandon1
force-pushed
the
remove_extra_loggers
branch
2 times, most recently
from
f34619f to
bffa04a
Compare
sebrandon1
force-pushed
the
remove_extra_loggers
branch
2 times, most recently
from
a866ed7 to
226088b
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
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
226088b to
ffa9f64
Compare
|
Rebased onto current master (was 9 months stale). Cherry-picked the klog consolidation commit cleanly. All tests pass with 84.6% coverage on istiocsr package. |
|
@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:
Summary by CodeRabbit
klogwith structuredInfoS/ErrorSoutput, updating reconcile start/error/success messages and resource-specific reconciliation logs (certificates, deployments, RBAC, services, service accounts, network policies, and status updates).