🌱 [refactor] reorg the ce generic packages - #157
Conversation
WalkthroughRefactors CloudEvents packages: splits functionality into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
|
/assign @qiujian16 |
![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 (1)
pkg/cloudevents/generic/clients/clients_metrics_test.go (1)
101-106: Consider storing the type-asserted client to avoid repeated assertions.The agent is cast to
*CloudEventAgentClient[*generictesting.MockResource]twice: once on line 101 and again on line 104 within the callback. Storing the concrete client would improve readability and efficiency.Apply this diff to eliminate the redundant cast:
+ agentClient := agent.(*CloudEventAgentClient[*generictesting.MockResource]) // start agent subscription - agent.(*CloudEventAgentClient[*generictesting.MockResource]).subscribe( + agentClient.subscribe( ctx, func(ctx context.Context, evt cloudevents.Event) { - agent.(*CloudEventAgentClient[*generictesting.MockResource]).receive(ctx, evt) + agentClient.receive(ctx, evt) }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
pkg/cloudevents/clients/addon/client.go(1 hunks)pkg/cloudevents/clients/addon/client_test.go(2 hunks)pkg/cloudevents/clients/cluster/client.go(1 hunks)pkg/cloudevents/clients/cluster/client_test.go(3 hunks)pkg/cloudevents/clients/csr/client.go(1 hunks)pkg/cloudevents/clients/csr/client_test.go(2 hunks)pkg/cloudevents/clients/event/client.go(1 hunks)pkg/cloudevents/clients/event/client_test.go(3 hunks)pkg/cloudevents/clients/lease/client.go(1 hunks)pkg/cloudevents/clients/lease/client_test.go(3 hunks)pkg/cloudevents/clients/options/generic.go(5 hunks)pkg/cloudevents/clients/work/agent/client/manifestwork.go(10 hunks)pkg/cloudevents/clients/work/source/client/manifestwork.go(12 hunks)pkg/cloudevents/generic/clients/agentclient.go(7 hunks)pkg/cloudevents/generic/clients/agentclient_test.go(22 hunks)pkg/cloudevents/generic/clients/baseclient.go(4 hunks)pkg/cloudevents/generic/clients/clients_metrics_test.go(11 hunks)pkg/cloudevents/generic/clients/sourceclient.go(8 hunks)pkg/cloudevents/generic/clients/sourceclient_test.go(20 hunks)pkg/cloudevents/generic/metrics/metrics_collector.go(11 hunks)pkg/cloudevents/generic/options/builder/optionsbuilder.go(1 hunks)pkg/cloudevents/generic/options/builder/optionsbuilder_test.go(2 hunks)pkg/cloudevents/generic/testing/resource.go(1 hunks)pkg/cloudevents/generic/utils/ratelimiter.go(2 hunks)test/integration/cloudevents/certrotation_test.go(2 hunks)test/integration/cloudevents/heartbeat_reconnect_test.go(2 hunks)test/integration/cloudevents/source/client.go(2 hunks)test/integration/cloudevents/suite_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#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/work/agent/client/manifestwork.gopkg/cloudevents/generic/clients/baseclient.gopkg/cloudevents/generic/metrics/metrics_collector.gopkg/cloudevents/generic/clients/clients_metrics_test.go
📚 Learning: 2025-09-17T13:29:00.675Z
Learnt from: qiujian16
PR: open-cluster-management-io/sdk-go#147
File: pkg/cloudevents/generic/options/grpc/protocol/heartbeat_integration_test.go:66-68
Timestamp: 2025-09-17T13:29:00.675Z
Learning: In the pkg/cloudevents/generic/options/grpc/protocol package, bufSize is already defined as a constant in protocal_test.go and is accessible to other test files in the same package, including heartbeat_integration_test.go.
Applied to files:
test/integration/cloudevents/suite_test.gopkg/cloudevents/generic/options/builder/optionsbuilder_test.gotest/integration/cloudevents/heartbeat_reconnect_test.go
📚 Learning: 2025-09-16T02:22:20.929Z
Learnt from: skeeey
PR: open-cluster-management-io/sdk-go#144
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:200-213
Timestamp: 2025-09-16T02:22:20.929Z
Learning: In the GRPC CloudEvents protocol implementation, when startEventsReceiver encounters a stream error, it sends the error to reconnectErrorChan. The consumer of this channel handles the error by calling Close() on the protocol, which triggers close(p.closeChan), causing OpenInbound to unblock and call cancel() to properly terminate both the events receiver and heartbeat watcher goroutines.
Applied to files:
pkg/cloudevents/generic/clients/baseclient.gopkg/cloudevents/generic/clients/sourceclient_test.gopkg/cloudevents/generic/clients/agentclient_test.go
📚 Learning: 2025-09-16T06:03:45.232Z
Learnt from: skeeey
PR: open-cluster-management-io/sdk-go#144
File: pkg/cloudevents/server/grpc/broker.go:169-194
Timestamp: 2025-09-16T06:03:45.232Z
Learning: In the Open Cluster Management gRPC CloudEvents implementation, client reconnections due to heartbeat timeout are considered acceptable behavior by the maintainer skeeey, rather than something that needs optimization to prevent.
Applied to files:
pkg/cloudevents/generic/clients/baseclient.gotest/integration/cloudevents/heartbeat_reconnect_test.go
📚 Learning: 2025-09-12T02:55:39.701Z
Learnt from: qiujian16
PR: open-cluster-management-io/sdk-go#140
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:36-39
Timestamp: 2025-09-12T02:55:39.701Z
Learning: In the GRPC protocol health check implementation, WithReconnectErrorOption already validates the interval parameter and sets a default of 20 seconds if interval <= 0, preventing time.NewTicker panics.
Applied to files:
test/integration/cloudevents/heartbeat_reconnect_test.go
🧬 Code graph analysis (25)
pkg/cloudevents/clients/addon/client.go (2)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/clients/addon/wrapper.go (1)
ManagedClusterAddOnInterface(59-68)
pkg/cloudevents/clients/cluster/client_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
pkg/cloudevents/clients/lease/client.go (1)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)
pkg/cloudevents/clients/work/agent/client/manifestwork.go (5)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/generic/metrics/metrics_collector.go (1)
IncreaseWorkProcessedCounter(353-359)pkg/cloudevents/clients/common/common.go (2)
ManifestWorkGR(37-37)CloudEventsDataTypeAnnotationKey(14-14)pkg/cloudevents/clients/utils/utils.go (1)
Patch(44-82)pkg/cloudevents/generic/types/types.go (1)
ParseCloudEventsDataType(193-204)
pkg/cloudevents/clients/event/client.go (1)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)
pkg/cloudevents/clients/csr/client.go (2)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/clients/store/interface.go (1)
ClientWatcherStore(32-63)
pkg/cloudevents/clients/work/source/client/manifestwork.go (2)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/generic/metrics/metrics_collector.go (1)
IncreaseWorkProcessedCounter(353-359)
pkg/cloudevents/generic/clients/agentclient.go (5)
pkg/cloudevents/generic/interface.go (4)
ResourceObject(20-34)Lister(36-39)CloudEventsClient(55-74)ResourceHandler(15-15)pkg/cloudevents/generic/options/options.go (3)
CloudEventsAgentOptions(64-77)CloudEventsOptions(16-28)EventRateLimit(39-47)pkg/cloudevents/generic/utils/ratelimiter.go (1)
NewRateLimiter(22-34)pkg/cloudevents/generic/metrics/metrics_collector.go (3)
IncreaseCloudEventsSentFromAgentCounter(308-320)IncreaseCloudEventsReceivedByAgentCounter(285-293)UpdateResourceStatusResyncDurationMetric(334-342)pkg/cloudevents/generic/types/types.go (2)
CloudEventsDataType(165-169)ParseCloudEventsType(209-230)
test/integration/cloudevents/suite_test.go (1)
pkg/cloudevents/generic/clients/baseclient.go (1)
DelayFn(29-35)
pkg/cloudevents/clients/csr/client_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
test/integration/cloudevents/certrotation_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
test/integration/cloudevents/source/client.go (1)
pkg/cloudevents/generic/clients/sourceclient.go (1)
NewCloudEventSourceClient(42-68)
pkg/cloudevents/clients/cluster/client.go (3)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/clients/store/interface.go (1)
ClientWatcherStore(32-63)vendor/open-cluster-management.io/api/client/cluster/clientset/versioned/typed/cluster/v1/managedcluster.go (1)
ManagedClusterInterface(24-36)
pkg/cloudevents/clients/addon/client_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
pkg/cloudevents/clients/lease/client_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
pkg/cloudevents/generic/clients/baseclient.go (2)
pkg/cloudevents/generic/metrics/metrics_collector.go (1)
IncreaseClientReconnectedCounter(345-350)pkg/cloudevents/generic/utils/ratelimiter.go (1)
LongThrottleLatency(14-14)
pkg/cloudevents/generic/testing/resource.go (1)
pkg/cloudevents/generic/types/types.go (3)
CloudEventsDataType(165-169)ListOptions(151-162)CloudEventsType(176-185)
pkg/cloudevents/clients/options/generic.go (4)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/generic/options/builder/optionsbuilder.go (2)
BuildCloudEventsAgentOptions(65-74)BuildCloudEventsSourceOptions(53-62)pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)pkg/cloudevents/generic/clients/sourceclient.go (1)
NewCloudEventSourceClient(42-68)
pkg/cloudevents/clients/event/client_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
pkg/cloudevents/generic/clients/sourceclient_test.go (3)
pkg/cloudevents/generic/testing/resource.go (5)
MockResource(22-29)NewMockResourceLister(47-51)StatusHash(57-59)NewMockResourceCodec(63-65)MockEventDataType(16-20)pkg/cloudevents/generic/types/types.go (4)
CloudEventsType(176-185)SubResourceStatus(33-33)CloudEventsDataType(165-169)ResourceAction(87-87)pkg/cloudevents/generic/clients/sourceclient.go (1)
NewCloudEventSourceClient(42-68)
pkg/cloudevents/generic/clients/agentclient_test.go (3)
pkg/cloudevents/generic/testing/resource.go (5)
MockResource(22-29)NewMockResourceLister(47-51)StatusHash(57-59)NewMockResourceCodec(63-65)MockEventDataType(16-20)pkg/cloudevents/generic/types/types.go (4)
CloudEventsType(176-185)SubResourceSpec(30-30)CloudEventsDataType(165-169)ResourceAction(87-87)pkg/cloudevents/generic/clients/agentclient.go (2)
NewCloudEventAgentClient(42-69)CloudEventAgentClient(26-33)
pkg/cloudevents/generic/clients/sourceclient.go (5)
pkg/cloudevents/generic/interface.go (3)
ResourceObject(20-34)Lister(36-39)ResourceHandler(15-15)pkg/cloudevents/generic/options/options.go (3)
CloudEventsSourceOptions(50-61)CloudEventsOptions(16-28)EventRateLimit(39-47)pkg/cloudevents/generic/utils/ratelimiter.go (1)
NewRateLimiter(22-34)pkg/cloudevents/generic/metrics/metrics_collector.go (3)
IncreaseCloudEventsSentFromSourceCounter(296-305)IncreaseCloudEventsReceivedBySourceCounter(273-282)UpdateResourceSpecResyncDurationMetric(323-331)pkg/cloudevents/generic/types/types.go (1)
CloudEventsDataType(165-169)
pkg/cloudevents/generic/metrics/metrics_collector.go (4)
vendor/github.com/prometheus/client_golang/prometheus/counter.go (1)
NewCounterVec(194-199)vendor/github.com/prometheus/client_golang/prometheus/histogram.go (1)
NewHistogramVec(1179-1184)vendor/github.com/prometheus/client_golang/prometheus/registry.go (2)
MustRegister(176-178)Registerer(96-134)vendor/github.com/prometheus/client_golang/prometheus/labels.go (1)
Labels(33-33)
pkg/cloudevents/generic/clients/clients_metrics_test.go (5)
pkg/cloudevents/generic/testing/resource.go (5)
MockResource(22-29)MockEventDataType(16-20)NewMockResourceLister(47-51)StatusHash(57-59)NewMockResourceCodec(63-65)pkg/cloudevents/generic/types/types.go (6)
SubResourceSpec(30-30)EventAction(37-37)CloudEventsDataType(165-169)ResyncRequestAction(41-41)ResyncResponseAction(44-44)SubResourceStatus(33-33)pkg/cloudevents/generic/metrics/metrics_collector.go (10)
ResetSourceCloudEventsMetrics(257-262)ResetClientCloudEventsMetrics(265-270)CloudeventsSentFromSourceCounterMetric(128-135)CloudeventsReceivedByClientCounterMetric(115-122)ClientReconnectedCounterMetric(222-229)CloudeventsReceivedBySourceCounterMetric(100-107)ResourceSpecResyncDurationMetric(166-182)ResourceStatusResyncDurationMetric(200-216)CloudeventsSentFromClientCounterMetric(141-148)NoneOriginalSource(29-29)pkg/cloudevents/generic/clients/sourceclient.go (1)
NewCloudEventSourceClient(42-68)pkg/cloudevents/generic/clients/agentclient.go (2)
NewCloudEventAgentClient(42-69)CloudEventAgentClient(26-33)
test/integration/cloudevents/heartbeat_reconnect_test.go (1)
pkg/cloudevents/generic/clients/baseclient.go (1)
DelayFn(29-35)
⏰ 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: integration
- GitHub Check: unit
- GitHub Check: verify
🔇 Additional comments (28)
pkg/cloudevents/generic/options/builder/optionsbuilder_test.go (1)
1-1: LGTM! Package rename aligns with directory structure.The package declaration correctly matches the directory name, following Go conventions. This change is consistent with the broader package reorganization described in the PR objectives.
pkg/cloudevents/generic/utils/ratelimiter.go (1)
1-34: LGTM! Package reorganization looks good.The package rename from
generictoutilsappropriately reflects the utility nature of this code, and exportingLongThrottleLatencymakes it accessible for other packages that need the same throttle latency threshold.pkg/cloudevents/generic/clients/sourceclient_test.go (1)
1-451: LGTM! Test updates are consistent with the package refactoring.The test file has been properly updated to use the new
clientspackage and thegenerictestingutilities for mock resources. All constructor calls and type references have been correctly updated.pkg/cloudevents/generic/clients/agentclient.go (1)
1-342: LGTM! Well-executed refactoring to use the CloudEventsClient interface.The migration from concrete types to the
generic.CloudEventsClient[T]interface improves flexibility and aligns with the broader package reorganization. The metrics instrumentation has been properly updated to use the exportedmetricspackage functions, and the rate limiter initialization correctly uses the newutilspackage.pkg/cloudevents/generic/metrics/metrics_collector.go (1)
1-359: LGTM! Metrics API properly exposed as public.The package rename to
metricsand the export of all metric collectors and helper functions successfully creates a public metrics API. The consistent use of UpperCamelCase for exported names follows Go conventions.pkg/cloudevents/clients/work/source/client/manifestwork.go (2)
30-30: LGTM! Interface-based field type improves flexibility.Changing from the concrete
*generic.CloudEventSourceClient[*workv1.ManifestWork]to thegeneric.CloudEventsClient[*workv1.ManifestWork]interface allows for better abstraction and testability.
24-24: LGTM! Metrics instrumentation properly migrated.All
IncreaseWorkProcessedCountercalls have been correctly updated to use the exportedmetricspackage functions across all operation paths (create, delete, get, list, watch, patch).Also applies to: 63-295
pkg/cloudevents/generic/options/builder/optionsbuilder.go (1)
1-74: LGTM! Package rename aligns with organizational structure.The package rename from
generictobuilderappropriately groups option builder utilities together and clarifies the purpose of this package.test/integration/cloudevents/source/client.go (1)
17-17: LGTM! Integration test properly updated for new package structure.The import path and constructor call have been correctly updated to use the
clientspackage, maintaining all existing functionality.Also applies to: 29-45
pkg/cloudevents/clients/addon/client_test.go (1)
20-20: LGTM! Test properly updated for new package structure.The import path and constructor call have been correctly updated to reference the
clientspackage. The test logic remains unchanged and should continue to work as expected.Also applies to: 70-78
pkg/cloudevents/clients/addon/client.go (1)
26-26: LGTM! Interface adoption improves design.The refactor from concrete type
*generic.CloudEventAgentClient[...]to interface typegeneric.CloudEventsClient[...]follows the dependency inversion principle and improves testability.Also applies to: 34-34
pkg/cloudevents/clients/event/client_test.go (1)
16-16: LGTM! Package reorganization applied consistently.The import path and function call updates align with the package restructuring from
generictogeneric/clients.Also applies to: 37-42, 92-97
test/integration/cloudevents/suite_test.go (1)
20-20: LGTM! DelayFn reference updated correctly.The import and usage of
DelayFnhave been correctly updated to reference the newclientspackage location.Also applies to: 73-73
pkg/cloudevents/clients/cluster/client_test.go (1)
21-21: LGTM! Consistent with package reorganization.The import path and constructor usage have been updated to align with the new
generic/clientspackage structure.Also applies to: 61-66, 120-125
pkg/cloudevents/clients/csr/client_test.go (1)
19-19: LGTM! Package reorganization applied consistently.The import and constructor call align with the package restructuring to
generic/clients.Also applies to: 65-70
test/integration/cloudevents/heartbeat_reconnect_test.go (2)
15-15: LGTM! Package reorganization applied correctly.The import path and
DelayFnreference have been updated to align with the newgeneric/clientspackage structure.Also applies to: 211-211
334-344: Verify the removal of early break in stabilization loop.The stabilization loop no longer breaks early after detecting 500ms of stability. Instead, it now runs for the full 2-second duration. This extends the verification period but changes the test behavior.
Was this intentional to ensure stability is maintained throughout the entire period, or was the
breakaccidentally removed during refactoring?test/integration/cloudevents/certrotation_test.go (1)
18-18: LGTM! Package reorganization applied consistently.The import path and constructor usage align with the package restructuring to
generic/clients.Also applies to: 68-74
pkg/cloudevents/generic/clients/baseclient.go (1)
1-1: LGTM! Metrics and utilities properly exposed as public APIs.The refactor successfully:
- Renames the package from
generictoclients- Exposes metrics via
metrics.IncreaseClientReconnectedCounter- Exposes utility constants via
utils.LongThrottleLatencyThis improves the public API surface while maintaining functionality.
Also applies to: 17-17, 20-20, 78-78, 123-123
pkg/cloudevents/clients/lease/client.go (1)
23-23: LGTM: Field type updated to use interface.The change from a concrete pointer type to the
CloudEventsClientinterface improves testability and follows interface-based design principles.pkg/cloudevents/clients/lease/client_test.go (2)
14-14: LGTM: Import updated to new package structure.The import path correctly reflects the reorganization to the
generic/clientssubpackage.
39-44: LGTM: Test constructor calls updated with new signature.Both test cases correctly use the new
NewCloudEventAgentClientconstructor with the additionalstatusHashGetterandcodecparameters, aligning with the refactored API.Also applies to: 91-96
pkg/cloudevents/clients/event/client.go (1)
22-22: LGTM: Consistent interface adoption.The field and constructor parameter types are correctly updated to use the
CloudEventsClientinterface, consistent with the broader refactoring pattern.Also applies to: 26-26
pkg/cloudevents/clients/csr/client.go (1)
26-26: LGTM: Interface-based design applied consistently.The CSRClient follows the same refactoring pattern as other clients, updating both the field and constructor parameter to use the
CloudEventsClientinterface.Also applies to: 33-33
pkg/cloudevents/clients/cluster/client.go (1)
27-27: LGTM: Refactoring completed consistently.The ManagedClusterClient correctly adopts the
CloudEventsClientinterface for both its field and constructor parameter, completing the consistent refactoring across all client implementations.Also applies to: 34-34
pkg/cloudevents/generic/clients/clients_metrics_test.go (3)
1-24: LGTM! Package reorganization looks good.The package rename and import updates align well with the refactoring objectives. The new
metricsandgenerictestingpackages are properly imported and organized.
291-372: LGTM! Test logic and metrics usage are correct.The status resync test properly uses the refactored
generictestingutilities andmetricsAPIs. The type assertion on line 345 is necessary to access the internalreceivemethod for testing purposes.
136-289: LGTM! Remaining tests and helpers are well-structured.The reconnect and spec resync tests properly use the refactored testing utilities and metrics APIs. The helper functions for extracting metric values are solid and unchanged.
Also applies to: 374-412
Signed-off-by: Wei Liu <liuweixa@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/cloudevents/generic/testing/resource.go (1)
106-113: Fix deletion timestamp decoding for nativetime.Timeextensions
When we encode a mock event we store the deletion timestamp as atime.Time. In tests (and in-memory transports likegochan) the receiver sees that native type, sofmt.Sprintf("%s", ts)produces"2025-10-30 07:20:17 +0000 UTC", which fails RFC3339 parsing and drops delete events such as theTestReceiveResourceSpec“delete a resource” case. The CloudEvents SDK explicitly permits extensions to surface as nativetime.Timevalues, so the decoder must accept both native times and canonical strings.(pkg.go.dev)
Please switch to a type switch that handlestime.Time,*time.Time, and string inputs before falling back to an error.- deletionTimestamp, err := evt.Context.GetExtension("deletiontimestamp") - if err == nil { - timestamp, err := time.Parse(time.RFC3339, fmt.Sprintf("%s", deletionTimestamp)) - if err != nil { - return nil, fmt.Errorf("failed to parse deletiontimestamp - %v to time.Time", deletionTimestamp) - } - res.DeletionTimestamp = &metav1.Time{Time: timestamp} - } + if deletionTimestamp, err := evt.Context.GetExtension("deletiontimestamp"); err == nil { + switch ts := deletionTimestamp.(type) { + case time.Time: + res.DeletionTimestamp = &metav1.Time{Time: ts} + case *time.Time: + res.DeletionTimestamp = &metav1.Time{Time: *ts} + case string: + timestamp, err := time.Parse(time.RFC3339, ts) + if err != nil { + return nil, fmt.Errorf("failed to parse deletiontimestamp %q to time.Time: %w", ts, err) + } + res.DeletionTimestamp = &metav1.Time{Time: timestamp} + default: + return nil, fmt.Errorf("unsupported deletiontimestamp type %T", deletionTimestamp) + } + }
🧹 Nitpick comments (1)
pkg/cloudevents/generic/clients/sourceclient.go (1)
135-140: Guard cluster-name extraction against missing/invalid extensionsLine [136] dereferences the
clusternameextension with a direct type assertion. If a codec implementation forgets to set that extension or sets it to a non-string value, we’ll panic in production rather than returning a clear error. Suggest returning an error when the extension is absent or malformed so the caller can see what went wrong instead of crashing.- clusterName := evt.Context.GetExtensions()[types.ExtensionClusterName].(string) - metrics.IncreaseCloudEventsSentFromSourceCounter(evt.Source(), clusterName, eventType.CloudEventsDataType.String(), string(eventType.SubResource), string(eventType.Action)) + clusterExt, ok := evt.Context.GetExtensions()[types.ExtensionClusterName] + if !ok { + return fmt.Errorf("missing %q extension on spec event", types.ExtensionClusterName) + } + + clusterName, err := cloudeventstypes.ToString(clusterExt) + if err != nil { + return fmt.Errorf("invalid %q extension on spec event: %w", types.ExtensionClusterName, err) + } + + metrics.IncreaseCloudEventsSentFromSourceCounter(evt.Source(), clusterName, eventType.CloudEventsDataType.String(), string(eventType.SubResource), string(eventType.Action))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
pkg/cloudevents/clients/addon/client.go(1 hunks)pkg/cloudevents/clients/addon/client_test.go(2 hunks)pkg/cloudevents/clients/cluster/client.go(1 hunks)pkg/cloudevents/clients/cluster/client_test.go(3 hunks)pkg/cloudevents/clients/csr/client.go(1 hunks)pkg/cloudevents/clients/csr/client_test.go(2 hunks)pkg/cloudevents/clients/event/client.go(1 hunks)pkg/cloudevents/clients/event/client_test.go(3 hunks)pkg/cloudevents/clients/lease/client.go(1 hunks)pkg/cloudevents/clients/lease/client_test.go(3 hunks)pkg/cloudevents/clients/options/generic.go(5 hunks)pkg/cloudevents/clients/work/agent/client/manifestwork.go(10 hunks)pkg/cloudevents/clients/work/source/client/manifestwork.go(12 hunks)pkg/cloudevents/generic/clients/agentclient.go(7 hunks)pkg/cloudevents/generic/clients/agentclient_test.go(22 hunks)pkg/cloudevents/generic/clients/baseclient.go(4 hunks)pkg/cloudevents/generic/clients/clients_metrics_test.go(11 hunks)pkg/cloudevents/generic/clients/sourceclient.go(8 hunks)pkg/cloudevents/generic/clients/sourceclient_test.go(20 hunks)pkg/cloudevents/generic/metrics/metrics_collector.go(11 hunks)pkg/cloudevents/generic/options/builder/optionsbuilder.go(1 hunks)pkg/cloudevents/generic/options/builder/optionsbuilder_test.go(2 hunks)pkg/cloudevents/generic/options/options.go(3 hunks)pkg/cloudevents/generic/ratelimiter.go(0 hunks)pkg/cloudevents/generic/testing/resource.go(1 hunks)pkg/cloudevents/generic/utils/ratelimiter.go(1 hunks)test/integration/cloudevents/certrotation_test.go(2 hunks)test/integration/cloudevents/heartbeat_reconnect_test.go(2 hunks)test/integration/cloudevents/source/client.go(2 hunks)test/integration/cloudevents/suite_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/cloudevents/generic/ratelimiter.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/cloudevents/clients/csr/client_test.go
- pkg/cloudevents/clients/cluster/client_test.go
- test/integration/cloudevents/heartbeat_reconnect_test.go
- pkg/cloudevents/generic/options/builder/optionsbuilder_test.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-17T13:29:00.675Z
Learnt from: qiujian16
PR: open-cluster-management-io/sdk-go#147
File: pkg/cloudevents/generic/options/grpc/protocol/heartbeat_integration_test.go:66-68
Timestamp: 2025-09-17T13:29:00.675Z
Learning: In the pkg/cloudevents/generic/options/grpc/protocol package, bufSize is already defined as a constant in protocal_test.go and is accessible to other test files in the same package, including heartbeat_integration_test.go.
Applied to files:
test/integration/cloudevents/suite_test.go
📚 Learning: 2025-09-01T03:34:05.141Z
Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#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/generic/clients/baseclient.gopkg/cloudevents/clients/work/agent/client/manifestwork.gopkg/cloudevents/generic/clients/clients_metrics_test.gopkg/cloudevents/generic/metrics/metrics_collector.go
📚 Learning: 2025-09-16T02:22:20.929Z
Learnt from: skeeey
PR: open-cluster-management-io/sdk-go#144
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:200-213
Timestamp: 2025-09-16T02:22:20.929Z
Learning: In the GRPC CloudEvents protocol implementation, when startEventsReceiver encounters a stream error, it sends the error to reconnectErrorChan. The consumer of this channel handles the error by calling Close() on the protocol, which triggers close(p.closeChan), causing OpenInbound to unblock and call cancel() to properly terminate both the events receiver and heartbeat watcher goroutines.
Applied to files:
pkg/cloudevents/generic/clients/baseclient.gopkg/cloudevents/generic/clients/sourceclient_test.gopkg/cloudevents/generic/clients/agentclient_test.go
📚 Learning: 2025-09-16T06:03:45.232Z
Learnt from: skeeey
PR: open-cluster-management-io/sdk-go#144
File: pkg/cloudevents/server/grpc/broker.go:169-194
Timestamp: 2025-09-16T06:03:45.232Z
Learning: In the Open Cluster Management gRPC CloudEvents implementation, client reconnections due to heartbeat timeout are considered acceptable behavior by the maintainer skeeey, rather than something that needs optimization to prevent.
Applied to files:
pkg/cloudevents/generic/clients/baseclient.go
📚 Learning: 2025-09-12T06:43:05.679Z
Learnt from: qiujian16
PR: open-cluster-management-io/sdk-go#140
File: pkg/server/grpc/server_test.go:260-265
Timestamp: 2025-09-12T06:43:05.679Z
Learning: In the gRPC Go library, grpc.NewClient was introduced in v1.63 and is the recommended way to create gRPC client connections. grpc.Dial and grpc.DialContext are deprecated as of v1.63, though they will be supported throughout the 1.x series. NewClient uses "dns" as the default name resolver while the deprecated Dial functions use "passthrough" for backward compatibility.
Applied to files:
test/integration/cloudevents/source/client.go
🧬 Code graph analysis (23)
pkg/cloudevents/clients/csr/client.go (2)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/clients/store/interface.go (1)
ClientWatcherStore(32-63)
test/integration/cloudevents/suite_test.go (1)
pkg/cloudevents/generic/clients/baseclient.go (1)
DelayFn(29-35)
pkg/cloudevents/generic/options/options.go (1)
pkg/cloudevents/generic/utils/ratelimiter.go (1)
EventRateLimit(21-29)
pkg/cloudevents/generic/clients/baseclient.go (2)
pkg/cloudevents/generic/metrics/metrics_collector.go (1)
IncreaseClientReconnectedCounter(345-350)pkg/cloudevents/generic/utils/ratelimiter.go (1)
LongThrottleLatency(12-12)
pkg/cloudevents/clients/work/source/client/manifestwork.go (4)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/generic/metrics/metrics_collector.go (1)
IncreaseWorkProcessedCounter(353-359)pkg/cloudevents/clients/common/common.go (2)
ManifestWorkGR(37-37)ManifestWorkGK(36-36)pkg/cloudevents/clients/errors/errors.go (1)
ToStatusError(18-30)
pkg/cloudevents/generic/testing/resource.go (1)
pkg/cloudevents/generic/types/types.go (3)
CloudEventsDataType(165-169)ListOptions(151-162)CloudEventsType(176-185)
test/integration/cloudevents/source/client.go (1)
pkg/cloudevents/generic/clients/sourceclient.go (1)
NewCloudEventSourceClient(42-68)
pkg/cloudevents/generic/clients/sourceclient_test.go (2)
pkg/cloudevents/generic/testing/resource.go (5)
MockResource(22-29)NewMockResourceLister(47-51)StatusHash(57-59)NewMockResourceCodec(63-65)MockEventDataType(16-20)pkg/cloudevents/generic/clients/sourceclient.go (1)
NewCloudEventSourceClient(42-68)
pkg/cloudevents/clients/event/client_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
pkg/cloudevents/clients/work/agent/client/manifestwork.go (5)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/generic/metrics/metrics_collector.go (1)
IncreaseWorkProcessedCounter(353-359)pkg/cloudevents/clients/common/common.go (2)
ManifestWorkGR(37-37)CloudEventsDataTypeAnnotationKey(14-14)pkg/cloudevents/clients/utils/utils.go (1)
Patch(44-82)pkg/cloudevents/generic/types/types.go (1)
ParseCloudEventsDataType(193-204)
pkg/cloudevents/clients/cluster/client.go (3)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/clients/store/interface.go (1)
ClientWatcherStore(32-63)vendor/open-cluster-management.io/api/client/cluster/clientset/versioned/typed/cluster/v1/managedcluster.go (1)
ManagedClusterInterface(24-36)
test/integration/cloudevents/certrotation_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
pkg/cloudevents/generic/clients/clients_metrics_test.go (5)
pkg/cloudevents/generic/testing/resource.go (5)
MockResource(22-29)MockEventDataType(16-20)NewMockResourceLister(47-51)StatusHash(57-59)NewMockResourceCodec(63-65)pkg/cloudevents/generic/types/types.go (6)
SubResourceSpec(30-30)EventAction(37-37)CloudEventsDataType(165-169)ResyncRequestAction(41-41)ResyncResponseAction(44-44)SubResourceStatus(33-33)pkg/cloudevents/generic/metrics/metrics_collector.go (10)
ResetSourceCloudEventsMetrics(257-262)ResetClientCloudEventsMetrics(265-270)CloudeventsSentFromSourceCounterMetric(128-135)CloudeventsReceivedByClientCounterMetric(115-122)ClientReconnectedCounterMetric(222-229)CloudeventsReceivedBySourceCounterMetric(100-107)ResourceSpecResyncDurationMetric(166-182)ResourceStatusResyncDurationMetric(200-216)CloudeventsSentFromClientCounterMetric(141-148)NoneOriginalSource(29-29)pkg/cloudevents/generic/clients/sourceclient.go (1)
NewCloudEventSourceClient(42-68)pkg/cloudevents/generic/clients/agentclient.go (2)
NewCloudEventAgentClient(42-69)CloudEventAgentClient(26-33)
pkg/cloudevents/clients/lease/client.go (1)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)
pkg/cloudevents/clients/addon/client_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
pkg/cloudevents/generic/metrics/metrics_collector.go (4)
vendor/github.com/prometheus/client_golang/prometheus/counter.go (1)
NewCounterVec(194-199)vendor/github.com/prometheus/client_golang/prometheus/histogram.go (1)
NewHistogramVec(1179-1184)vendor/github.com/prometheus/client_golang/prometheus/registry.go (2)
MustRegister(176-178)Registerer(96-134)vendor/github.com/prometheus/client_golang/prometheus/labels.go (1)
Labels(33-33)
pkg/cloudevents/generic/clients/agentclient_test.go (3)
pkg/cloudevents/generic/testing/resource.go (5)
MockResource(22-29)NewMockResourceLister(47-51)StatusHash(57-59)NewMockResourceCodec(63-65)MockEventDataType(16-20)pkg/cloudevents/generic/types/types.go (4)
CloudEventsType(176-185)SubResourceSpec(30-30)CloudEventsDataType(165-169)ResourceAction(87-87)pkg/cloudevents/generic/clients/agentclient.go (2)
NewCloudEventAgentClient(42-69)CloudEventAgentClient(26-33)
pkg/cloudevents/clients/event/client.go (1)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)
pkg/cloudevents/clients/options/generic.go (4)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/generic/options/builder/optionsbuilder.go (2)
BuildCloudEventsAgentOptions(65-74)BuildCloudEventsSourceOptions(53-62)pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)pkg/cloudevents/generic/clients/sourceclient.go (1)
NewCloudEventSourceClient(42-68)
pkg/cloudevents/clients/lease/client_test.go (1)
pkg/cloudevents/generic/clients/agentclient.go (1)
NewCloudEventAgentClient(42-69)
pkg/cloudevents/generic/clients/sourceclient.go (5)
pkg/cloudevents/generic/interface.go (3)
ResourceObject(20-34)Lister(36-39)ResourceHandler(15-15)pkg/cloudevents/generic/options/options.go (2)
CloudEventsSourceOptions(40-51)CloudEventsOptions(17-29)pkg/cloudevents/generic/utils/ratelimiter.go (2)
NewRateLimiter(31-43)EventRateLimit(21-29)pkg/cloudevents/generic/metrics/metrics_collector.go (3)
IncreaseCloudEventsSentFromSourceCounter(296-305)IncreaseCloudEventsReceivedBySourceCounter(273-282)UpdateResourceSpecResyncDurationMetric(323-331)pkg/cloudevents/generic/types/types.go (1)
CloudEventsDataType(165-169)
pkg/cloudevents/generic/clients/agentclient.go (5)
pkg/cloudevents/generic/interface.go (4)
ResourceObject(20-34)Lister(36-39)CloudEventsClient(55-74)ResourceHandler(15-15)pkg/cloudevents/generic/options/options.go (2)
CloudEventsAgentOptions(54-67)CloudEventsOptions(17-29)pkg/cloudevents/generic/utils/ratelimiter.go (2)
NewRateLimiter(31-43)EventRateLimit(21-29)pkg/cloudevents/generic/metrics/metrics_collector.go (3)
IncreaseCloudEventsSentFromAgentCounter(308-320)IncreaseCloudEventsReceivedByAgentCounter(285-293)UpdateResourceStatusResyncDurationMetric(334-342)pkg/cloudevents/generic/types/types.go (2)
CloudEventsDataType(165-169)ParseCloudEventsType(209-230)
pkg/cloudevents/clients/addon/client.go (4)
pkg/cloudevents/generic/interface.go (1)
CloudEventsClient(55-74)pkg/cloudevents/clients/store/interface.go (1)
ClientWatcherStore(32-63)vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1alpha1/managedclusteraddon.go (1)
ManagedClusterAddOnInterface(24-36)pkg/cloudevents/clients/addon/wrapper.go (1)
ManagedClusterAddOnInterface(59-68)
⏰ 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: verify
- GitHub Check: integration
- GitHub Check: unit
🔇 Additional comments (15)
test/integration/cloudevents/certrotation_test.go (1)
18-74: Constructor update aligns with new generic client API.The switch to
clients.NewCloudEventAgentClientwith the extended signature keeps the cert rotation test wired correctly.test/integration/cloudevents/suite_test.go (1)
20-75: Delay override tracks the new clients package surface.Pointing
DelayFnto the accelerated test lambda after the refactor keeps the suite behavior intact.pkg/cloudevents/generic/utils/ratelimiter.go (1)
1-43: Centralized rate-limiter helper looks solid.Default fallbacks and token bucket creation match the client-go pattern, giving callers a consistent utility.
pkg/cloudevents/clients/work/agent/client/manifestwork.go (1)
25-262: Interface adoption and metrics refactor check out.Switching to
generic.CloudEventsClient[*workv1.ManifestWork]keeps Publish/Resync usage intact, and the metrics move preserves the counter semantics.pkg/cloudevents/clients/work/source/client/manifestwork.go (1)
24-295: Source client updates mirror the intended abstraction shift.The CloudEvents interface and metrics package changes keep the spec-side workflows and instrumentation consistent.
pkg/cloudevents/clients/options/generic.go (1)
12-240: Options helper cleanly transitions to the new builders and interface.The builder-based option construction and interface return type preserve subscriber/resync behavior with the refactored clients.
pkg/cloudevents/clients/csr/client.go (1)
26-26: LGTM! Interface abstraction improves testability.The change from concrete
*generic.CloudEventAgentClienttogeneric.CloudEventsClientinterface follows the dependency inversion principle and makes the CSRClient more testable by allowing mock implementations to be injected.Also applies to: 33-33
pkg/cloudevents/generic/options/builder/optionsbuilder.go (1)
1-1: LGTM! Package reorganization improves structure.The package rename from
generictobuilderaligns with the directory structure and improves code organization by grouping builder-related functionality.pkg/cloudevents/clients/addon/client_test.go (1)
20-20: LGTM! Test updated correctly for package reorganization.The import path and constructor call have been properly updated to reference the new
generic/clientspackage location. The constructor arguments remain unchanged, maintaining test correctness.Also applies to: 70-75
pkg/cloudevents/clients/event/client_test.go (1)
16-16: LGTM! Tests updated consistently for package reorganization.Both
TestCreateandTestPatchhave been properly updated with the new import path and constructor reference, maintaining test correctness.Also applies to: 37-42, 92-97
pkg/cloudevents/generic/clients/baseclient.go (2)
1-1: LGTM! Package reorganization with proper separation of concerns.The package rename to
clientsand the addition ofmetricsandutilsimports support the extraction of cross-cutting concerns into dedicated packages, improving code organization and reusability.Also applies to: 17-17, 20-20
78-78: LGTM! Proper usage of extracted metrics and utils APIs.The function call and constant reference have been correctly updated to use the newly extracted
metricsandutilspackages, maintaining the same behavior while improving code organization.Also applies to: 123-123
pkg/cloudevents/clients/lease/client.go (1)
23-23: LGTM! Consistent interface abstraction.The change to
generic.CloudEventsClientinterface is consistent with the broader refactoring pattern and improves testability through dependency injection.pkg/cloudevents/clients/lease/client_test.go (1)
14-14: LGTM! Tests properly updated with complete constructor arguments.Both test functions have been correctly updated with the new import path and constructor reference, providing all required arguments including the status hash getter and codec.
Also applies to: 39-44, 91-96
pkg/cloudevents/generic/options/options.go (1)
9-9: LGTM! Type consolidation improves reusability.Moving
EventRateLimitto theutilspackage and updating bothCloudEventsSourceOptionsandCloudEventsAgentOptionsto use the shared type eliminates duplication and centralizes rate limit configuration.Also applies to: 50-50, 66-66
|
/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 |
b492d8b
into
open-cluster-management-io:main
2 participants
Summary
CloudEventsClientinterface instead ofCloudEventAgentClientandCloudEventSourceClientRelated issue(s)
Fixes #
Summary by CodeRabbit