🌱 Use base controller in sdk-go #1251
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 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 |
WalkthroughUpgrades sdk-go and adjusts many controller/factory and queue APIs: renames common/helpers → common/recorder, adds a contextual logging recorder and fake SDK sync context, converts queue/key callbacks to return slices, updates numerous controller sync signatures and test helpers, and replaces several imports to sdk-go factory and cloudevents builder. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to review closely:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
qiujian16
force-pushed
the
work-ctx-logging
branch
from
da688d5 to
c0eb459
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1251 +/- ##
==========================================
- Coverage 62.22% 62.14% -0.09%
==========================================
Files 209 210 +1
Lines 16997 17016 +19
==========================================
- Hits 10576 10574 -2
- Misses 5304 5325 +21
Partials 1117 1117
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/common/queue/queuekey.go (1)
10-15: Fix return type inconsistency and correct the typo in function names.The review comment is accurate. FileterByLabel returns
func(obj interface{}) bool(untyped), while FileterByLabelKeyValue and FilterByNames returnfactory.EventFilterFunc(typed alias). Although factory.EventFilterFunc is defined astype EventFilterFunc func(obj interface{}) bool, the inconsistent return types should be unified for code consistency. Additionally, the typo "Fileter" appears in both line 10 and line 17 and should be corrected to "Filter" in the function names and all call sites.Changes needed:
- Rename
FileterByLabel→FilterByLabel- Rename
FileterByLabelKeyValue→FilterByLabelKeyValue- Change
FileterByLabelreturn type fromfunc(obj interface{}) booltofactory.EventFilterFunc- Update all call sites in test files and production code (5 usages identified)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (99)
go.sumis excluded by!**/*.sumvendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/option.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/write_producer_message.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/.gitignoreis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/00version.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/README.mdis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/adminapi.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/adminoptions.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/api.htmlis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_darwin_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_darwin_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_dynamic.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_glibc_linux_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_glibc_linux_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_musl_linux_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_musl_linux_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_windows.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/config.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/consumer.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/context.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/error.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/error_gen.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/event.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/generated_errors.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/glue_rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/handle.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/header.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/kafka.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/.gitignoreis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/LICENSES.txtis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/README.mdis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/bundle-import.shis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/import.shis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_darwin_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_darwin_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_glibc_linux_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_glibc_linux_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_musl_linux_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_musl_linux_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_windows.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/rdkafka_mock.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/log.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/message.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/metadata.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/misc.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/mockcluster.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/offset.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/producer.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/select_rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/testconf-example.jsonis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/time.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/base_controller.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/options/generic.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/store/informer.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/store/interface.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/store/simplestore.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/codec/manifestbundle.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/store/base.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/store/informer.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/store/local.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/constants/constants.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/agentclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/baseclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/sourceclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/interface.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/metrics/metrics_collector.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/rotation.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/options_noop.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/logger.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/ratelimiter.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/utils/ratelimiter.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/healthcheck.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/heartbeat.gois excluded by!vendor/**
📒 Files selected for processing (37)
go.mod(1 hunks)pkg/common/queue/queuekey.go(1 hunks)pkg/common/recorder/event_recorder.go(1 hunks)pkg/common/recorder/event_recorder_test.go(1 hunks)pkg/common/recorder/logging_recorder.go(1 hunks)pkg/common/testing/fake_sync_context.go(1 hunks)pkg/placement/controllers/manager.go(2 hunks)pkg/registration/hub/lease/controller_test.go(3 hunks)pkg/registration/hub/manager.go(1 hunks)pkg/registration/register/grpc/spoke_driver.go(3 hunks)pkg/registration/spoke/managedcluster/claim_reconcile_test.go(3 hunks)pkg/registration/spoke/managedcluster/joining_controller_test.go(2 hunks)pkg/registration/spoke/managedcluster/resource_reconcile_test.go(2 hunks)pkg/registration/spoke/spokeagent.go(2 hunks)pkg/work/helper/helpers.go(2 hunks)pkg/work/hub/controllers/manifestworkgarbagecollection/controller.go(2 hunks)pkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.go(1 hunks)pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.go(3 hunks)pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.go(1 hunks)pkg/work/hub/manager.go(2 hunks)pkg/work/spoke/auth/cache/auth.go(1 hunks)pkg/work/spoke/auth/cache/executor_cache_controller.go(6 hunks)pkg/work/spoke/auth/factory.go(1 hunks)pkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller_test.go(1 hunks)pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go(1 hunks)pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.go(1 hunks)pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.go(2 hunks)pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.go(2 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go(5 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go(3 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go(6 hunks)pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go(2 hunks)pkg/work/spoke/spokeagent.go(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1196
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go:231-242
Timestamp: 2025-10-14T09:37:12.472Z
Learning: In the ocm repository, there are two different factory packages with different SyncContext.Queue() return types:
1. `github.com/openshift/library-go/pkg/controller/factory` returns `workqueue.RateLimitingInterface` (non-typed)
2. `open-cluster-management.io/sdk-go/pkg/basecontroller/factory` returns `workqueue.TypedRateLimitingInterface[string]` (typed)
When reviewing controller code, check which factory import is used to determine the correct queue interface type.
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
Applied to files:
pkg/registration/hub/lease/controller_test.gopkg/registration/spoke/managedcluster/joining_controller_test.gopkg/work/spoke/spokeagent.gopkg/registration/spoke/managedcluster/claim_reconcile_test.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.gopkg/registration/spoke/managedcluster/resource_reconcile_test.gopkg/registration/register/grpc/spoke_driver.gopkg/registration/spoke/spokeagent.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.gopkg/work/helper/helpers.gopkg/work/hub/manager.gopkg/placement/controllers/manager.gopkg/work/spoke/auth/cache/executor_cache_controller.go
📚 Learning: 2025-10-14T09:37:12.472Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1196
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go:231-242
Timestamp: 2025-10-14T09:37:12.472Z
Learning: In the ocm repository, there are two different factory packages with different SyncContext.Queue() return types:
1. `github.com/openshift/library-go/pkg/controller/factory` returns `workqueue.RateLimitingInterface` (non-typed)
2. `open-cluster-management.io/sdk-go/pkg/basecontroller/factory` returns `workqueue.TypedRateLimitingInterface[string]` (typed)
When reviewing controller code, check which factory import is used to determine the correct queue interface type.
Applied to files:
pkg/common/testing/fake_sync_context.gopkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.gopkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.gopkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.gopkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.gopkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.gopkg/work/spoke/auth/cache/auth.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller_test.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.gopkg/work/hub/controllers/manifestworkgarbagecollection/controller.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.gopkg/work/helper/helpers.gopkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.gopkg/work/spoke/controllers/statuscontroller/availablestatus_controller.gopkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.gopkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.gopkg/work/spoke/auth/cache/executor_cache_controller.go
📚 Learning: 2025-11-06T08:55:13.306Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1242
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go:88-88
Timestamp: 2025-11-06T08:55:13.306Z
Learning: In pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go, the sync method initializes a logger with manifestWorkName and attaches it to the context before calling reconcile methods. Therefore, reconcile methods (like manifestworkReconciler.reconcile) that use klog.FromContext(ctx) automatically inherit the manifestWorkName context and do not need to add it again.
Applied to files:
pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.gopkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.gopkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.gopkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.gopkg/work/spoke/auth/factory.gopkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller_test.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.gopkg/work/hub/controllers/manifestworkgarbagecollection/controller.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.gopkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.gopkg/work/spoke/controllers/statuscontroller/availablestatus_controller.gopkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.gopkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.gopkg/work/spoke/auth/cache/executor_cache_controller.go
📚 Learning: 2025-07-15T06:10:13.001Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1071
File: pkg/server/grpc/clients.go:73-76
Timestamp: 2025-07-15T06:10:13.001Z
Learning: In OCM (Open Cluster Management) gRPC server informer setup, cache sync verification is not necessary when starting informers in the clients.Run() method. The current pattern of starting informers as goroutines without explicit cache sync waiting is the preferred approach for this codebase.
Applied to files:
pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.gopkg/registration/spoke/managedcluster/joining_controller_test.gopkg/registration/spoke/managedcluster/resource_reconcile_test.gopkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.gopkg/registration/spoke/spokeagent.gopkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.gopkg/placement/controllers/manager.gopkg/work/spoke/controllers/statuscontroller/availablestatus_controller.gopkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.gopkg/work/spoke/auth/cache/executor_cache_controller.go
📚 Learning: 2025-10-28T02:55:13.893Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1224
File: pkg/registration/register/grpc/spoke_driver.go:89-98
Timestamp: 2025-10-28T02:55:13.893Z
Learning: In pkg/registration/register/grpc/spoke_driver.go (Go), when calling cloudeventscsr.NewAgentClientHolder with GenericClientOptions, the watcher store does not need to be explicitly provided via WithClientWatcherStore. The GenericClientOptions.AgentClient() method automatically creates a default AgentInformerWatcherStore if none is provided, which satisfies the NewAgentClientHolder requirements.
Applied to files:
pkg/work/spoke/spokeagent.gopkg/registration/register/grpc/spoke_driver.gopkg/registration/spoke/spokeagent.gopkg/work/hub/manager.gopkg/placement/controllers/manager.go
📚 Learning: 2025-06-26T00:34:09.815Z
Learnt from: zhiweiyin318
Repo: open-cluster-management-io/ocm PR: 1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrole.yaml:5-10
Timestamp: 2025-06-26T00:34:09.815Z
Learning: The open-cluster-management-io/ocm codebase uses Go templates (text/template), not Helm templates. The standard pattern for dynamic labels in manifests is: `{{ if gt (len .Labels) 0 }}{{ range $key, $value := .Labels }}"{{ $key }}": "{{ $value }}"{{ end }}{{ end }}`. Do not suggest Helm-specific functions like `toYaml` for this codebase.
Applied to files:
pkg/registration/spoke/managedcluster/claim_reconcile_test.go
📚 Learning: 2025-09-03T08:43:34.751Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1158
File: test/integration/work/completedmanifestwork_test.go:216-256
Timestamp: 2025-09-03T08:43:34.751Z
Learning: In TTL=0 ManifestWork deletion tests, avoid waiting for WorkComplete condition because the controller deletes the resource immediately upon completion, creating a race condition where the test tries to check completion status on an already-deleted resource.
Applied to files:
pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller_test.go
📚 Learning: 2025-08-04T08:58:41.865Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: manifests/klusterlet/management/klusterlet-registration-deployment.yaml:111-115
Timestamp: 2025-08-04T08:58:41.865Z
Learning: In OCM klusterlet deployments, gRPC authentication uses different file naming conventions than CSR/kube authentication: gRPC auth expects config.yaml files (/spoke/bootstrap/config.yaml and /spoke/hub-kubeconfig/config.yaml) while CSR/kube auth uses kubeconfig files. The gRPC driver explicitly creates config.yaml files in the secret data via additionalSecretData["config.yaml"] = d.configTemplate.
Applied to files:
pkg/registration/register/grpc/spoke_driver.go
📚 Learning: 2025-09-24T00:18:33.339Z
Learnt from: zhiweiyin318
Repo: open-cluster-management-io/ocm PR: 1194
File: deploy/klusterlet/chart/klusterlet/templates/bootstrap_kubeconfig_secret.yaml:25-27
Timestamp: 2025-09-24T00:18:33.339Z
Learning: gRPC config in OCM klusterlet bootstrap secrets does not support multiHubBootstrapHubKubeConfigs scenarios - it is intentionally designed only for single hub bootstrap configurations.
Applied to files:
pkg/registration/register/grpc/spoke_driver.go
📚 Learning: 2025-07-02T05:42:41.749Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1058
File: pkg/server/services/work/work.go:39-49
Timestamp: 2025-07-02T05:42:41.749Z
Learning: In the OCM (Open Cluster Management) codebase, nil checks with panic statements in constructor functions for interface parameters are considered unnecessary, as the dependency injection/wiring is managed properly and such checks are not part of the established codebase patterns.
Applied to files:
pkg/placement/controllers/manager.go
🧬 Code graph analysis (28)
pkg/registration/hub/lease/controller_test.go (1)
pkg/common/recorder/event_recorder.go (1)
NewEventRecorder(12-22)
pkg/common/testing/fake_sync_context.go (1)
vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/addontesting/helpers.go (1)
NewTestingEventRecorder(318-320)
pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go (2)
pkg/work/helper/helpers.go (2)
AppliedManifestworkQueueKeyFunc(301-310)AppliedManifestworkHubHashFilter(324-329)vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/registration/spoke/managedcluster/joining_controller_test.go (1)
pkg/common/recorder/event_recorder.go (1)
NewEventRecorder(12-22)
pkg/work/spoke/spokeagent.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.go (1)
NewConfigLoader(24-29)
pkg/registration/spoke/managedcluster/claim_reconcile_test.go (1)
pkg/common/recorder/event_recorder.go (1)
NewEventRecorder(12-22)
pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/registration/spoke/managedcluster/resource_reconcile_test.go (1)
pkg/common/recorder/event_recorder.go (1)
NewEventRecorder(12-22)
pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/registration/register/grpc/spoke_driver.go (3)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.go (1)
NewConfigLoader(24-29)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/constants/constants.go (1)
ConfigTypeGRPC(5-5)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.go (1)
LoadConfig(158-174)
pkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/work/hub/controllers/manifestworkgarbagecollection/controller.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/registration/spoke/spokeagent.go (1)
pkg/common/recorder/event_recorder.go (1)
NewEventRecorder(12-22)
pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go (1)
pkg/common/recorder/logging_recorder.go (1)
NewContextualLoggingEventRecorder(23-27)
pkg/work/hub/manager.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.go (1)
NewConfigLoader(24-29)
pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go (3)
pkg/common/queue/queuekey.go (1)
QueueKeyByMetaName(57-60)pkg/work/helper/helpers.go (1)
AppliedManifestworkAgentIDFilter(313-321)vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (2)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/controller_context.go (1)
NewSyncContext(21-28)vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/placement/controllers/manager.go (1)
pkg/common/recorder/event_recorder.go (1)
NewEventRecorder(12-22)
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.go (2)
pkg/common/queue/queuekey.go (1)
FileterByLabel(10-15)vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/spoke/auth/cache/executor_cache_controller.go (3)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/controller_context.go (1)
NewSyncContext(21-28)vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)pkg/work/spoke/auth/store/cache_store.go (1)
Dimension(75-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: unit
- GitHub Check: e2e-singleton
- GitHub Check: e2e-hosted
- GitHub Check: integration
- GitHub Check: verify
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: cloudevents-integration
🔇 Additional comments (53)
pkg/work/hub/manager.go (1)
21-21: LGTM! Improved readability with more descriptive import alias.The rename from
generictogenericbuildermakes the code more self-documenting, and the usage is updated consistently.Also applies to: 80-80
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go (3)
24-24: LGTM! Correct factory import for sdk-go migration.The import has been properly updated to use the sdk-go basecontroller factory, which is consistent with the PR objectives.
79-81: LGTM! Clean migration to sdk-go controller construction.The controller construction has been properly updated:
- The recorder argument is correctly removed from
ToController(), aligning with the sdk-go's contextual logging approach- The factory builder pattern is cleanly applied
84-107: LGTM! Excellent sync method migration.The sync method has been properly updated to work with the sdk-go basecontroller:
- The signature now includes
manifestWorkName stringas a parameter, which is the standard pattern for sdk-go basecontroller where the queue key is directly passed to the sync function- Logger initialization correctly uses the manifestWorkName parameter and attaches it to the context (line 85-87)
- The typed queue usage at line 105 (
Queue().AddAfter(manifestWorkName, ...)) is correct, as the sdk-go factory returnsTypedRateLimitingInterface[string]Based on learnings.
pkg/common/recorder/event_recorder_test.go (1)
1-1: LGTM! Package rename aligns with directory structure.The package rename from
helperstorecorderis consistent with the file's location inpkg/common/recorder/and aligns with the broader refactoring to consolidate event recorder functionality.pkg/work/spoke/auth/cache/auth.go (1)
20-20: LGTM! Clean migration to sdk-go base controller factory.The import change from
openshift/library-gotoopen-cluster-management.io/sdk-go/pkg/basecontroller/factoryaligns with the PR's objective to leverage the base controller in sdk-go.Based on learnings
pkg/work/spoke/auth/factory.go (1)
58-59: LGTM! Contextual logging improves observability.The migration from
klog.Infofto contextual logging with structured key-value pairs (logger.Info("Executor caches enabled", "cacheValidator", isCacheValidator)) aligns with the project-wide adoption of context-aware logging and improves log filtering and analysis.go.mod (1)
44-44: LGTM! SDK upgrade enables new controller features.The upgrade to
open-cluster-management.io/sdk-go v1.1.1-0.20251110055937-105041a1c094enables the base controller and contextual logging features used throughout this PR. Note that this is a pre-release version (indicated by the commit hash suffix).pkg/work/helper/helpers.go (2)
32-32: LGTM! Import addition supports typed queue migration.The addition of the sdk-go
basecontroller/factoryimport enables the use offactory.ObjectQueueKeysFuncfor multi-key queue operations.Based on learnings
301-309: LGTM! Function signature updated for multi-key queue support.The function signature change from
factory.ObjectQueueKeyFuncreturningstringtofactory.ObjectQueueKeysFuncreturning[]stringaligns with the sdk-go base controller's typed queue interface. The implementation correctly wraps return values in slices:
- Empty slice
[]string{}when the name doesn't match the hubhash prefix- Single-element slice
[]string{trimmedName}when it matchesBased on learnings
pkg/common/testing/fake_sync_context.go (1)
29-43: LGTM! Test helper added for SDK-based controllers.The new
FakeSDKSyncContexttype provides test scaffolding for controllers migrated to the sdk-go base controller factory, usingTypedRateLimitingInterface[string]consistent with the SDK's typed queue interface. The implementation mirrors the existingFakeSyncContextpattern while supporting the new typed queue.Based on learnings
pkg/work/hub/controllers/manifestworkgarbagecollection/controller.go (3)
20-20: LGTM! Migration to sdk-go base controller factory.The import change from
openshift/library-gotoopen-cluster-management.io/sdk-go/pkg/basecontroller/factoryis consistent with the PR's objective to leverage the base controller in sdk-go.Based on learnings
48-48: LGTM! ToController simplified with recorder removal.The
ToControllercall no longer passes the recorder argument, which aligns with the sdk-go base controller pattern where the recorder is likely managed internally by the factory.
52-54: LGTM! Sync method signature updated for SDK pattern.The sync method signature change to accept a
key stringparameter aligns with the sdk-go base controller's sync interface. The key is now provided as a parameter rather than being derived fromcontrollerContext.QueueKey(), consistent with the SDK'sSyncContextinterface that only providesQueue()access.Based on learnings
pkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.go (1)
92-93: LGTM! SDK sync context migration is correct.The test correctly migrates to
NewFakeSDKSyncContextand passes the appropriate key parameter to the sync method, aligning with the SDK-based controller factory pattern.pkg/work/spoke/auth/cache/executor_cache_controller.go (6)
20-20: LGTM! Factory import updated to SDK-based controller.The migration from
library-go/pkg/controller/factorytosdk-go/pkg/basecontroller/factoryaligns with the PR objectives and the learned pattern about typed queue interfaces.Based on learnings
111-111: LGTM! SDK sync context creation updated correctly.The
NewSyncContextcall correctly removes the recorder parameter, matching the SDK factory API signature shown in the relevant code snippets.
140-140: LGTM! Controller builder updated to SDK pattern.The
ToControllercall correctly removes the recorder parameter, consistent with the SDK-based factory API.
257-260: LGTM! Excellent contextual logging implementation.The sync method properly:
- Accepts the
executorKeyparameter (aligning with SDK factory sync signature)- Extracts logger from context
- Enriches logger with
executorKeystructured field- Re-attaches enriched logger to context for downstream use
This follows best practices for contextual logging.
263-265: LGTM! Structured logging migration is well done.The conversion from
klog.V(4).Infoftologger.V(4).Infowith structured key-value pairs ("count", c.executorCaches.Count()) improves log machine-readability and follows modern logging best practices.
281-293: LGTM! Nested logger usage is correct.The
iterateCacheItemsFnproperly extracts the logger from the already-enriched context and uses structured logging for the cache update operation.pkg/registration/hub/lease/controller_test.go (3)
25-25: LGTM! Event recorder import path updated.The import change from
helperstorecorderis part of the systematic package reorganization moving event recording utilities to a dedicated package.
177-177: LGTM! Event recorder constructor updated.The call site correctly uses
recorder.NewEventRecordermatching the new package structure.
291-291: LGTM! Event recorder constructor updated.Consistent usage of
recorder.NewEventRecorderas expected.pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller_test.go (1)
197-197: LGTM! Test migrated to SDK sync context.The update to
NewFakeSDKSyncContextis consistent with the broader test infrastructure migration to SDK-based patterns.pkg/registration/spoke/managedcluster/resource_reconcile_test.go (2)
29-29: LGTM! Event recorder import updated.Consistent with the project-wide migration to the
recorderpackage.
323-323: LGTM! Event recorder constructor updated.Correct usage of
recorder.NewEventRecorder.pkg/registration/spoke/managedcluster/joining_controller_test.go (2)
23-23: LGTM! Event recorder import updated.Consistent with the package reorganization.
89-89: LGTM! Event recorder constructor updated.Correct usage of
recorder.NewEventRecorder.pkg/registration/spoke/managedcluster/claim_reconcile_test.go (3)
28-28: LGTM! Event recorder import updated.Consistent with the package reorganization.
202-202: LGTM! Event recorder constructor updated.Correct usage of
recorder.NewEventRecorder.
581-581: LGTM! Event recorder constructor updated.Consistent usage in the second test case.
pkg/registration/spoke/spokeagent.go (2)
31-31: LGTM! Event recorder import updated with clear alias.The import alias
eventrecorderdistinguishes the package from theevents.Recorderinterface type used throughout this file, improving code clarity.
361-361: LGTM! Event recorder constructor updated.Correct usage of
eventrecorder.NewEventRecorderwith the aliased import.pkg/registration/hub/manager.go (1)
32-32: LGTM!The import path update from
helperstorecorderis clean and maintains backward compatibility through the alias.pkg/common/recorder/event_recorder.go (1)
1-1: LGTM!Package rename from
helperstorecorderclearly reflects the module's purpose for event recording.pkg/work/spoke/spokeagent.go (1)
26-26: LGTM!The migration from
generic.NewConfigLoadertogenericbuilder.NewConfigLoadercorrectly adopts the updated sdk-go API. The builder pattern provides better separation of concerns for configuration loading.Also applies to: 237-237
pkg/registration/register/grpc/spoke_driver.go (1)
34-34: LGTM!Consistent application of the
genericbuilder.NewConfigLoaderAPI in both bootstrapped and non-bootstrapped config loading paths.Also applies to: 240-240, 249-249
pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go (2)
19-19: LGTM!The migration from library-go factory to SDK basecontroller factory is correctly implemented. The switch from
WithFilteredEventsInformersQueueKeyFunc(singular) toWithFilteredEventsInformersQueueKeysFunc(plural) aligns with the helper function returning[]string, and the removal of the recorder fromToControlleris consistent with the new SDK API. Based on learnings.Also applies to: 61-65
68-68: LGTM!The sync method signature correctly adds the
manifestWorkNameparameter, which is now passed explicitly rather than being derived from the queue key within the method. This change aligns with the SDK basecontroller factory's typed queue interface. Based on learnings.pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.go (1)
289-290: LGTM!Test correctly updated to use
NewFakeSDKSyncContextand pass theappliedManifestWorkNameparameter to the sync method, aligning with the controller's new signature.pkg/placement/controllers/manager.go (1)
17-17: LGTM!The import path and function call correctly updated to use the
recorderpackage instead ofhelpers.Also applies to: 47-47
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.go (1)
248-249: LGTM!Test correctly migrated to
NewFakeSDKSyncContextand the sync method invocation properly includes the key parameter (namespace/name), consistent with the SDK basecontroller factory pattern.pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go (2)
19-19: LGTM! Factory migration is consistent.The migration from library-go factory to sdk-go basecontroller factory is correctly implemented. The removal of the recorder parameter from
ToControlleraligns with the new SDK pattern where contextual logging is handled differently.Also applies to: 59-59
62-65: LGTM! Sync signature and logging pattern are correct.The updated signature accepts the
appliedManifestWorkNameas an explicit parameter, which aligns with the new SDK factory pattern. The logger construction correctly adds the work name to the context before propagating it to downstream operations.Based on learnings
pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go (1)
207-208: LGTM! Test updates align with production changes.The test correctly uses
NewFakeSDKSyncContextand passes the work name as the third argument tosync, matching the updated controller signature.pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go (2)
378-379: LGTM! Test sync calls correctly updated.All test cases have been properly updated to use
NewFakeSDKSyncContextand pass the work name as the third argument tosync, consistent with the new controller signature.Also applies to: 421-422, 556-557, 593-594
900-900: LGTM! Queue construction updated to typed interface.The queue construction correctly uses
workqueue.NewTypedRateLimitingQueue[string]withDefaultTypedControllerRateLimiter[string](), aligning with the new SDK factory's typed queue interface.Based on learnings
Also applies to: 1063-1063
pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.go (1)
19-19: LGTM! Import and unused parameter handling are appropriate.The factory import migration is correct, and marking the
SyncContextparameter as unused with_is the proper Go idiom since this reconciler doesn't reference it in its implementation.Also applies to: 32-32
pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.go (1)
22-22: LGTM! Test infrastructure correctly migrated.The test properly imports the SDK factory, uses
NewFakeSDKSyncContext, and passes the work name tosync, aligning with the production code changes.Also applies to: 300-301
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (3)
28-28: LGTM! Controller factory migration is well-executed.The migration to SDK factory is correctly implemented:
- Import changed to
sdk-go/pkg/basecontroller/factory- Sync context created using
factory.NewSyncContext- ToController call properly removes the recorder parameter
Also applies to: 80-80, 120-120
126-129: LGTM! Sync signature and logging are correctly updated.The sync method now receives
manifestWorkNameas an explicit string parameter, and the logger is initialized with this value before being attached to the context. This follows the established pattern for contextual logging in this codebase.Based on learnings
231-242: LGTM! Event handlers correctly use typed queues.Both
onAddFuncandonUpdateFunchave been properly updated to acceptworkqueue.TypedRateLimitingInterface[string]instead of the non-typed interface, which aligns with the SDK factory's typed queue implementation.Based on learnings
Also applies to: 244-267
We can leverage contextual logger in base controller. Signed-off-by: Jian Qiu <[email protected]>
qiujian16
force-pushed
the
work-ctx-logging
branch
from
c0eb459 to
f8cceaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pkg/work/spoke/auth/cache/executor_cache_controller.go (1)
261-261: Consider using a named constant for the cleanup trigger.The magic string
"key"is used as a special sentinel value to trigger cache cleanup during resync. This makes the code less clear and maintainable.Consider defining a named constant at package level:
+const ( + cleanupTriggerKey = "key" +) + var ( ResyncInterval = 10 * time.Minute )Then use it in the check:
- if executorKey == "key" { + if executorKey == cleanupTriggerKey {pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go (1)
54-82: Clarify the unusedrecorderargument.
recorderis no longer passed to the sdk-go factory or stored on the controller, so callers now supply a value that is silently dropped. Please either remove the parameter (and adjust call sites) or plumb it into the new base controller so event recording continues to flow through the sync context. Leaving a dead parameter invites confusion later.pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (1)
66-122: Reconnect or drop the unusedrecorderparameter.After the move to the sdk-go factory,
recordernever gets used: it isn’t stored onManifestWorkController, fed into the sync context, or otherwise propagated. Please either thread it into the new factory wiring (if events are still needed) or delete the parameter and clean up call sites. Carrying an unused constructor argument is misleading for future readers.pkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.go (1)
29-46: Handle therecorderparameter explicitly.
recorderisn’t used anymore—ToControlleris invoked without it and the struct doesn’t retain it—so the argument is effectively ignored. Please remove the parameter or wire it into the new sdk-go factory so it serves a purpose. Otherwise we leave a misleading API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (99)
go.sumis excluded by!**/*.sumvendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/option.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/write_producer_message.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/.gitignoreis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/00version.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/README.mdis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/adminapi.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/adminoptions.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/api.htmlis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_darwin_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_darwin_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_dynamic.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_glibc_linux_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_glibc_linux_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_musl_linux_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_musl_linux_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_windows.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/config.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/consumer.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/context.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/error.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/error_gen.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/event.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/generated_errors.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/glue_rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/handle.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/header.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/kafka.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/.gitignoreis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/LICENSES.txtis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/README.mdis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/bundle-import.shis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/import.shis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_darwin_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_darwin_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_glibc_linux_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_glibc_linux_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_musl_linux_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_musl_linux_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_windows.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/rdkafka_mock.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/log.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/message.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/metadata.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/misc.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/mockcluster.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/offset.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/producer.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/select_rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/testconf-example.jsonis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/time.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/base_controller.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/options/generic.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/store/informer.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/store/interface.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/store/simplestore.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/codec/manifestbundle.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/store/base.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/store/informer.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/store/local.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/constants/constants.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/agentclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/baseclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/sourceclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/interface.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/metrics/metrics_collector.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/rotation.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/options_noop.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/logger.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/ratelimiter.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/utils/ratelimiter.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/healthcheck.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/heartbeat.gois excluded by!vendor/**
📒 Files selected for processing (37)
go.mod(1 hunks)pkg/common/queue/queuekey.go(1 hunks)pkg/common/recorder/event_recorder.go(1 hunks)pkg/common/recorder/event_recorder_test.go(1 hunks)pkg/common/recorder/logging_recorder.go(1 hunks)pkg/common/testing/fake_sync_context.go(1 hunks)pkg/placement/controllers/manager.go(2 hunks)pkg/registration/hub/lease/controller_test.go(3 hunks)pkg/registration/hub/manager.go(1 hunks)pkg/registration/register/grpc/spoke_driver.go(3 hunks)pkg/registration/spoke/managedcluster/claim_reconcile_test.go(3 hunks)pkg/registration/spoke/managedcluster/joining_controller_test.go(2 hunks)pkg/registration/spoke/managedcluster/resource_reconcile_test.go(2 hunks)pkg/registration/spoke/spokeagent.go(2 hunks)pkg/work/helper/helpers.go(2 hunks)pkg/work/hub/controllers/manifestworkgarbagecollection/controller.go(2 hunks)pkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.go(1 hunks)pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.go(3 hunks)pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.go(1 hunks)pkg/work/hub/manager.go(2 hunks)pkg/work/spoke/auth/cache/auth.go(1 hunks)pkg/work/spoke/auth/cache/executor_cache_controller.go(6 hunks)pkg/work/spoke/auth/factory.go(1 hunks)pkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller_test.go(1 hunks)pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go(1 hunks)pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.go(1 hunks)pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.go(2 hunks)pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.go(2 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go(5 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go(3 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go(6 hunks)pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go(2 hunks)pkg/work/spoke/spokeagent.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/common/recorder/event_recorder_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/registration/spoke/managedcluster/claim_reconcile_test.go
- pkg/registration/hub/manager.go
- pkg/registration/hub/lease/controller_test.go
- pkg/registration/spoke/spokeagent.go
- pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go
- pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go
- pkg/registration/register/grpc/spoke_driver.go
- pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller_test.go
- pkg/placement/controllers/manager.go
- pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.go
- pkg/common/recorder/event_recorder.go
- go.mod
- pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1196
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go:231-242
Timestamp: 2025-10-14T09:37:12.472Z
Learning: In the ocm repository, there are two different factory packages with different SyncContext.Queue() return types:
1. `github.com/openshift/library-go/pkg/controller/factory` returns `workqueue.RateLimitingInterface` (non-typed)
2. `open-cluster-management.io/sdk-go/pkg/basecontroller/factory` returns `workqueue.TypedRateLimitingInterface[string]` (typed)
When reviewing controller code, check which factory import is used to determine the correct queue interface type.
📚 Learning: 2025-11-06T08:55:13.306Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1242
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go:88-88
Timestamp: 2025-11-06T08:55:13.306Z
Learning: In pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go, the sync method initializes a logger with manifestWorkName and attaches it to the context before calling reconcile methods. Therefore, reconcile methods (like manifestworkReconciler.reconcile) that use klog.FromContext(ctx) automatically inherit the manifestWorkName context and do not need to add it again.
Applied to files:
pkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.gopkg/work/hub/controllers/manifestworkgarbagecollection/controller.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.gopkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.gopkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.gopkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.gopkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.gopkg/work/spoke/auth/cache/executor_cache_controller.gopkg/work/spoke/auth/factory.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.gopkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go
📚 Learning: 2025-10-14T09:37:12.472Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1196
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go:231-242
Timestamp: 2025-10-14T09:37:12.472Z
Learning: In the ocm repository, there are two different factory packages with different SyncContext.Queue() return types:
1. `github.com/openshift/library-go/pkg/controller/factory` returns `workqueue.RateLimitingInterface` (non-typed)
2. `open-cluster-management.io/sdk-go/pkg/basecontroller/factory` returns `workqueue.TypedRateLimitingInterface[string]` (typed)
When reviewing controller code, check which factory import is used to determine the correct queue interface type.
Applied to files:
pkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.gopkg/work/hub/controllers/manifestworkgarbagecollection/controller.gopkg/common/testing/fake_sync_context.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.gopkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.gopkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.gopkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.gopkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.gopkg/work/helper/helpers.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.gopkg/work/spoke/auth/cache/auth.gopkg/work/spoke/auth/cache/executor_cache_controller.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.gopkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go
📚 Learning: 2025-07-15T06:10:13.001Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1071
File: pkg/server/grpc/clients.go:73-76
Timestamp: 2025-07-15T06:10:13.001Z
Learning: In OCM (Open Cluster Management) gRPC server informer setup, cache sync verification is not necessary when starting informers in the clients.Run() method. The current pattern of starting informers as goroutines without explicit cache sync waiting is the preferred approach for this codebase.
Applied to files:
pkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.gopkg/registration/spoke/managedcluster/resource_reconcile_test.gopkg/registration/spoke/managedcluster/joining_controller_test.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.gopkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.gopkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.gopkg/work/spoke/auth/cache/executor_cache_controller.gopkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
Applied to files:
pkg/registration/spoke/managedcluster/resource_reconcile_test.gopkg/registration/spoke/managedcluster/joining_controller_test.gopkg/work/spoke/spokeagent.gopkg/work/hub/manager.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.gopkg/work/helper/helpers.gopkg/work/spoke/auth/cache/executor_cache_controller.gopkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.go
📚 Learning: 2025-10-28T02:55:13.893Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1224
File: pkg/registration/register/grpc/spoke_driver.go:89-98
Timestamp: 2025-10-28T02:55:13.893Z
Learning: In pkg/registration/register/grpc/spoke_driver.go (Go), when calling cloudeventscsr.NewAgentClientHolder with GenericClientOptions, the watcher store does not need to be explicitly provided via WithClientWatcherStore. The GenericClientOptions.AgentClient() method automatically creates a default AgentInformerWatcherStore if none is provided, which satisfies the NewAgentClientHolder requirements.
Applied to files:
pkg/work/spoke/spokeagent.gopkg/work/hub/manager.go
🧬 Code graph analysis (18)
pkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/hub/controllers/manifestworkgarbagecollection/controller.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/common/testing/fake_sync_context.go (1)
vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/addontesting/helpers.go (1)
NewTestingEventRecorder(318-320)
pkg/registration/spoke/managedcluster/resource_reconcile_test.go (1)
pkg/common/recorder/event_recorder.go (1)
NewEventRecorder(12-22)
pkg/registration/spoke/managedcluster/joining_controller_test.go (1)
pkg/common/recorder/event_recorder.go (1)
NewEventRecorder(12-22)
pkg/work/spoke/spokeagent.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.go (1)
NewConfigLoader(24-29)
pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/work/hub/manager.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.go (1)
NewConfigLoader(24-29)
pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.go (1)
pkg/common/testing/fake_sync_context.go (1)
NewFakeSDKSyncContext(37-43)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.go (2)
pkg/common/queue/queuekey.go (1)
FileterByLabel(10-15)vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go (3)
pkg/common/queue/queuekey.go (1)
QueueKeyByMetaName(57-60)pkg/work/helper/helpers.go (1)
AppliedManifestworkAgentIDFilter(313-321)vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.go (2)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)vendor/k8s.io/klog/v2/internal/serialize/keyvalues.go (1)
WithValues(35-52)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (3)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/controller_context.go (1)
NewSyncContext(21-28)vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)vendor/k8s.io/klog/v2/internal/serialize/keyvalues.go (1)
WithValues(35-52)
pkg/work/spoke/auth/cache/executor_cache_controller.go (3)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/controller_context.go (1)
NewSyncContext(21-28)vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)pkg/work/spoke/auth/store/cache_store.go (1)
Dimension(75-82)
pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: cloudevents-integration
- GitHub Check: verify
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: e2e-singleton
- GitHub Check: e2e-hosted
- GitHub Check: e2e
🔇 Additional comments (33)
pkg/work/spoke/auth/factory.go (1)
58-59: LGTM! Clean adoption of contextual, structured logging.The change from plain
klog.Infofto context-derived structured logging is a solid improvement. The key-value format is more maintainable and the contextual logger can carry request-specific metadata.pkg/registration/spoke/managedcluster/joining_controller_test.go (2)
23-23: LGTM! Package import updated correctly.The import change from
helperstorecorderaligns with the broader refactor to consolidate event recording utilities. Based on learnings.
89-93: LGTM! Event recorder creation updated correctly.The call to
recorder.NewEventRecorderis correct with all required parameters properly supplied, and error handling is appropriately implemented. This change enables contextual logging support in the test.pkg/work/hub/manager.go (1)
21-21: LGTM! Improved import alias naming.The import alias rename from
generictogenericbuilderimproves code clarity by making the package purpose more explicit. The usage is consistently updated throughout the file.Also applies to: 80-80
pkg/work/spoke/auth/cache/auth.go (1)
20-20: LGTM! Clean migration to sdk-go base controller.The import path update from library-go factory to sdk-go basecontroller/factory aligns with the PR objective to leverage the contextual logger in the base controller. The field type on line 40 and controller usage remain compatible with the new factory package.
pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.go (2)
19-19: LGTM! Import migration to sdk-go base controller.The import change from library-go factory to sdk-go basecontroller factory aligns with the PR objective to leverage the base controller framework. Based on learnings, the sdk-go factory provides typed queue interfaces which improve type safety.
32-32: LGTM! Appropriate use of blank identifier.Using the blank identifier for the unused SyncContext parameter is correct and idiomatic. The reconciler doesn't need the SyncContext since it has its own rateLimiter field (line 27) and properly uses the contextual logger via
klog.FromContext(ctx)(line 36).pkg/work/spoke/spokeagent.go (2)
26-26: LGTM! Import updated to use builder package.The import correctly switches to the new builder-based config loader from sdk-go, with a clear alias that makes the usage readable.
237-238: LGTM! Config loader usage correctly updated.The function call correctly uses the new builder package while maintaining the same parameters and error handling pattern. This is a clean refactoring with no behavioral changes.
pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go (3)
19-19: LGTM: Migration to sdk-go factory.The import change from library-go to sdk-go basecontroller factory aligns with the PR objectives and provides typed queue interfaces for better type safety.
59-59: LGTM: Updated controller binding for sdk-go.The ToController binding correctly follows the sdk-go pattern without passing a recorder parameter.
62-65: LGTM: Explicit parameter and contextual logging setup.The sync method signature change makes the appliedManifestWorkName explicit, and the logger initialization correctly establishes contextual logging that propagates downstream. This pattern ensures better observability and aligns with the sdk-go base controller approach.
pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go (1)
21-21: LGTM! Clean migration to sdk-go base controller.The migration from
library-go/pkg/controller/factorytosdk-go/pkg/basecontroller/factoryis correctly implemented:
- Import path updated to use sdk-go (line 21)
WithInformersQueueKeyFunc→WithInformersQueueKeysFuncwith proper[]stringreturn type (lines 80-82)ToControllercall updated to remove the recorder parameter (line 87)syncmethod signature correctly extended with explicitappliedManifestWorkName stringparameter (line 90)- Contextual logger properly initialized using the provided key parameter (lines 91-93)
The queue operations at line 147 (
Queue().AddAfter) are compatible with the new typed queue interfaceTypedRateLimitingInterface[string].Based on learnings.
Also applies to: 80-87, 90-93
pkg/work/spoke/auth/cache/executor_cache_controller.go (4)
20-20: LGTM: Migration to sdk-go base controller factory.The import change aligns with the PR objective to leverage contextual logging in the base controller. Based on learnings, this factory provides a typed queue interface (
TypedRateLimitingInterface[string]) compared to the previous library-go factory.Based on learnings
111-111: LGTM: API signature updates for sdk-go factory.The updated signatures for
NewSyncContextandToControllercorrectly reflect the new sdk-go base controller API, which no longer requires the recorder parameter at these construction points.Also applies to: 140-140
257-260: LGTM: Contextual logging correctly implemented.The sync method signature update correctly adopts the new pattern where
executorKeyis passed directly as a parameter. The contextual logging implementation follows best practices: extracting the logger from context, enriching it with the executorKey, and propagating it through the call chain.
263-263: LGTM: Structured logging adoption.The migration to structured logging using
logger.Info()with key-value pairs is a best practice improvement over formatted strings. The logger correctly inherits theexecutorKeycontext set in the sync method through context propagation.Also applies to: 265-265, 281-281, 293-293
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go (2)
378-379: LGTM!The test correctly uses
NewFakeSDKSyncContextand passeswork.Nameas the key parameter to thesyncmethod, aligning with the new SDK-based controller pattern.
900-900: LGTM!The queue creation correctly uses the typed
NewTypedRateLimitingQueue[string]with the appropriate typed rate limiter, consistent with the SDK migration.pkg/work/hub/controllers/manifestworkgarbagecollection/controller_test.go (1)
92-93: LGTM!The test correctly uses
NewFakeSDKSyncContextand passes the key"default/test"to thesyncmethod, consistent with the SDK-based controller pattern.pkg/work/hub/controllers/manifestworkgarbagecollection/controller.go (3)
20-20: LGTM!The import correctly switches from
github.com/openshift/library-go/pkg/controller/factorytoopen-cluster-management.io/sdk-go/pkg/basecontroller/factory, aligning with the SDK migration. Based on learnings, the sdk-go factory returns a typed queue interface.
48-48: LGTM!The
ToControllercall is correctly updated to match the sdk-go factory pattern by removing the recorder parameter.
52-60: LGTM!The sync method signature correctly accepts the
keyparameter directly instead of retrieving it viacontrollerContext.QueueKey(). The key is properly split into namespace and name components and logged appropriately.pkg/common/testing/fake_sync_context.go (1)
29-33: The review comment is incorrect.
FakeSDKSyncContextcorrectly implements theSyncContextinterface fromopen-cluster-management.io/sdk-go, which only requires theQueue()method. ThespokeNameandrecorderfields are used during initialization but do not need to be exposed as methods, since the sdk-go interface doesn't require them.The existing
FakeSyncContextexposesQueueKey()andRecorder()methods because it implements the separateopenshift/library-gofactory interface, which has different requirements. These are two distinct interfaces for different factory implementations—not variations of the same interface.Likely an incorrect or invalid review comment.
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controllers_test.go (1)
248-249: LGTM! Test correctly updated for new SDK sync signature.The test properly migrates to
NewFakeSDKSyncContextand passes the explicitkeyparameter to match the updated sync method signature. The key format (namespace/name) aligns with the controller's expectation.pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.go (4)
30-30: LGTM! Migration to sdk-go basecontroller factory.The import correctly switches to the sdk-go basecontroller factory, which provides typed queue interfaces and contextual logging support.
Based on learnings
104-114: LGTM! Queue key function correctly migrated to multi-key contract.The function now returns
[]stringto support the sdk-go basecontroller's multi-key per event capability. Empty results ([]string{}) and single-key results ([]string{"namespace/name"}) are correctly represented.
120-120: LGTM! ToController correctly updated.The recorder parameter is removed as the sdk-go basecontroller now handles event recording internally with contextual logging support, aligning with the PR's objective.
157-160: LGTM! Sync signature correctly updated with contextual logging.The sync method now receives an explicit
keyparameter and initializes a contextual logger with structured fields. This aligns with the sdk-go basecontroller pattern and enables better observability throughout the reconciliation flow.Based on learnings
pkg/common/queue/queuekey.go (1)
10-15: Inconsistent return types across filter functions in the same file.The change removes the
factory.EventFilterFunctype fromFileterByLabel's return signature, making it returnfunc(obj interface{}) booldirectly. This creates an inconsistency:
FileterByLabelnow returns:func(obj interface{}) boolFileterByLabelKeyValue(line 17) still returns:factory.EventFilterFuncFilterByNames(line 24) still returns:factory.EventFilterFuncWhile these types are structurally identical and the code likely compiles, mixing named and unnamed versions of the same function type across a single module is poor API design and suggests incomplete migration. Either all three functions should use the same type, or the change should be reverted.
Additionally, the pre-existing typo in the function name ("Fileter" vs "Filter") remains.
pkg/work/helper/helpers.go (2)
32-32: LGTM! Import migrated to sdk-go base controller.The import change from library-go factory to sdk-go basecontroller factory aligns with the PR objective to leverage the base controller with contextual logging.
Based on learnings
301-309: Function signature correctly migrated to support multiple queue keys—all call sites verified.The migration from
ObjectQueueKeyFunc(returningstring) toObjectQueueKeysFunc(returning[]string) is correctly implemented. The single usage atpkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go:62properly invokesWithFilteredEventsInformersQueueKeysFunc(), which expectsObjectQueueKeysFunc. The logic is preserved: no prefix match returns an empty slice, and a match returns a single-element slice. The sdk-go factory import confirms compatibility with typed queue requirements.pkg/common/recorder/logging_recorder.go (1)
33-37: LGTM!The method correctly creates a new instance to avoid mutation, and appropriately preserves the context from the original recorder.
| func (r *ContextualLoggingEventRecorder) WithContext(ctx context.Context) events.Recorder { | ||
| r.ctx = ctx | ||
| return r | ||
| } |
There was a problem hiding this comment.
Fix race condition: WithContext mutates shared state.
The method mutates r.ctx directly and returns the same receiver. If the same recorder instance is shared across goroutines (common in controller setups), concurrent calls to WithContext will race on the ctx field, leading to nondeterministic context propagation.
Apply this diff to return a new instance:
func (r *ContextualLoggingEventRecorder) WithContext(ctx context.Context) events.Recorder {
- r.ctx = ctx
- return r
+ newRecorder := *r
+ newRecorder.ctx = ctx
+ return &newRecorder
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *ContextualLoggingEventRecorder) WithContext(ctx context.Context) events.Recorder { | |
| r.ctx = ctx | |
| return r | |
| } | |
| func (r *ContextualLoggingEventRecorder) WithContext(ctx context.Context) events.Recorder { | |
| newRecorder := *r | |
| newRecorder.ctx = ctx | |
| return &newRecorder | |
| } |
🤖 Prompt for AI Agents
In pkg/common/recorder/logging_recorder.go around lines 17 to 20, WithContext
currently mutates the receiver's ctx field which introduces a race when the
recorder is shared across goroutines; change it to return a new
ContextualLoggingEventRecorder instance (a shallow copy of the receiver) with
ctx set to the provided context so the original receiver is not mutated and
callers receive a separate recorder with the new context.
| // NewContextualLoggingEventRecorder provides event recorder that will log all recorded events via klog. | ||
| func NewContextualLoggingEventRecorder(component string) events.Recorder { | ||
| return &ContextualLoggingEventRecorder{ | ||
| component: component, | ||
| } | ||
| } |
There was a problem hiding this comment.
Document nil‑context fallback or initialize ctx.
The constructor leaves ctx uninitialized (nil). When Event or Warning is called before WithContext, klog.FromContext(nil) falls back to the global logger, losing contextual information. Consider either initializing ctx to context.Background() or documenting that callers must invoke WithContext before logging.
Optional fix to initialize context:
func NewContextualLoggingEventRecorder(component string) events.Recorder {
return &ContextualLoggingEventRecorder{
component: component,
+ ctx: context.Background(),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NewContextualLoggingEventRecorder provides event recorder that will log all recorded events via klog. | |
| func NewContextualLoggingEventRecorder(component string) events.Recorder { | |
| return &ContextualLoggingEventRecorder{ | |
| component: component, | |
| } | |
| } | |
| // NewContextualLoggingEventRecorder provides event recorder that will log all recorded events via klog. | |
| func NewContextualLoggingEventRecorder(component string) events.Recorder { | |
| return &ContextualLoggingEventRecorder{ | |
| component: component, | |
| ctx: context.Background(), | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/common/recorder/logging_recorder.go around lines 22 to 27, the
constructor leaves the recorder's ctx nil which causes klog.FromContext(nil) to
fall back to the global logger; either initialize ctx to context.Background() in
NewContextualLoggingEventRecorder so the recorder always has a non‑nil context,
or update the constructor's doc comment to explicitly require callers to call
WithContext before Event/Warning; if choosing initialization, ensure the context
package is imported and set the struct's ctx field to context.Background() when
constructing the recorder.
| func (r *ContextualLoggingEventRecorder) Event(reason, message string) { | ||
| logger := klog.FromContext(r.ctx) | ||
| logger.Info(fmt.Sprintf("INFO: %s", message), "reason", reason) | ||
| } | ||
|
|
||
| func (r *ContextualLoggingEventRecorder) Eventf(reason, messageFmt string, args ...interface{}) { | ||
| r.Event(reason, fmt.Sprintf(messageFmt, args...)) | ||
| } | ||
|
|
||
| func (r *ContextualLoggingEventRecorder) Warning(reason, message string) { | ||
| logger := klog.FromContext(r.ctx) | ||
| logger.Info(fmt.Sprintf("WARNING: %s", message), "reason", reason) | ||
| } | ||
|
|
||
| func (r *ContextualLoggingEventRecorder) Warningf(reason, messageFmt string, args ...interface{}) { | ||
| r.Warning(reason, fmt.Sprintf(messageFmt, args...)) | ||
| } |
There was a problem hiding this comment.
Include component name in log output.
The component field is stored and managed via ForComponent and ComponentName, but never emitted in the actual log messages. Operators cannot distinguish which component produced a log entry, reducing the value of component-scoped recorders.
Apply this diff to include the component:
func (r *ContextualLoggingEventRecorder) Event(reason, message string) {
logger := klog.FromContext(r.ctx)
- logger.Info(fmt.Sprintf("INFO: %s", message), "reason", reason)
+ logger.Info(fmt.Sprintf("INFO: %s", message), "component", r.component, "reason", reason)
}
func (r *ContextualLoggingEventRecorder) Warning(reason, message string) {
logger := klog.FromContext(r.ctx)
- logger.Info(fmt.Sprintf("WARNING: %s", message), "reason", reason)
+ logger.Info(fmt.Sprintf("WARNING: %s", message), "component", r.component, "reason", reason)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *ContextualLoggingEventRecorder) Event(reason, message string) { | |
| logger := klog.FromContext(r.ctx) | |
| logger.Info(fmt.Sprintf("INFO: %s", message), "reason", reason) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Eventf(reason, messageFmt string, args ...interface{}) { | |
| r.Event(reason, fmt.Sprintf(messageFmt, args...)) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warning(reason, message string) { | |
| logger := klog.FromContext(r.ctx) | |
| logger.Info(fmt.Sprintf("WARNING: %s", message), "reason", reason) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warningf(reason, messageFmt string, args ...interface{}) { | |
| r.Warning(reason, fmt.Sprintf(messageFmt, args...)) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Event(reason, message string) { | |
| logger := klog.FromContext(r.ctx) | |
| logger.Info(fmt.Sprintf("INFO: %s", message), "component", r.component, "reason", reason) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Eventf(reason, messageFmt string, args ...interface{}) { | |
| r.Event(reason, fmt.Sprintf(messageFmt, args...)) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warning(reason, message string) { | |
| logger := klog.FromContext(r.ctx) | |
| logger.Info(fmt.Sprintf("WARNING: %s", message), "component", r.component, "reason", reason) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warningf(reason, messageFmt string, args ...interface{}) { | |
| r.Warning(reason, fmt.Sprintf(messageFmt, args...)) | |
| } |
1 participant
We can leverage contextual logger in base controller.
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Refactor
Tests
Chores