🌱 handle resource in informer - #178
Conversation
WalkthroughThis PR removes the ResourceAction parameter and the ResourceAction type/constants across CloudEvents handler and store code, updating interfaces, store implementations, clients, and tests so handlers and store entry points accept only the resource object (context + resource). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ 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 |
1be7def to
dcd8347
Compare
![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
🧹 Nitpick comments (3)
pkg/cloudevents/clients/store/simplestore_test.go (1)
33-37: Remove unusedactionfield from test cases.The
actionfield is defined in the test case struct but never used after line 111 changed to callHandleReceivedResourcewith only the resource. This creates dead code that may confuse future readers.cases := []struct { name string - action watch.EventType received *coordv1.Lease validate func(t *testing.T, namespace, name string) }{And remove the
actionfield assignments from each test case (lines 41, 61, 88).pkg/cloudevents/clients/store/informer.go (1)
80-85: VariableruntimeObjis shadowed inside the if block.The variable
runtimeObjdeclared on line 80 with:=shadows the outerruntimeObjfrom line 51. While this appears intentional (convertingdeletingObjto a runtime object), it may cause confusion during maintenance.Consider using a different variable name for clarity:
- runtimeObj, err := utils.ToRuntimeObject(deletingObj) + deletingRuntimeObj, err := utils.ToRuntimeObject(deletingObj) if err != nil { return err } - return s.Update(runtimeObj) + return s.Update(deletingRuntimeObj)pkg/cloudevents/clients/work/store/informer.go (1)
221-234: Consider performance implications of full store scan for UID lookup.
findWorksByUIDiterates over all works in the store viaListAll()to find works by UID. This is O(n) per received resource and may become a bottleneck with large work counts.The TODO comment on line 170 acknowledges this is for compatibility. Consider adding an index on UID for efficient lookups if this code path is frequently exercised:
// Consider using cache.Indexers with a UID indexer: // cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{ // "uid": func(obj interface{}) ([]string, error) { ... }, // })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
pkg/cloudevents/clients/store/informer.go(1 hunks)pkg/cloudevents/clients/store/informer_test.go(2 hunks)pkg/cloudevents/clients/store/interface.go(1 hunks)pkg/cloudevents/clients/store/simplestore.go(1 hunks)pkg/cloudevents/clients/store/simplestore_test.go(5 hunks)pkg/cloudevents/clients/work/store/base.go(1 hunks)pkg/cloudevents/clients/work/store/informer.go(2 hunks)pkg/cloudevents/clients/work/store/informer_test.go(2 hunks)pkg/cloudevents/generic/clients/agentclient.go(1 hunks)pkg/cloudevents/generic/clients/agentclient_test.go(5 hunks)pkg/cloudevents/generic/clients/clients_metrics_test.go(1 hunks)pkg/cloudevents/generic/clients/sourceclient.go(1 hunks)pkg/cloudevents/generic/clients/sourceclient_test.go(5 hunks)pkg/cloudevents/generic/interface.go(1 hunks)pkg/cloudevents/generic/types/types.go(0 hunks)test/integration/cloudevents/source/client.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/cloudevents/generic/types/types.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-11T13:27:36.331Z
Learnt from: morvencao
Repo: open-cluster-management-io/sdk-go PR: 162
File: pkg/cloudevents/generic/options/pubsub/options.go:157-174
Timestamp: 2025-11-11T13:27:36.331Z
Learning: For open-cluster-management PubSub transport in pkg/cloudevents/generic/options/pubsub: broadcast topics (SourceBroadcast, AgentBroadcast) and their corresponding subscriptions are always required, not optional. The omitempty tags on types.Topics broadcast fields exist because the struct is shared with MQTT (where broadcasts are optional), but PubSub requires all broadcast channels for resync functionality.
Applied to files:
pkg/cloudevents/generic/clients/agentclient_test.gopkg/cloudevents/generic/clients/clients_metrics_test.gopkg/cloudevents/clients/work/store/base.gopkg/cloudevents/clients/store/interface.gotest/integration/cloudevents/source/client.go
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
Repo: open-cluster-management-io/sdk-go PR: 138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
Applied to files:
pkg/cloudevents/clients/store/interface.go
🧬 Code graph analysis (6)
pkg/cloudevents/generic/clients/agentclient_test.go (1)
pkg/cloudevents/generic/testing/resource.go (1)
MockResource(23-31)
pkg/cloudevents/clients/store/informer.go (2)
pkg/cloudevents/clients/work/store/informer.go (1)
AgentInformerWatcherStore(91-95)pkg/cloudevents/clients/utils/utils.go (1)
ToRuntimeObject(183-190)
pkg/cloudevents/generic/clients/clients_metrics_test.go (1)
pkg/cloudevents/generic/testing/resource.go (1)
MockResource(23-31)
pkg/cloudevents/generic/clients/sourceclient_test.go (1)
pkg/cloudevents/generic/testing/resource.go (1)
MockResource(23-31)
pkg/cloudevents/clients/store/simplestore.go (1)
pkg/cloudevents/clients/utils/utils.go (1)
ToRuntimeObject(183-190)
pkg/cloudevents/clients/work/store/informer.go (2)
pkg/cloudevents/clients/store/informer.go (1)
AgentInformerWatcherStore(21-24)pkg/cloudevents/clients/utils/utils.go (1)
UID(222-225)
🔇 Additional comments (17)
pkg/cloudevents/generic/clients/clients_metrics_test.go (1)
102-105: LGTM!The callback signature correctly aligns with the updated
ResourceHandler[T]type that no longer includes the action parameter. The test's focus on metrics validation remains intact.pkg/cloudevents/generic/interface.go (1)
14-15: Breaking API change - verify external consumers.The signature change removes the
actionparameter, shifting responsibility for determining resource handling semantics to the handler implementations. Handlers can infer actions from resource state (e.g.,DeletionTimestampfor deletions, presence in store for adds vs updates).Ensure this breaking change is documented in release notes for any external consumers of this SDK.
pkg/cloudevents/generic/clients/sourceclient.go (1)
205-214: LGTM!The handler invocation correctly passes only the decoded resource object, eliminating the previously hardcoded
types.StatusModifiedaction. Handlers can now determine the appropriate action based on resource state.test/integration/cloudevents/source/client.go (1)
39-41: LGTM!The callback correctly uses the simplified signature. The
UpdateStatuscall appropriately handles the resource without needing an explicit action parameter.pkg/cloudevents/clients/store/interface.go (1)
35-36: LGTM!The interface change correctly removes the action parameter. Implementations can determine the appropriate action from resource state:
- Deletion:
resource.GetDeletionTimestamp() != nil- Add vs Update: check store existence
pkg/cloudevents/clients/store/simplestore_test.go (1)
109-117: LGTM!The test correctly invokes
HandleReceivedResourcewith only the context and resource, matching the updated interface signature. The validation logic properly verifies store state after each operation.pkg/cloudevents/generic/clients/agentclient.go (1)
201-210: LGTM! Handler invocation simplified correctly.The handler is now invoked with just the context and resource object. Each handler can determine the appropriate action by inspecting the resource state (e.g., checking
DeletionTimestampfor deletions). The error handling with conditional verbose logging is appropriate.pkg/cloudevents/clients/work/store/base.go (1)
29-32: LGTM! Clean separation of concerns.The
HandleReceivedResourcemethod now simply enqueues the work for later processing byhandleWork, which determines the appropriate action (add/update/delete) based on the resource state. This aligns with the PR's goal of having handlers determine actions from the resource itself.pkg/cloudevents/clients/work/store/informer_test.go (2)
545-577: LGTM! Test correctly updated for new API.The
HandleReceivedResourcecalls are properly updated to use the new signature. The test expectation change fromDeletedtoModifiedis correct—as noted in the comment,HandleReceivedResourcewith a deletion timestamp callsUpdate(notDelete), which emits aModifiedwatch event. This is consistent with how the informer store handles soft deletes.
8-9: Import cleanup is correct.The
typespackage import was removed sinceResourceActionis no longer used in this test file.pkg/cloudevents/generic/clients/sourceclient_test.go (3)
363-363: Validate signature correctly updated.The validate function signature properly reflects the removal of the
ResourceActionparameter, now taking only the resource.
378-382: Validation logic correctly uses nil checks.For cases where the handler should not be invoked ("unsupported sub resource" and "no registered codec"), checking
resource != nilis the correct approach to verify no handler invocation occurred.Also applies to: 396-400
426-430: Handler and validation calls properly updated.The inline handler now correctly accepts only context and resource. The validation call passes the resource directly without the action parameter.
Also applies to: 451-457
pkg/cloudevents/clients/store/informer_test.go (1)
176-199: LGTM! Test correctly demonstrates all resource handling scenarios.The three
HandleReceivedResourcecalls properly show:
- Add (line 176): New resource with just a name
- Modify (lines 179-187): Existing resource with updated status
- Delete (lines 188-195): Resource with
DeletionTimestampsetThe test correctly validates that all three watch event types (
Added,Modified,Deleted) are received, confirming the store correctly interprets resource state to determine the appropriate action.pkg/cloudevents/generic/clients/agentclient_test.go (1)
350-448: LGTM! Test updates correctly reflect the API change.The test modifications properly align with the removal of
ResourceActionfrom handler signatures. The validation functions now correctly focus on resource-centric checks, and the renamed test case "receive a resource" better describes the new behavior where handlers receive resources without explicit action parameters.pkg/cloudevents/clients/store/simplestore.go (1)
70-75: Behavioral difference fromAgentInformerWatcherStorewhen handling deleting resources with finalizers.When a resource is deleting but has finalizers, this implementation returns
nil(no-op), whileAgentInformerWatcherStore.HandleReceivedResource(ininformer.go) updates the stored object with the deletion timestamp, finalizers, and generation.This may be intentional since
SimpleStoredoesn't have watchers, but please verify this is the expected behavior. If the store should reflect the deletion state for consistency:if !metaObj.GetDeletionTimestamp().IsZero() { if len(metaObj.GetFinalizers()) != 0 { - return nil + return s.Update(runtimeObj) } return s.Delete(runtimeObj) }pkg/cloudevents/clients/store/informer.go (1)
69-89: No type mismatch withmeta.Accessor(lastResource)at line 73.meta.Accessor accepts an arbitrary object pointer, which means it takes
interface{}and can work with any type, includingT(constrained togeneric.ResourceObject). No explicit type assertion or conversion is necessary—the current code is safe and correct. The pattern used here (line 73) appropriately differs from theutils.ToRuntimeObjectconversion at lines 51 and 80 becausemeta.Accessoris explicitly designed to handle arbitrary types.Likely an incorrect or invalid review comment.
Signed-off-by: Wei Liu <liuweixa@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
pkg/cloudevents/clients/store/informer.go(1 hunks)pkg/cloudevents/clients/store/informer_test.go(2 hunks)pkg/cloudevents/clients/store/interface.go(1 hunks)pkg/cloudevents/clients/store/simplestore.go(1 hunks)pkg/cloudevents/clients/store/simplestore_test.go(5 hunks)pkg/cloudevents/clients/work/store/base.go(1 hunks)pkg/cloudevents/clients/work/store/informer.go(2 hunks)pkg/cloudevents/clients/work/store/informer_test.go(2 hunks)pkg/cloudevents/generic/clients/agentclient.go(1 hunks)pkg/cloudevents/generic/clients/agentclient_test.go(5 hunks)pkg/cloudevents/generic/clients/clients_metrics_test.go(1 hunks)pkg/cloudevents/generic/clients/sourceclient.go(1 hunks)pkg/cloudevents/generic/clients/sourceclient_test.go(5 hunks)pkg/cloudevents/generic/interface.go(1 hunks)pkg/cloudevents/generic/types/types.go(0 hunks)test/integration/cloudevents/source/client.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/cloudevents/generic/types/types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cloudevents/generic/interface.go
- pkg/cloudevents/generic/clients/sourceclient.go
- pkg/cloudevents/clients/work/store/informer_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-11T13:27:36.331Z
Learnt from: morvencao
Repo: open-cluster-management-io/sdk-go PR: 162
File: pkg/cloudevents/generic/options/pubsub/options.go:157-174
Timestamp: 2025-11-11T13:27:36.331Z
Learning: For open-cluster-management PubSub transport in pkg/cloudevents/generic/options/pubsub: broadcast topics (SourceBroadcast, AgentBroadcast) and their corresponding subscriptions are always required, not optional. The omitempty tags on types.Topics broadcast fields exist because the struct is shared with MQTT (where broadcasts are optional), but PubSub requires all broadcast channels for resync functionality.
Applied to files:
pkg/cloudevents/generic/clients/clients_metrics_test.gopkg/cloudevents/clients/work/store/base.gopkg/cloudevents/generic/clients/agentclient_test.gopkg/cloudevents/clients/store/interface.gotest/integration/cloudevents/source/client.go
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
Repo: open-cluster-management-io/sdk-go PR: 138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.
Applied to files:
pkg/cloudevents/clients/store/interface.go
🧬 Code graph analysis (6)
pkg/cloudevents/generic/clients/clients_metrics_test.go (1)
pkg/cloudevents/generic/testing/resource.go (1)
MockResource(23-31)
pkg/cloudevents/clients/work/store/informer.go (2)
pkg/cloudevents/clients/store/informer.go (1)
AgentInformerWatcherStore(21-24)pkg/cloudevents/clients/utils/utils.go (1)
UID(222-225)
pkg/cloudevents/generic/clients/sourceclient_test.go (1)
pkg/cloudevents/generic/testing/resource.go (1)
MockResource(23-31)
pkg/cloudevents/generic/clients/agentclient_test.go (1)
pkg/cloudevents/generic/testing/resource.go (1)
MockResource(23-31)
pkg/cloudevents/clients/store/informer.go (2)
pkg/cloudevents/clients/work/store/informer.go (1)
AgentInformerWatcherStore(91-95)pkg/cloudevents/clients/utils/utils.go (1)
ToRuntimeObject(183-190)
test/integration/cloudevents/source/client.go (1)
test/integration/cloudevents/store/resource.go (1)
Resource(18-26)
⏰ 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). (3)
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (18)
pkg/cloudevents/generic/clients/clients_metrics_test.go (1)
102-105: LGTM!The callback signature correctly updated to remove the
ResourceActionparameter, aligning with the PR's objective of unified resource handling.pkg/cloudevents/clients/store/interface.go (1)
35-36: LGTM!The interface signature correctly updated to remove the
ResourceActionparameter. All implementations across the codebase appear to have been updated accordingly.test/integration/cloudevents/source/client.go (1)
39-41: LGTM!The callback signature correctly updated to remove the
ResourceActionparameter while preserving the status update functionality.pkg/cloudevents/generic/clients/agentclient.go (1)
201-210: LGTM!The handler invocation correctly simplified to pass only the resource object, removing action-based logic. This aligns with the PR's objective of unified resource handling.
pkg/cloudevents/clients/work/store/informer.go (3)
168-221: LGTM!The UID-based resource handling logic is well-structured and correctly addresses the past review concern by including an explicit return statement at line 194 after handling the name-change case. The generation checking, deletion handling, and local field preservation all look correct.
223-236: LGTM!The helper method correctly finds works by UID and returns deep copies to prevent unintended mutations.
238-247: LGTM!The helper method correctly identifies the work by namespace and name from the provided list.
pkg/cloudevents/clients/store/simplestore_test.go (1)
35-111: LGTM!The test correctly updated to use
watch.EventTypeand theHandleReceivedResourcecall properly reflects the new signature without the action parameter. Test coverage remains comprehensive.pkg/cloudevents/generic/clients/sourceclient_test.go (1)
363-457: LGTM!The test correctly updated to remove
ResourceActionfrom validation and handler signatures. The validation logic now appropriately focuses on the resource object, with nil checks for non-invocation cases and UID checks for update cases.pkg/cloudevents/clients/work/store/base.go (1)
29-32: LGTM!The method correctly simplified to unconditionally enqueue the received work without action-based branching. This aligns with the PR's objective of unified resource handling.
pkg/cloudevents/generic/clients/agentclient_test.go (4)
356-376: LGTM! Test validation simplified correctly.The validation function signature change from
func(event types.ResourceAction, resource *generictesting.MockResource)tofunc(resource *generictesting.MockResource)aligns with the PR objective of removingResourceAction. The nil-check logic correctly validates that the handler should not be invoked for unsupported sub-resources.
391-395: Validation logic is consistent.The same pattern is applied for the "no registered codec" case - checking
resource != nilto ensure the handler is not invoked when decoding fails.
398-422: Test case renamed and validation updated appropriately.The rename from "create a resource" to "receive a resource" better reflects the new semantics where the handler receives a resource without action context. The validation correctly checks the resource UID.
442-447: Handler signature and invocation updated correctly.The callback now receives only
(context.Context, resource)instead of(context.Context, action, resource), andc.validate(actualRes)correctly validates just the resource. This is consistent with the API change.pkg/cloudevents/clients/store/simplestore.go (1)
51-78: Clean implementation of resource-centric handling.The logic correctly:
- Converts the resource to
runtime.Objectand extracts metadata- Adds new resources that don't exist in the store
- Handles deletion properly - only deletes when
DeletionTimestampis set AND no finalizers remain (lines 70-74)- Updates existing resources otherwise
This follows Kubernetes semantics where a resource with finalizers should be updated (to reflect the deleting state) rather than immediately deleted.
pkg/cloudevents/clients/store/informer_test.go (1)
176-195: Test correctly exercises the new API with all three event types.The test validates:
- Add (line 176): New resource "test0" triggers
watch.Added- Update (lines 179-187): Existing "test1" with status change triggers
watch.Modified- Delete (lines 188-195): Resource with
DeletionTimestampset and no finalizers triggerswatch.DeletedThe test structure properly verifies that the single-path
HandleReceivedResourcecorrectly dispatches to Add/Update/Delete internally based on resource state.pkg/cloudevents/clients/store/informer.go (2)
50-68: Single-path resource handling implemented correctly.The new implementation correctly:
- Converts the resource to
runtime.Objectand extracts metadata- Checks for existence in the store
- Adds new resources directly
This is a clean simplification from the previous action-based dispatch.
88-91: Deletion and update paths are correct.The logic correctly deletes when
DeletionTimestampis set with no finalizers (line 88), and updates otherwise (line 91).
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, skeeey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
08bb1ca
into
open-cluster-management-io:main
2 participants
Summary
the generic agent client only send its received resource to handle, it will not preprocess the resource, each handler will determine how to handle the resource
Related issue(s)
Fixes #
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.