🌱 Switch grpc addon client to v1beta1 - #212
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 |
WalkthroughReplaces v1alpha1 addon client usage with v1beta1 in the cloudevents addon client wrapper; updates imports and Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
LGTM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cloudevents/clients/addon/wrapper.go (1)
31-33:⚠️ Potential issue | 🟡 Minor
AddonV1alpha1()returns nil, risking obscure NPE.Since
ManagedClusterAddOnInterfacenow only populatesbetaClient, calls toAddonV1alpha1()will returnnil. Any subsequent method call (e.g.,.ManagedClusterAddOns(ns).Get(...)) will cause a nil pointer dereference at a confusing call site.For consistency with other unsupported methods like
Discovery()and those inAddonClientWrapper(context snippet 3), consider panicking with a clear message:Proposed fix
func (a AddonClientSetWrapper) AddonV1alpha1() addonv1alpha1client.AddonV1alpha1Interface { + if a.alphaClient == nil { + panic("AddonV1alpha1 is unsupported") + } return a.alphaClient }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/addon/wrapper.go` around lines 31 - 33, The AddonV1alpha1 method currently returns a potentially nil alphaClient which can cause obscure nil-pointer panics; update AddonClientSetWrapper.AddonV1alpha1 to instead panic with a clear message stating that v1alpha1 is unsupported (consistent with how Discovery() and unsupported methods in AddonClientWrapper behave) so callers get an explicit failure instead of a nil return; reference AddonClientSetWrapper.AddonV1alpha1, alphaClient, betaClient and the existing patterns in AddonClientWrapper/Discovery() when implementing the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/cloudevents/clients/addon/wrapper.go`:
- Around line 31-33: The AddonV1alpha1 method currently returns a potentially
nil alphaClient which can cause obscure nil-pointer panics; update
AddonClientSetWrapper.AddonV1alpha1 to instead panic with a clear message
stating that v1alpha1 is unsupported (consistent with how Discovery() and
unsupported methods in AddonClientWrapper behave) so callers get an explicit
failure instead of a nil return; reference AddonClientSetWrapper.AddonV1alpha1,
alphaClient, betaClient and the existing patterns in
AddonClientWrapper/Discovery() when implementing the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cd22813-d5ca-4706-9abc-3856449a9458
📒 Files selected for processing (1)
pkg/cloudevents/clients/addon/wrapper.go
560c4fa to
6ed1ccb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cloudevents/clients/addon/wrapper.go (1)
31-33:⚠️ Potential issue | 🟡 Minor
AddonV1alpha1()will return nil, but no callers in the codebase are affected.
ManagedClusterAddOnInterface()only initializesbetaClient(lines 49-51), leavingalphaClientas nil. CallingAddonV1alpha1()on the returned wrapper will return nil. However, a search of the codebase found no calls toAddonV1alpha1(), so no existing code breaks.That said, this violates the interface contract defined by
addonclientset.Interface. Consider either initializingalphaClientor adding a panic with a clear error message (consistent with theDiscovery()method) to prevent silent nil-pointer failures if the method is called in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/addon/wrapper.go` around lines 31 - 33, The AddonV1alpha1 method currently returns a possibly nil alphaClient because ManagedClusterAddOnInterface initializes only betaClient; update the wrapper creation to also initialize alphaClient (set AddonClientSetWrapper.alphaClient alongside betaClient) or, to match Discovery(), change AddonV1alpha1 to check for nil and panic with a clear error message if alphaClient is nil so callers fail fast; refer to AddonV1alpha1, ManagedClusterAddOnInterface, alphaClient, betaClient and Discovery() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cloudevents/clients/addon/wrapper.go`:
- Around line 40-51: ManagedClusterAddOnInterface currently constructs an
AddonClientSetWrapper but only sets betaClient leaving alphaClient nil which
will make AddonV1alpha1() crash; fix by also constructing and assigning the
alpha client before returning the wrapper (i.e. call the alpha API client
creation like you do for beta: obtain an alpha AgentClient via the appropriate
GenericClientOptions for the alpha ManagedClusterAddOn, create the alpha
ManagedClusterAddOn client (e.g. v1alpha1.NewManagedClusterAddOnClient(...)) and
set AddonClientSetWrapper.alphaClient), or if an alpha options object isn’t
available then update AddonV1alpha1() on AddonClientSetWrapper to defensively
return a safe no-op/empty client or a clear error instead of returning the nil
pointer.
---
Outside diff comments:
In `@pkg/cloudevents/clients/addon/wrapper.go`:
- Around line 31-33: The AddonV1alpha1 method currently returns a possibly nil
alphaClient because ManagedClusterAddOnInterface initializes only betaClient;
update the wrapper creation to also initialize alphaClient (set
AddonClientSetWrapper.alphaClient alongside betaClient) or, to match
Discovery(), change AddonV1alpha1 to check for nil and panic with a clear error
message if alphaClient is nil so callers fail fast; refer to AddonV1alpha1,
ManagedClusterAddOnInterface, alphaClient, betaClient and Discovery() when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e82aedc-bf6d-4b63-b71d-79289948cef1
📒 Files selected for processing (1)
pkg/cloudevents/clients/addon/wrapper.go
6ed1ccb to
cf2bc1f
Compare
Signed-off-by: Jian Qiu <jqiu@redhat.com>
cf2bc1f to
3ee69eb
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/cloudevents/clients/addon/wrapper.go (1)
39-50:⚠️ Potential issue | 🔴 CriticalBeta-only construction still returns a partially initialized
addonclientset.Interface.This factory now populates only
betaClient, butAddonClientSetWrapperstill exposesAddonV1alpha1()and returnsa.alphaClientdirectly on Lines 30-31. Any caller that treats this as a full clientset and reaches for the alpha accessor will getniland fail at runtime. Either initialize the alpha side as well, or stop returning the fulladdonclientset.Interfacefrom this helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/addon/wrapper.go` around lines 39 - 50, ManagedClusterAddOnInterface currently builds only the beta side (betaClient) and returns an AddonClientSetWrapper that still exposes AddonV1alpha1(), causing alphaClient to be nil at runtime; fix by either initializing alphaClient in the AddonClientSetWrapper before returning or change the function signature/return to not present a full addonclientset.Interface (e.g., return a beta-only type). Locate ManagedClusterAddOnInterface and AddonClientSetWrapper and ensure alphaClient is constructed (mirror how betaClient is created) or adjust callers/signature so AddonV1alpha1() cannot be called on a partially initialized wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/cloudevents/clients/addon/wrapper.go`:
- Around line 39-50: ManagedClusterAddOnInterface currently builds only the beta
side (betaClient) and returns an AddonClientSetWrapper that still exposes
AddonV1alpha1(), causing alphaClient to be nil at runtime; fix by either
initializing alphaClient in the AddonClientSetWrapper before returning or change
the function signature/return to not present a full addonclientset.Interface
(e.g., return a beta-only type). Locate ManagedClusterAddOnInterface and
AddonClientSetWrapper and ensure alphaClient is constructed (mirror how
betaClient is created) or adjust callers/signature so AddonV1alpha1() cannot be
called on a partially initialized wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aaae7199-5360-4f84-b50c-e6d5d6733946
📒 Files selected for processing (2)
pkg/cloudevents/clients/addon/wrapper.gopkg/cloudevents/clients/errors/errors.go
|
/lgtm |
f98f8ca
into
open-cluster-management-io:main
* 🌱 Fix non-deterministic golangci-lint results for revive linter Disable issue count limits (max-issues-per-linter, max-same-issues) to prevent golangci-lint from randomly dropping issues, which causes flaky CI results. Also exclude the revive var-naming rule for stdlib package name conflicts, which is non-deterministic due to concurrent analysis in golangci-lint. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: xuezhaojun <zxue@redhat.com> * 🌱 Document revive flaky behavior and exclusion rule rationale Add documentation for the non-deterministic revive linter behavior discovered during PR #212 review, including upstream references and the rationale for each customized exclusion rule. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: xuezhaojun <zxue@redhat.com> --------- Signed-off-by: xuezhaojun <zxue@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
Refactor
Bug Fixes