🌱 Update cloudevent client with addon v1beta1 api - #176
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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds a v1beta1 CloudEvents addon client, codec, and tests; refactors and renames v1alpha1 addon client components and wrappers to support alpha/beta paths; updates wrapper usage and imports; and bumps the open-cluster-management.io/api dependency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/cloudevents/server/grpc/authz/kube/sar.go (1)
198-204: Missing v1beta1 addon event data type in authorization switch.The switch only handles
v1alpha1.ManagedClusterAddOnEventDataType. With the introduction of v1beta1 addon clients, events usingv1beta1.ManagedClusterAddOnEventDataTypewill fall through to the default case and be rejected as "unsupported event type".Consider adding v1beta1 support:
+ addonv1beta1 "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/v1beta1"case v1alpha1.ManagedClusterAddOnEventDataType, + addonv1beta1.ManagedClusterAddOnEventDataType, csr.CSREventDataType, event.EventEventDataType, lease.LeaseEventDataType:If v1beta1 addon events are not expected to go through this authorization path, please clarify the intended design.
pkg/cloudevents/clients/addon/v1alpha1/client.go (1)
44-47: Namespace method mutates receiver, risking shared state issues.The
Namespacemethod modifiesc.namespacein place and returns the same pointer. If the sameManagedClusterAddOnClientinstance is used concurrently with different namespaces, this could lead to race conditions.Consider returning a new instance to avoid shared state:
func (c *ManagedClusterAddOnClient) Namespace(namespace string) *ManagedClusterAddOnClient { - c.namespace = namespace - return c + return &ManagedClusterAddOnClient{ + cloudEventsClient: c.cloudEventsClient, + watcherStore: c.watcherStore, + namespace: namespace, + } }
🧹 Nitpick comments (7)
pkg/cloudevents/server/grpc/authz/kube/sar.go (1)
6-7: Import ordering: standard library imports should be grouped together.The new import breaks the conventional Go import grouping. Consider moving it to the appropriate group.
import ( "context" "fmt" - "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/v1alpha1" "sync" "google.golang.org/grpc"Then add it to the open-cluster-management.io import group:
"open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/v1alpha1" "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster"pkg/cloudevents/clients/addon/v1alpha1/client_test.go (2)
74-80: Consider usingt.Fatalfor setup errors.Using
t.Errorin setup code (lines 75, 79) allows the test to continue executing even when critical setup fails, which can lead to confusing nil-pointer panics or misleading error messages.if err != nil { - t.Error(err) + t.Fatal(err) } addonClient := NewAddonClientWrapper(NewManagedClusterAddOnClient(ceClient, watcherStore)) if err := watcherStore.Store.Add(c.addon); err != nil { - t.Error(err) + t.Fatal(err) }
82-91: Test only verifies no error; consider asserting patch result.The test verifies that
Patchdoesn't return an error but doesn't validate that the addon was actually patched with the expected status. Consider retrieving the addon after patching and asserting the status was updated.pkg/cloudevents/clients/addon/v1beta1/client_test.go (1)
75-81: Consider usingt.Fatalfor setup errors.Same as v1alpha1 test: using
t.Errorin setup code allows the test to continue even when critical setup fails.if err != nil { - t.Error(err) + t.Fatal(err) } addonClient := NewAddonClientWrapper(NewManagedClusterAddOnClient(ceClient, watcherStore)) if err := watcherStore.Store.Add(c.addon); err != nil { - t.Error(err) + t.Fatal(err) }pkg/cloudevents/clients/addon/v1beta1/codec.go (2)
3-10: Consider grouping imports by origin.The imports mix standard library, k8s, and external packages without clear grouping. Standard Go convention groups imports as: standard library, external packages, and local packages.
import ( "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - addonapiv1beta1 "open-cluster-management.io/api/addon/v1beta1" cloudevents "github.com/cloudevents/sdk-go/v2" + + addonapiv1beta1 "open-cluster-management.io/api/addon/v1beta1" "open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types" )
58-65: Consider validating event data type in Decode for consistency.
Encodevalidates that the event type matchesManagedClusterAddOnEventDataType, butDecodedoes not perform similar validation. While this may be acceptable if events are pre-filtered, adding validation would make the codec more robust against misuse.func (c *ManagedClusterAddOnCodec) Decode(evt *cloudevents.Event) (*addonapiv1beta1.ManagedClusterAddOn, error) { + eventType, err := types.ParseCloudEventsType(evt.Type()) + if err != nil { + return nil, fmt.Errorf("failed to parse cloud event type %s, %v", evt.Type(), err) + } + if eventType.CloudEventsDataType != ManagedClusterAddOnEventDataType { + return nil, fmt.Errorf("unsupported cloudevents data type %s", eventType.CloudEventsDataType) + } + addon := &addonapiv1beta1.ManagedClusterAddOn{} if err := evt.DataAs(addon); err != nil { return nil, fmt.Errorf("failed to unmarshal event data %s, %v", string(evt.Data()), err) } return addon, nil }pkg/cloudevents/clients/addon/wrapper.go (1)
44-47: Consider documenting the incomplete v1beta1 initialization.The TODO indicates this is intentional, but consumers calling
AddonV1beta1()will get a nil pointer. Consider adding a comment to theAddonClientSetWrapperstruct orAddonV1beta1()method indicating the current limitation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/types_clustermanagementaddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/types_managedclusteraddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/doc.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/register.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/types_clustermanagementaddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/types_managedclusteraddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/clientset.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/scheme/register.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/doc.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/work/v1alpha1/types_manifestworkreplicaset.gois excluded by!vendor/**
📒 Files selected for processing (9)
go.mod(1 hunks)pkg/cloudevents/clients/addon/v1alpha1/client.go(2 hunks)pkg/cloudevents/clients/addon/v1alpha1/client_test.go(2 hunks)pkg/cloudevents/clients/addon/v1alpha1/codec.go(1 hunks)pkg/cloudevents/clients/addon/v1beta1/client.go(1 hunks)pkg/cloudevents/clients/addon/v1beta1/client_test.go(1 hunks)pkg/cloudevents/clients/addon/v1beta1/codec.go(1 hunks)pkg/cloudevents/clients/addon/wrapper.go(3 hunks)pkg/cloudevents/server/grpc/authz/kube/sar.go(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-07T12:29:10.858Z
Learnt from: skeeey
Repo: open-cluster-management-io/sdk-go PR: 129
File: go.mod:3-3
Timestamp: 2025-07-07T12:29:10.858Z
Learning: Starting with Go 1.18, the `go` directive in `go.mod` files supports patch versions (e.g., `go 1.23.6`). This is valid syntax and will work correctly with Go tooling.
Applied to files:
go.mod
📚 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/server/grpc/authz/kube/sar.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/server/grpc/authz/kube/sar.go
🧬 Code graph analysis (3)
pkg/cloudevents/server/grpc/authz/kube/sar.go (2)
pkg/cloudevents/clients/addon/v1alpha1/codec.go (1)
ManagedClusterAddOnEventDataType(13-17)pkg/cloudevents/clients/addon/v1beta1/codec.go (1)
ManagedClusterAddOnEventDataType(12-16)
pkg/cloudevents/clients/addon/v1alpha1/client_test.go (3)
pkg/cloudevents/clients/addon/v1alpha1/client.go (2)
NewAddonClientWrapper(153-155)NewManagedClusterAddOnClient(34-42)pkg/cloudevents/clients/addon/v1beta1/client.go (2)
NewAddonClientWrapper(155-157)NewManagedClusterAddOnClient(32-40)pkg/cloudevents/clients/utils/utils.go (1)
Patch(44-82)
pkg/cloudevents/clients/addon/wrapper.go (2)
pkg/cloudevents/clients/addon/v1alpha1/client.go (3)
AddonClientWrapper(147-149)NewManagedClusterAddOnClient(34-42)NewAddonClientWrapper(153-155)pkg/cloudevents/clients/addon/v1beta1/client.go (3)
AddonClientWrapper(149-151)NewManagedClusterAddOnClient(32-40)NewAddonClientWrapper(155-157)
⏰ 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: unit
- GitHub Check: integration
🔇 Additional comments (3)
pkg/cloudevents/clients/addon/v1alpha1/codec.go (1)
1-67: LGTM!The package rename to
v1alpha1properly organizes the versioned addon client code. The codec correctly usesaddonapiv1alpha1.GroupVersionfor the event data type definition and handles encoding/decoding appropriately.pkg/cloudevents/clients/addon/v1alpha1/client.go (1)
146-175: AddonClientWrapper implementation looks good.The wrapper correctly implements
AddonV1alpha1Interface, delegating to the underlying client forManagedClusterAddOnsand panicking for unsupported methods. This is consistent with the v1beta1 wrapper approach.pkg/cloudevents/clients/addon/v1beta1/client.go (1)
109-146: Patch implementation looks correct.The Patch method properly validates that only status subresource patches are allowed, applies the patch, and publishes the update via CloudEvents. The error handling and event type construction are appropriate.
| k8s.io/klog/v2 v2.130.1 | ||
| k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 | ||
| open-cluster-management.io/api v1.1.1-0.20251124092621-2337d27c3b7f | ||
| open-cluster-management.io/api v1.1.1-0.20251215032811-ee922fbb996c |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n go.mod | sed -n '35,45p'Repository: open-cluster-management-io/sdk-go
Length of output: 457
🏁 Script executed:
rg "v1beta1" --type go -A 2 -B 2 | head -50Repository: open-cluster-management-io/sdk-go
Length of output: 3635
🏁 Script executed:
rg "open-cluster-management.io/api" --type go | head -20Repository: open-cluster-management-io/sdk-go
Length of output: 2566
Dependency update provides v1beta1 addon types, but implementation remains incomplete.
The pseudo-version includes the required v1beta1 addon API types from open-cluster-management.io/api. However, the addon client wrapper still has a TODO comment and only initializes the alpha client; the betaClient field is not yet populated in NewAddonClientSetWrapper. The dependency change is correct, but complete the switch to v1beta1 in pkg/cloudevents/clients/addon/wrapper.go.
8222fd1 to
a5bbc6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/cloudevents/clients/addon/wrapper.go (1)
33-38: Nil-check added butbetaClientis never initialized.The nil-check panic (addressing the previous review comment) provides a clear error message when
AddonV1beta1()is called. However, sincebetaClientis never initialized inManagedClusterAddOnInterface, any call toAddonV1beta1()will always panic.This appears intentional given the TODO on line 49. Consider documenting this limitation in the struct or method comment.
🧹 Nitpick comments (2)
pkg/cloudevents/clients/addon/v1beta1/client_test.go (2)
36-54: Consider usingt.Fatalfor setup errors that should abort the test.Errors during patch data generation (lines 38, 48, 53) will allow the test to continue with invalid/nil data. Using
t.Fatalwould immediately stop the test on setup failures.oldData, err := json.Marshal(old) if err != nil { - t.Error(err) + t.Fatal(err) } new := old.DeepCopy() new.Status = addonapiv1beta1.ManagedClusterAddOnStatus{ Namespace: "install", } newData, err := json.Marshal(new) if err != nil { - t.Error(err) + t.Fatal(err) } patchBytes, err := jsonpatch.CreateMergePatch(oldData, newData) if err != nil { - t.Error(err) + t.Fatal(err) }
74-80: Uset.Fatalfor setup errors in test body.Errors creating the CloudEvent client (line 75) or adding to the watcher store (line 79) are setup failures that should stop the test immediately rather than continuing with a nil client or missing data.
if err != nil { - t.Error(err) + t.Fatal(err) } addonClient := NewAddonClientWrapper(NewManagedClusterAddOnClient(ceClient, watcherStore)) if err := watcherStore.Store.Add(c.addon); err != nil { - t.Error(err) + t.Fatal(err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/types_clustermanagementaddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/types_managedclusteraddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/doc.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/register.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/types_clustermanagementaddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/types_managedclusteraddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/clientset.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/scheme/register.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/doc.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/work/v1alpha1/types_manifestworkreplicaset.gois excluded by!vendor/**
📒 Files selected for processing (9)
go.mod(1 hunks)pkg/cloudevents/clients/addon/v1alpha1/client.go(4 hunks)pkg/cloudevents/clients/addon/v1alpha1/client_test.go(2 hunks)pkg/cloudevents/clients/addon/v1alpha1/codec.go(1 hunks)pkg/cloudevents/clients/addon/v1beta1/client.go(1 hunks)pkg/cloudevents/clients/addon/v1beta1/client_test.go(1 hunks)pkg/cloudevents/clients/addon/v1beta1/codec.go(1 hunks)pkg/cloudevents/clients/addon/wrapper.go(3 hunks)pkg/cloudevents/server/grpc/authz/kube/sar.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cloudevents/server/grpc/authz/kube/sar.go
- pkg/cloudevents/clients/addon/v1beta1/codec.go
- pkg/cloudevents/clients/addon/v1alpha1/client_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-07T12:29:10.858Z
Learnt from: skeeey
Repo: open-cluster-management-io/sdk-go PR: 129
File: go.mod:3-3
Timestamp: 2025-07-07T12:29:10.858Z
Learning: Starting with Go 1.18, the `go` directive in `go.mod` files supports patch versions (e.g., `go 1.23.6`). This is valid syntax and will work correctly with Go tooling.
Applied to files:
go.mod
🧬 Code graph analysis (4)
pkg/cloudevents/clients/addon/v1beta1/client_test.go (4)
pkg/cloudevents/generic/options/fake/fakeoptions.go (1)
NewEventChan(35-40)pkg/cloudevents/clients/store/lister.go (1)
NewAgentWatcherStoreLister(15-19)pkg/cloudevents/clients/addon/v1beta1/codec.go (1)
NewManagedClusterAddOnCodec(21-23)pkg/cloudevents/clients/addon/v1beta1/client.go (2)
NewAddonClientWrapper(158-160)NewManagedClusterAddOnClient(32-40)
pkg/cloudevents/clients/addon/wrapper.go (2)
pkg/cloudevents/clients/addon/v1alpha1/client.go (3)
AddonClientWrapper(150-152)NewManagedClusterAddOnClient(34-42)NewAddonClientWrapper(156-158)pkg/cloudevents/clients/addon/v1beta1/client.go (3)
AddonClientWrapper(152-154)NewManagedClusterAddOnClient(32-40)NewAddonClientWrapper(158-160)
pkg/cloudevents/clients/addon/v1alpha1/client.go (2)
pkg/cloudevents/clients/addon/v1beta1/client.go (3)
ManagedClusterAddOnClient(24-28)AddonClientWrapper(152-154)NewAddonClientWrapper(158-160)pkg/cloudevents/clients/addon/wrapper.go (1)
ManagedClusterAddOnInterface(41-51)
pkg/cloudevents/clients/addon/v1beta1/client.go (6)
pkg/cloudevents/clients/store/interface.go (1)
ClientWatcherStore(32-63)pkg/cloudevents/clients/addon/wrapper.go (1)
ManagedClusterAddOnInterface(41-51)pkg/cloudevents/clients/common/common.go (1)
ManagedClusterAddOnGR(43-43)pkg/patcher/patcher.go (1)
PatchOptions(34-37)pkg/cloudevents/clients/addon/v1beta1/codec.go (1)
ManagedClusterAddOnEventDataType(12-16)pkg/cloudevents/clients/errors/errors.go (1)
ToStatusError(18-30)
⏰ 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: verify
- GitHub Check: integration
🔇 Additional comments (9)
go.mod (1)
40-40: Verify that implementation inwrapper.gohas been completed.The dependency version bump appears valid, but a previous review flagged that the addon client wrapper had incomplete initialization—specifically, the
betaClientfield inNewAddonClientSetWrapperwas not populated despite the dependency providing v1beta1 types. Confirm thatpkg/cloudevents/clients/addon/wrapper.gohas been fully updated to initialize both alpha and beta clients.pkg/cloudevents/clients/addon/v1alpha1/codec.go (1)
1-1: LGTM - Package rename aligns with version separation.The package rename from
addontov1alpha1correctly organizes the code to support both v1alpha1 and v1beta1 API versions in separate packages.pkg/cloudevents/clients/addon/v1alpha1/client.go (2)
44-50: Good refactor - Namespace now returns a new instance.This immutable pattern avoids shared-state issues when the same client is used concurrently.
148-178: LGTM - AddonClientWrapper properly bridges to the interface.The wrapper correctly implements
AddonV1alpha1Interfacewith panic stubs for unsupported methods and delegatesManagedClusterAddOnsto the underlying client.pkg/cloudevents/clients/addon/wrapper.go (1)
47-50: LGTM - v1alpha1 client wiring is correct.The alpha client is properly initialized. The TODO comment clarifies that v1beta1 support will be added later.
pkg/cloudevents/clients/addon/v1beta1/client.go (4)
42-48: Good - Namespace returns a new instance avoiding shared state.This addresses the concurrency concern from the previous review.
71-83: Structured logging is correctly implemented.The logging now properly uses key-value pairs with
klog.FromContext(ctx).
112-149: Patch implementation looks correct.The method properly:
- Validates status subresource is present
- Fetches current state from watcher store
- Applies the patch
- Publishes CloudEvents update request
- Handles errors appropriately
151-172: LGTM - AddonClientWrapper correctly implements AddonV1beta1Interface.The comment is now correct (references v1beta1), and the wrapper properly delegates to the underlying client.
Signed-off-by: Jian Qiu <jqiu@redhat.com>
a5bbc6a to
5d18af6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cloudevents/clients/addon/v1beta1/client_test.go (1)
19-94: LGTM! Solid test coverage for v1beta1 client.The test correctly validates the Patch operation for v1beta1 ManagedClusterAddOn, using v1beta1 types consistently throughout. The structure mirrors the proven v1alpha1 test pattern.
The v1alpha1 and v1beta1 tests share significant structure. Consider extracting a shared test helper function in a future refactor to reduce duplication:
// Future optional refactor func testPatchHelper[T any](t *testing.T, codec Codec[T], wrapper func(*Client[T]) Interface) { // shared test logic }This is entirely optional and can be deferred to a future cleanup PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/types_clustermanagementaddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/types_managedclusteraddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/doc.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/register.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/types_clustermanagementaddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/types_managedclusteraddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/clientset.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/scheme/register.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/doc.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.gois excluded by!vendor/**vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.gois excluded by!vendor/**vendor/open-cluster-management.io/api/work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/work/v1alpha1/types_manifestworkreplicaset.gois excluded by!vendor/**
📒 Files selected for processing (9)
go.mod(1 hunks)pkg/cloudevents/clients/addon/v1alpha1/client.go(7 hunks)pkg/cloudevents/clients/addon/v1alpha1/client_test.go(2 hunks)pkg/cloudevents/clients/addon/v1alpha1/codec.go(1 hunks)pkg/cloudevents/clients/addon/v1beta1/client.go(1 hunks)pkg/cloudevents/clients/addon/v1beta1/client_test.go(1 hunks)pkg/cloudevents/clients/addon/v1beta1/codec.go(1 hunks)pkg/cloudevents/clients/addon/wrapper.go(3 hunks)pkg/cloudevents/server/grpc/authz/kube/sar.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cloudevents/server/grpc/authz/kube/sar.go
- go.mod
- pkg/cloudevents/clients/addon/v1beta1/client.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/cloudevents/clients/addon/v1alpha1/client.go (2)
pkg/cloudevents/clients/addon/v1beta1/client.go (3)
ManagedClusterAddOnClient(24-28)AddonClientWrapper(152-154)NewAddonClientWrapper(158-160)pkg/cloudevents/clients/addon/wrapper.go (1)
ManagedClusterAddOnInterface(41-51)
pkg/cloudevents/clients/addon/v1beta1/codec.go (2)
pkg/cloudevents/generic/types/types.go (4)
CloudEventsDataType(210-214)CloudEventsType(221-230)NewEventBuilder(288-293)ExtensionResourceVersion(64-64)pkg/cloudevents/clients/addon/v1alpha1/codec.go (3)
NewManagedClusterAddOnCodec(22-24)ManagedClusterAddOnCodec(20-20)ManagedClusterAddOnEventDataType(13-17)
pkg/cloudevents/clients/addon/wrapper.go (2)
pkg/cloudevents/clients/addon/v1alpha1/client.go (3)
AddonClientWrapper(154-156)NewManagedClusterAddOnClient(34-42)NewAddonClientWrapper(160-162)pkg/cloudevents/clients/addon/v1beta1/client.go (3)
AddonClientWrapper(152-154)NewManagedClusterAddOnClient(32-40)NewAddonClientWrapper(158-160)
⏰ 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: verify
- GitHub Check: unit
🔇 Additional comments (8)
pkg/cloudevents/clients/addon/v1alpha1/codec.go (1)
1-1: LGTM! Clean package reorganization.The package rename to
v1alpha1properly aligns with the version-based directory structure and enables clean separation between alpha and beta API versions.pkg/cloudevents/clients/addon/v1alpha1/client_test.go (1)
77-82: LGTM! Test correctly adapted to new wrapper architecture.The test now properly exercises the new
AddonClientWrapperpattern, usingManagedClusterAddOns(namespace)to access the namespace-scoped client interface.pkg/cloudevents/clients/addon/v1alpha1/client.go (3)
44-50: LGTM! Namespace method correctly preserves immutability.The updated implementation returns a new
ManagedClusterAddOnClientinstance with the namespace set, rather than mutating the receiver. This is a good practice that prevents unexpected side effects when the same client is used for multiple namespaces.
74-75: LGTM! Structured logging properly implemented.The switch to
klog.FromContext(ctx)with structured key-value pairs provides better observability and log parsing capabilities. The context-aware logging also enables correlation with distributed tracing when configured.Also applies to: 88-89, 104-105, 116-117
153-182: LGTM! Clean adapter pattern for version-specific interface.The
AddonClientWrapperprovides a clean bridge between the CloudEvents-based client and the legacyAddonV1alpha1Interface. The panic stubs for unsupported operations are acceptable since this client is specifically designed for CloudEvents-based agent communication, not general-purpose REST operations.pkg/cloudevents/clients/addon/wrapper.go (2)
17-38: LGTM! Clean dual-version wrapper structure.The refactored
AddonClientSetWrapperproperly supports both v1alpha1 and v1beta1 interfaces. The nil check onbetaClient(lines 34-36) appropriately guards against premature access before v1beta1 is initialized.
47-50: TODO is appropriate for gradual v1beta1 migration.The TODO comment clearly documents the planned migration path. The current alpha-only initialization is correct for this transitional phase, allowing beta support to be added incrementally.
pkg/cloudevents/clients/addon/v1beta1/codec.go (1)
1-66: LGTM! Solid v1beta1 codec implementation.The codec correctly implements CloudEvents encoding/decoding for v1beta1 ManagedClusterAddOn resources. The implementation appropriately mirrors the proven v1alpha1 pattern while using v1beta1 types throughout:
- Event data type properly references v1beta1 GroupVersion
- Encode validates data type and embeds resource metadata
- ResourceVersion extension handling preserves version information
- TypeMeta stamping ensures proper API version/kind in events
- Error handling provides clear diagnostics
The code duplication between v1alpha1 and v1beta1 codecs is expected and acceptable for version-specific CloudEvents encoding.
|
/lgtm |
72dfdeb
into
open-cluster-management-io:main
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.