✨ upgrade addon API to v1beta1 #407
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haoqing0110 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds a new addon v1beta1 API: CRDs, Go types, deepcopy gen, clients (real & fake), informers, listers, tests, scheme registration, and build/test tooling updates; includes small v1alpha1 deprecation/validation edits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (38)
🚧 Files skipped from review as they are similar to previous changes (18)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-12-01T01:10:01.745ZApplied to files:
📚 Learning: 2025-07-02T14:09:55.079ZApplied to files:
🧬 Code graph analysis (7)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_managedclusteraddon.go (5)
test/integration/api/managedclusteraddon_test.go (2)
client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go (4)
client/addon/informers/externalversions/generic.go (2)
client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go (7)
test/integration/api/clustermanagementaddon_test.go (3)
addon/v1beta1/zz_generated.deepcopy.go (2)
⏰ 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). (1)
🔇 Additional comments (19)
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: 7
🧹 Nitpick comments (3)
addon/v1alpha1/types_managedclusteraddon.go (1)
57-66: Ensure deprecation message clarity for API consumers.The deprecation messages reference "type: kubeClient and type: csr in v1beta1" without further explanation. While this aligns with the enhancement PR (#162) mentioned in the PR description, consider whether the message should include a reference or brief explanation of the replacement mechanism, especially if API consumers may not be familiar with the upcoming v1beta1 changes.
Alternative (if more context is appropriate):
- // Deprecated: Will be replaced with type: kubeClient and type: csr in v1beta1. - // signerName is the name of signer that addon agent will use to create csr. + // Deprecated: Will be replaced with type: kubeClient and type: csr in v1beta1. + // See https://github.com/open-cluster-management-io/enhancements/pull/162 for migration details. + // signerName is the name of signer that addon agent will use to create csr.(Adjust reference link as needed based on your project's documentation standards.)
addon/v1beta1/types_managedclusteraddon.go (2)
59-74: Inconsistent required field annotation withomitempty.The
Typefield is marked with+requiredbut the JSON tag includesomitempty, which creates an inconsistency. Required fields should typically not haveomitemptyto ensure they are always serialized.type RegistrationConfig struct { // type specifies the type of registration configuration. // +kubebuilder:validation:Enum=KubeClient;CSR // +required - Type RegistrationType `json:"type,omitempty"` + Type RegistrationType `json:"type"`Alternatively, if the field is intended to be optional with a default, the
+requiredannotation should be removed.
111-122: Consider adding+requiredannotation oromitemptyforUserfield.The
Subjectstruct hasUseras a non-optional field (noomitemptyin JSON tag), but the struct documentation indicates a default subject is used if not set. If the field is truly required whenSubjectis specified, this is fine. Otherwise, consider addingomitemptyfor consistency with optional behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
Makefile(1 hunks)addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(1 hunks)addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml(2 hunks)addon/v1alpha1/types_clustermanagementaddon.go(1 hunks)addon/v1alpha1/types_managedclusteraddon.go(1 hunks)addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(1 hunks)addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml(1 hunks)addon/v1beta1/doc.go(1 hunks)addon/v1beta1/register.go(1 hunks)addon/v1beta1/types_clustermanagementaddon.go(1 hunks)addon/v1beta1/types_managedclusteraddon.go(1 hunks)addon/v1beta1/zz_generated.deepcopy.go(1 hunks)client/addon/clientset/versioned/clientset.go(3 hunks)client/addon/clientset/versioned/fake/clientset_generated.go(2 hunks)client/addon/clientset/versioned/fake/register.go(1 hunks)client/addon/clientset/versioned/scheme/register.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/doc.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/doc.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_managedclusteraddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go(1 hunks)client/addon/informers/externalversions/addon/interface.go(2 hunks)client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/informers/externalversions/addon/v1beta1/interface.go(1 hunks)client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go(1 hunks)client/addon/informers/externalversions/generic.go(2 hunks)client/addon/listers/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/listers/addon/v1beta1/expansion_generated.go(1 hunks)client/addon/listers/addon/v1beta1/managedclusteraddon.go(1 hunks)hack/lib/init.sh(2 hunks)hack/verify-crds.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-02T14:09:55.079Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 380
File: client/work/clientset/versioned/fake/clientset_generated.go:40-40
Timestamp: 2025-07-02T14:09:55.079Z
Learning: Generated code files marked with "Code generated by client-gen. DO NOT EDIT." should not be manually edited as they are automatically generated by Kubernetes code-generator tools and any changes would be overwritten.
Applied to files:
client/addon/clientset/versioned/typed/addon/v1beta1/doc.goclient/addon/listers/addon/v1beta1/expansion_generated.goclient/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go
📚 Learning: 2025-09-02T11:59:50.703Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 392
File: operator/v1/types_clustermanager.go:178-195
Timestamp: 2025-09-02T11:59:50.703Z
Learning: XValidation markers in kubebuilder require Kubernetes 1.25+ and should not be added to the open-cluster-management-io/api project until minimum Kubernetes version requirements are established to avoid breaking compatibility with older clusters.
Applied to files:
addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml
🧬 Code graph analysis (13)
client/addon/clientset/versioned/fake/register.go (9)
addon/v1beta1/register.go (1)
AddToScheme(22-22)client/addon/clientset/versioned/scheme/register.go (1)
AddToScheme(38-38)addon/v1alpha1/register.go (1)
AddToScheme(22-22)client/work/clientset/versioned/fake/register.go (1)
AddToScheme(38-38)client/cluster/clientset/versioned/fake/register.go (1)
AddToScheme(42-42)client/work/clientset/versioned/scheme/register.go (1)
AddToScheme(38-38)client/cluster/clientset/versioned/scheme/register.go (1)
AddToScheme(42-42)client/operator/clientset/versioned/fake/register.go (1)
AddToScheme(36-36)client/operator/clientset/versioned/scheme/register.go (1)
AddToScheme(36-36)
client/addon/clientset/versioned/fake/clientset_generated.go (3)
client/addon/clientset/versioned/clientset.go (1)
Clientset(24-28)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
AddonV1beta1Interface(14-18)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go (1)
FakeAddonV1beta1(12-14)
client/addon/informers/externalversions/addon/v1beta1/interface.go (5)
client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go (1)
ClusterManagementAddOnInformer(22-25)client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (1)
ManagedClusterAddOnInformer(22-25)client/addon/clientset/versioned/clientset.go (2)
New(111-118)Interface(17-21)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
New(69-71)client/addon/informers/externalversions/addon/interface.go (2)
New(27-29)Interface(13-18)
client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go (4)
addon/v1alpha1/types_clustermanagementaddon.go (2)
ClusterManagementAddOn(22-33)ClusterManagementAddOnList(253-262)addon/v1beta1/types_clustermanagementaddon.go (2)
ClusterManagementAddOn(21-32)ClusterManagementAddOnList(214-223)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go (1)
FakeAddonV1beta1(12-14)addon/v1beta1/register.go (1)
SchemeGroupVersion(19-19)
client/addon/informers/externalversions/generic.go (2)
addon/v1beta1/register.go (1)
SchemeGroupVersion(19-19)addon/v1alpha1/register.go (1)
SchemeGroupVersion(19-19)
client/addon/clientset/versioned/clientset.go (1)
client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (4)
AddonV1beta1Interface(14-18)AddonV1beta1Client(21-23)NewForConfigAndClient(48-56)New(69-71)
client/addon/informers/externalversions/addon/interface.go (5)
client/addon/clientset/versioned/clientset.go (2)
Interface(17-21)New(111-118)client/addon/informers/externalversions/addon/v1beta1/interface.go (2)
Interface(11-16)New(25-27)client/addon/informers/externalversions/addon/v1alpha1/interface.go (2)
Interface(11-20)New(29-31)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
New(69-71)client/addon/clientset/versioned/typed/addon/v1alpha1/addon_client.go (1)
New(79-81)
client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (4)
client/addon/listers/addon/v1beta1/managedclusteraddon.go (2)
ManagedClusterAddOnLister(15-22)NewManagedClusterAddOnLister(30-32)client/addon/clientset/versioned/clientset.go (1)
Interface(17-21)client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
Interface(11-16)addon/v1beta1/types_managedclusteraddon.go (1)
ManagedClusterAddOn(19-32)
client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go (3)
client/addon/listers/addon/v1beta1/clustermanagementaddon.go (2)
ClusterManagementAddOnLister(15-23)NewClusterManagementAddOnLister(31-33)client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
Interface(11-16)addon/v1beta1/types_clustermanagementaddon.go (1)
ClusterManagementAddOn(21-32)
addon/v1beta1/register.go (4)
client/addon/clientset/versioned/fake/register.go (1)
AddToScheme(38-38)client/addon/clientset/versioned/scheme/register.go (2)
AddToScheme(38-38)Scheme(16-16)addon/v1beta1/types_clustermanagementaddon.go (2)
ClusterManagementAddOn(21-32)ClusterManagementAddOnList(214-223)addon/v1beta1/types_managedclusteraddon.go (2)
ManagedClusterAddOn(19-32)ManagedClusterAddOnList(243-248)
client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go (7)
addon/v1beta1/types_managedclusteraddon.go (2)
ManagedClusterAddOn(19-32)ManagedClusterAddOnList(243-248)client/addon/clientset/versioned/clientset.go (1)
Interface(17-21)client/addon/informers/externalversions/addon/interface.go (1)
Interface(13-18)client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
Interface(11-16)client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go (1)
ManagedClusterAddOnExpansion(8-8)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
AddonV1beta1Client(21-23)client/addon/clientset/versioned/scheme/register.go (1)
ParameterCodec(18-18)
addon/v1beta1/types_managedclusteraddon.go (2)
addon/v1alpha1/types_clustermanagementaddon.go (4)
ConfigGroupResource(105-116)ConfigReferent(119-130)AddOnMeta(66-74)ConfigSpecHash(133-139)addon/v1beta1/register.go (1)
Resource(27-29)
client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (6)
client/addon/clientset/versioned/clientset.go (2)
Interface(17-21)New(111-118)client/addon/informers/externalversions/addon/interface.go (2)
Interface(13-18)New(27-29)client/addon/informers/externalversions/addon/v1beta1/interface.go (2)
Interface(11-16)New(25-27)client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go (2)
ClusterManagementAddOnsGetter(19-21)ClusterManagementAddOnInterface(24-36)client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go (2)
ManagedClusterAddOnsGetter(19-21)ManagedClusterAddOnInterface(24-36)client/addon/clientset/versioned/scheme/register.go (2)
Scheme(16-16)Codecs(17-17)
⏰ 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). (1)
- GitHub Check: integration
🔇 Additional comments (35)
client/addon/clientset/versioned/typed/addon/v1beta1/fake/doc.go (1)
1-5: LGTM!Standard generated documentation file for the v1beta1 fake client package with proper copyright, generation markers, and package documentation.
addon/v1alpha1/types_managedclusteraddon.go (1)
57-58: Deprecation comments are sufficient; kubebuilder does not support field-level deprecation markers.Kubebuilder/controller-gen does not provide a built-in marker (e.g.,
+kubebuilder:deprecated) for field-level deprecation. The tool only supports version-level deprecation via+kubebuilder:deprecatedversion. Your approach of adding deprecation comments is the correct and standard pattern for indicating deprecated fields in Kubernetes API Go code. If you needdeprecated: truein the generated CRD OpenAPI schema, that requires post-processing the CRD YAML or a custom build pipeline step, not Go markers.client/addon/clientset/versioned/typed/addon/v1beta1/doc.go (1)
1-5: LGTM - Standard generated doc file.This is a correctly generated doc.go file for the v1beta1 typed addon client package. Based on learnings, this generated file should not be manually edited.
client/addon/clientset/versioned/fake/register.go (1)
13-13: LGTM - v1beta1 scheme registration follows established pattern.The import and scheme builder registration for
addonv1beta1.AddToSchemecorrectly mirrors the v1alpha1 pattern and is consistent with other fake register files in the codebase (e.g.,client/work/clientset/versioned/fake/register.go,client/cluster/clientset/versioned/fake/register.go).Also applies to: 19-22
client/addon/clientset/versioned/fake/clientset_generated.go (1)
16-17: LGTM - v1beta1 fake clientset wiring is correct.The new
AddonV1beta1()method follows the same pattern asAddonV1alpha1()and correctly wires the fake client. This aligns with theClientsetstruct inclient/addon/clientset/versioned/clientset.gowhich now includes bothaddonV1alpha1andaddonV1beta1fields.Also applies to: 82-86
client/addon/listers/addon/v1beta1/expansion_generated.go (1)
1-16: LGTM - Standard lister expansion interfaces.These empty interfaces provide extension points for custom lister methods without modifying generated code. This follows the standard Kubernetes lister-gen pattern.
client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_managedclusteraddon.go (1)
1-37: LGTM - Correctly generated fake ManagedClusterAddOn client.The fake client properly uses
gentype.FakeClientWithListwith correct resource (managedclusteraddons), kind (ManagedClusterAddOn), and list conversion helpers. This follows the modern client-gen pattern.client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go (1)
1-29: LGTM - FakeAddonV1beta1 correctly implements the interface.The fake client properly implements
AddonV1beta1Interfacewith:
ClusterManagementAddOns()for cluster-scoped resourcesManagedClusterAddOns(namespace)for namespace-scoped resourcesRESTClient()returning nil (standard for fake clients)This aligns with the interface defined in
client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go.client/addon/clientset/versioned/clientset.go (1)
14-14: LGTM! v1beta1 client integration follows established patterns.The new
AddonV1beta1client is correctly wired into the clientset with proper error handling inNewForConfigAndClientand consistent initialization across all constructor paths (NewForConfig,NewForConfigAndClient,NewForConfigOrDie,New).Also applies to: 20-20, 27-27, 35-38, 88-91, 114-114
client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go (1)
1-37: LGTM! Fake client implementation is correct.The fake client for
ClusterManagementAddOncorrectly implements the cluster-scoped resource pattern (empty namespace on line 22) and properly wires all factory functions for thegentype.FakeClientWithListgeneric type.addon/v1beta1/register.go (1)
1-41: LGTM! API registration scaffolding is complete and correct.The registration file properly:
- Defines the
addon.open-cluster-management.io/v1beta1group version- Registers all required types (
ClusterManagementAddOn,ClusterManagementAddOnList,ManagedClusterAddOn,ManagedClusterAddOnList)- Maintains backward-compatible deprecated aliases (
SchemeGroupVersion,AddToScheme) for generated code compatibility- Follows the standard Kubernetes API registration pattern
client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
1-91: LGTM! Typed client implementation is correct.The generated
AddonV1beta1Clientproperly implements:
- Standard client-go constructor patterns (
NewForConfig,NewForConfigAndClient,NewForConfigOrDie,New)- Correct
setConfigDefaultswith v1beta1SchemeGroupVersion,/apispath, and proper codec configuration- Nil-safe
RESTClient()accessor- Resource accessors for both
ClusterManagementAddOns(cluster-scoped) andManagedClusterAddOns(namespaced)addon/v1beta1/types_managedclusteraddon.go (2)
1-248: LGTM! Well-structured v1beta1 API types.The
ManagedClusterAddOnCRD type definition is comprehensive and includes:
- Proper kubebuilder annotations for CRD generation with status subresource and print columns
- Clear separation of spec and status
- Well-documented registration configuration with KubeClient and CSR types
- Complete health check configuration with Lease and Customized modes
- Comprehensive set of condition types, reasons, labels, annotations, and finalizers for addon lifecycle management
The constants block provides good documentation for each condition type and reason.
Also applies to: 250-445
95-101: Based on the web search results, I found important information: ConfigGroupResource and ConfigReferent are defined in addon v1alpha1, not v1beta1. The search indicates "there is no widely published addon v1beta1 in the official API repo."This confirms the original review comment's concern is valid. Since the code at lines 95-101 in
addon/v1beta1/types_managedclusteraddon.goembeds these types, they must either be:
- Defined in v1beta1 (copied from v1alpha1), or
- Imported from v1alpha1
Without direct repository access to verify the actual v1beta1 file contents, I cannot conclusively determine if the type definitions exist in v1beta1 or if there's a compilation issue.
Verify that ConfigGroupResource and ConfigReferent are properly defined or imported in the v1beta1 package. The web search indicates these types currently exist only in addon v1alpha1. If this PR introduces a new v1beta1 API version, ensure these embedded types are either re-defined in v1beta1 or properly imported from v1alpha1 to avoid compilation errors.
addon/v1beta1/zz_generated.deepcopy.go (1)
1-632: Autogenerated deepcopy implementations look correct.This file is autogenerated by deepcopy-gen and follows standard Kubernetes patterns for runtime deepcopy support. The implementations correctly handle nil checks, slice/map copies, and nested struct deepcopying.
hack/verify-crds.sh (1)
18-18: LGTM - v1beta1 CRDs properly added to verification.The addition of
addon/v1beta1/*.crd.yamlfollows the existing pattern and ensures the new v1beta1 CRDs are validated alongside existing API versions.client/addon/clientset/versioned/scheme/register.go (1)
13-13: LGTM - v1beta1 properly registered in the scheme.The v1beta1 API group is correctly imported and registered in the runtime scheme builder, following the same pattern as v1alpha1. This enables proper serialization and client construction for v1beta1 types.
Also applies to: 21-21
Makefile (1)
39-39: LGTM - CRD generation added for v1beta1.The CRD generation entry for addonv1beta1 follows the established pattern and correctly specifies the API, manifest, and output paths.
addon/v1beta1/doc.go (1)
1-8: LGTM - Package documentation and code generation tags are correct.The doc.go file properly defines the v1beta1 package with standard Kubernetes code generation directives for deepcopy, OpenAPI schema, and kubebuilder validation.
client/addon/informers/externalversions/generic.go (1)
12-12: LGTM - v1beta1 informers properly wired.The generic informer correctly handles v1beta1 ClusterManagementAddOns and ManagedClusterAddOns resources, following the same pattern as the existing v1alpha1 implementation.
Also applies to: 51-56
hack/lib/init.sh (1)
22-22: LGTM! Configuration updated correctly for v1beta1.The addon/v1beta1 API version has been properly added to both API_GROUP_VERSIONS and API_PACKAGES with correct formatting.
Also applies to: 33-34
client/addon/informers/externalversions/addon/interface.go (1)
8-8: LGTM! Generated code follows expected patterns.The v1beta1 informer interface has been correctly generated following the same pattern as v1alpha1.
Also applies to: 16-18, 36-39
client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go (1)
1-8: LGTM! Standard generated expansion interfaces.These empty expansion interfaces follow the Kubernetes client-go pattern and provide extension points for future functionality.
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (1)
630-713: Verify registration validation for type-specific fields.The v1beta1 registration structure uses a discriminated union pattern with a
typefield (KubeClient or CSR), but the schema validation doesn't enforce that the corresponding nested field is populated. For example:
- When
type: KubeClient, thekubeClientfield should be required- When
type: CSR, thecsrfield should be requiredCurrently, only
typeis in the required list (lines 710-712), which could allow invalid configurations where a type is specified but the corresponding nested object is missing.Consider adding CEL validation rules to enforce this constraint:
x-kubernetes-validations: - rule: "self.type == 'KubeClient' ? has(self.kubeClient) : true" message: "kubeClient must be set when type is KubeClient" - rule: "self.type == 'CSR' ? has(self.csr) : true" message: "csr must be set when type is CSR"Alternatively, verify that admission webhook validation handles this constraint.
client/addon/listers/addon/v1beta1/clustermanagementaddon.go (1)
1-33: LGTM! Generated lister code is correct.The ClusterManagementAddOn lister follows standard Kubernetes patterns and is correctly generated for v1beta1.
client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
1-37: LGTM! Generated informer interface is correct.The v1beta1 informer interface correctly provides access to both ClusterManagementAddOn and ManagedClusterAddOn informers following standard patterns.
addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (1)
246-247: LGTM! Deprecation notices correctly added.The v1alpha1 CRD has been properly updated with deprecation notices pointing users to the v1beta1 API changes. The notices clearly indicate that the flat
signerNameandsubjectfields will be replaced with the new type-based structure.Also applies to: 304-306, 313-313
addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml (2)
626-629: LGTM! Storage version correctly configured.The CRD properly serves both v1alpha1 and v1beta1, with v1alpha1 as the storage version as stated in the PR objectives.
Also applies to: 1203-1206
17-1206: Based on the web search results and the CRD file structure provided, I can now assess the review comment:Findings:
The web search confirms that Kubernetes CRDs with multiple versions serving different schemas require conversion webhook configuration at
spec.conversion. The required structure includes:
spec.conversion.strategy: Webhookspec.conversion.webhook.conversionReviewVersionsspec.conversion.webhook.clientConfigCritical Issue: The provided CRD snippet starts at line 17 (the
versions:section), meaning lines 1-16 would contain the CRD metadata and spec configuration wherespec.conversionshould be defined. The snippet provided does not show any conversion configuration, and the CRD defines:
- v1alpha1: with
supportedConfigsandaddOnConfigurationfields- v1beta1: with
defaultConfigsonlyThese represent structural schema changes that require conversion webhooks to transform resources between versions.
However: The absence of conversion configuration in the visible snippet (lines 17-1206) cannot definitively prove it's missing from the full file, since it would be in the header lines (1-16) not shown in the provided context.
Verify conversion webhook is properly configured for transforming resources between v1alpha1 and v1beta1 versions.
The CRD defines two versions with incompatible schemas (v1alpha1 has
supportedConfigs/addOnConfiguration; v1beta1 hasdefaultConfigsonly). Per Kubernetes requirements, this requiresspec.conversion.strategy: Webhookwith appropriateclientConfigpointing to a conversion webhook server. The provided snippet does not show this configuration in the visible lines.client/addon/listers/addon/v1beta1/managedclusteraddon.go (1)
1-55: LGTM! Generated lister code follows standard patterns.This generated lister implementation correctly implements the standard Kubernetes lister pattern for namespaced resources, with proper use of generic
ResourceIndexerand expansion interfaces.client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (1)
1-87: LGTM! Generated informer code is correctly structured.The informer implementation follows standard informer-gen patterns with proper handling of the namespaced resource, including both legacy and context-aware list/watch functions.
client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go (1)
1-86: LGTM! Generated informer code for cluster-scoped resource.The informer correctly implements the cluster-scoped pattern without a namespace parameter, following standard informer-gen conventions.
client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go (1)
1-55: LGTM! Generated typed client follows standard patterns.The client implementation correctly uses the generic
ClientWithListwrapper for the namespaced ManagedClusterAddOn resource with proper CRUD operations including status updates.client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go (1)
1-55: LGTM! Generated typed client for cluster-scoped resource.The client implementation correctly configures the generic wrapper with an empty namespace for the cluster-scoped ClusterManagementAddOn resource.
addon/v1beta1/types_clustermanagementaddon.go (1)
45-45: Verify ifAddOnConfigtype is defined in the v1beta1 package.The original review comment flags
AddOnConfigas undefined, but I cannot access the repository to verify this claim. The web search results indicate thatAddOnConfigis a known type in the open-cluster-management-io/api codebase, suggesting the type definition likely exists.Without direct access to the codebase, I cannot confirm whether:
AddOnConfigis defined locally in the v1beta1 package filesAddOnConfigis imported from another packageAddOnConfigis actually undefined and would cause compilation errorsPlease manually verify by checking:
- Search for
type AddOnConfigdefinition inaddon/v1beta1/*.gofiles- Check for imports that might bring
AddOnConfiginto scope- Attempt to compile the code to see if there are actual type errors
addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml
Outdated
Show resolved
Hide resolved
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml
Show resolved
Hide resolved
|
|
||
| import ( | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| clusterv1alpha1 "open-cluster-management.io/api/cluster/v1alpha1" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for RolloutStrategy type definitions across API versions
rg -n "type RolloutStrategy" --type=go
echo "---"
echo "Also searching for RolloutStrategy struct definitions:"
rg -n "RolloutStrategy\s*struct" --type=goRepository: open-cluster-management-io/api
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Search for RolloutStrategy in current repository
echo "=== Searching for RolloutStrategy in current repository ==="
rg -n "RolloutStrategy" --type=go
echo ""
echo "=== Checking for v1alpha1 imports across codebase ==="
rg -n "v1alpha1" --type=go | head -20
echo ""
echo "=== Checking for v1beta1 imports to understand pattern ==="
rg -n "v1beta1" --type=go | head -20Repository: open-cluster-management-io/api
Length of output: 204
🌐 Web query:
open-cluster-management api RolloutStrategy cluster addon versioning
💡 Result:
Summary — RolloutStrategy for Cluster Add‑ons in Open Cluster Management (OCM)
-
Where defined: RolloutStrategy is part of the ClusterManagementAddOn API (used by addon-framework and addon-manager) to control how an add‑on (ManagedClusterAddOn / ManifestWork) is upgraded across managed clusters. [1][3]
-
Strategy types:
- All — apply to all selected clusters simultaneously. [2]
- Progressive — canary/gradual rollout with parameters (mandatory decision groups first, then others at a concurrency rate). Key fields: mandatoryDecisionGroups, maxConcurrency (e.g., 25%), minSuccessTime (soak), progressDeadline (timeout), maxFailures. [1][2]
- ProgressivePerGroup — progressive rollout applied per decision group with its own parameters. [2]
-
How it ties to versioning / upgrades:
- Rollouts are triggered by changes to add‑on configuration resources (e.g., AddonDeploymentConfig / manifest configs). The controller compares applied state (including agent versions reported in addon resources/status) and uses the rollout strategy to stage upgrades. You can reflect agent version in addon resources to control progressive upgrades. [1]
- The same rollout API is used by ManifestWorkReplicaSet (work distribution) — so rollout behavior is consistent between add‑on agent upgrades and manifest/work upgrades. [2][3]
References
- Add‑on management / ClusterManagementAddOn rollout example and fields. [1]
- ManifestWorkReplicaSet rollout strategy and types (All, Progressive, ProgressivePerGroup). [2]
- Add‑on developer guide (install/rollout strategy usage and addon-framework notes). [3]
🌐 Web query:
open-cluster-management v1beta1 v1alpha1 API compatibility
💡 Result:
Short answer: v1alpha1 APIs in Open Cluster Management were deprecated and migrated to v1beta1 (and later v1beta2/v1) — v1alpha1 placements, placementdecisions, managedclustersets/managedclustersetbindings and several other v1alpha1 APIs were removed in later OCM releases. Migrate manifests to the v1beta1 (or the newer v1beta2/v1) API versions and adjust any changed fields. [1][2][3]
Quick details and action items:
- Which APIs moved/removed: placements and placementdecisions (v1alpha1 → v1beta1), managedclustersets and managedclustersetbindings (v1alpha1 → v1beta1 → v1beta2 later), many v1alpha1 addon/app-related APIs were removed or upgraded in OCM 2.x releases. See the removal/upgrade lists. [2][3]
- Field changes to check: some spec fields changed between versions (example: Placement prioritizerPolicy configuration fields changed between v1alpha1 and v1beta1), so you must compare CRD schemas and update manifests accordingly. [3]
- Migration steps: install the OCM release that supports the target API version, update CRD/apiVersion in your YAMLs, update any renamed/removed fields, reapply and test. If you support multiple OCM releases during upgrade, consult the release notes for the exact compatibility/migration guidance for each component. [1][2]
References:
- OCM release notes / API changelog. [1]
- Red Hat Advanced Cluster Management (OCM) release notes — lists API removals and recommended replacements. [2][3]
🌐 Web query:
open-cluster-management RolloutStrategy v1beta1 cluster api
💡 Result:
Summary — RolloutStrategy (OCM v1beta1)
-
Purpose: controls how workloads/addons are rolled out across Placement decision groups or clusters (used by Placement, ClusterManagementAddOn/InstallStrategy, ManifestWorkReplicaSet). [1][2][3][4]
-
Types:
- All — apply to all selected clusters at once. [2][3]
- Progressive — gradual, per-cluster rollout with controls for mandatory canary groups, concurrency, soak and deadlines. Common fields: mandatoryDecisionGroups, maxConcurrency (absolute or percent), minSuccessTime, progressDeadline, maxFailures. [1][2][3]
- ProgressivePerGroup — roll per decision-group (advance to next group only after current group succeeds). Config fields include progressDeadline and maxFailures. [2][3]
-
Where it appears:
- Placement.spec.rolloutStrategy (cluster selection/decision grouping). [2]
- ClusterManagementAddOn.spec.installStrategy. rolloutStrategy (addon upgrades). Example uses Progressive with mandatoryDecisionGroups and maxConcurrency. [1]
- ManifestWorkReplicaSet.placementRefs[].rolloutStrategy (work distribution). Requires ManifestWork status conditions (Progressing/Degraded) via CEL. [3]
-
Implementation notes:
- Controllers evaluate PlacementDecision groups and enforce maxFailures/progressDeadline/minSuccessTime before advancing. [2][3]
- API types and CRDs are defined in the open-cluster-management api repo. Use that repo for exact schema and client types. [4]
Sources:
- Open Cluster Management — Add-on management (Rollout strategy examples). [online]. [2 Nov 2025 snapshot]. URL doc. [1]
- Open Cluster Management — Placement (decisionStrategy & rolloutStrategy). [online]. [2 Nov 2025 snapshot]. URL doc. [2]
- Open Cluster Management — ManifestWorkReplicaSet (rollout strategy details). [online]. [2 Nov 2025 snapshot]. URL doc. [3]
- open-cluster-management-io/api GitHub (API/CRD definitions). [online]. [2 Nov 2025 snapshot]. [4]
Would you like the exact CRD/schema snippet for v1beta1 or an example manifest adapted to your use case?
Based on the web search results, I can now verify the review comment. The evidence shows that:
-
RolloutStrategy exists in v1beta1: The web search confirms that
ClusterManagementAddOn.spec.installStrategy.rolloutStrategyuses v1beta1 RolloutStrategy. -
v1alpha1 deprecation: Earlier search results confirmed that v1alpha1 APIs in OCM were deprecated and removed in later releases.
-
Cross-version mismatch: The file
addon/v1beta1/types_clustermanagementaddon.go(a beta API) importsclusterv1alpha1, creating a forward-compatibility risk since v1alpha1 was deprecated.
The review comment's concern is valid and substantiated. This is a genuine issue that should be addressed by upgrading the import to use clusterv1beta1 instead of clusterv1alpha1.
Update the cluster API import from v1alpha1 to v1beta1 to ensure forward compatibility.
The v1beta1 addon API currently imports clusterv1alpha1 "open-cluster-management.io/api/cluster/v1alpha1" (line 6), but v1alpha1 APIs have been deprecated and removed in later OCM releases. Since RolloutStrategy is available in the v1beta1 cluster API and is used in the addon v1beta1 context, upgrade the import to clusterv1beta1 to prevent breaking when v1alpha1 is no longer available.
🤖 Prompt for AI Agents
In addon/v1beta1/types_clustermanagementaddon.go around line 6, the file imports
clusterv1alpha1 which is deprecated; update the import to clusterv1beta1 (e.g.,
change the import path from "open-cluster-management.io/api/cluster/v1alpha1" to
"open-cluster-management.io/api/cluster/v1beta1" and update the alias to
clusterv1beta1), then adjust any type references in this file that use the
clusterv1alpha1 alias to use clusterv1beta1 so RolloutStrategy and other types
resolve to the beta cluster API.
haoqing0110
force-pushed
the
br_addon-v1beta1
branch
from
034bd14 to
3fb6c88
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (1)
17-778: Conversion webhook configuration is missing for multi-version CRD with schema differences.This CRD serves v1alpha1 and v1beta1 with notable schema changes:
- v1alpha1 spec includes
installNamespace; v1beta1 does not- v1beta1 restructures
registrationswith newtype,csr, andkubeClientfieldsPer Kubernetes CRD documentation, multi-version CRDs with incompatible schemas require
spec.conversion.strategy: Webhookconfiguration. Without this, the API server cannot properly convert objects between versions.addon/v1beta1/types_clustermanagementaddon.go (3)
6-6: Update the cluster API import from v1alpha1 to v1beta1 to ensure forward compatibility.The v1beta1 addon API imports
clusterv1alpha1which has been deprecated. Update toclusterv1beta1to align with the v1beta1 addon API version and prevent breaking when v1alpha1 is removed.Apply this diff:
- clusterv1alpha1 "open-cluster-management.io/api/cluster/v1alpha1" + clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1"And update line 154:
- RolloutStrategy clusterv1alpha1.RolloutStrategy `json:"rolloutStrategy,omitempty"` + RolloutStrategy clusterv1beta1.RolloutStrategy `json:"rolloutStrategy,omitempty"`
159-159: Fix casing in comment.The comment uses "defaultconfigReferences" but should match the field name
DefaultConfigReferenceswith uppercase 'C'.Apply this diff:
- // DefaultconfigReferences is a list of current add-on default configuration references. + // DefaultConfigReferences is a list of current add-on default configuration references.
232-232: Fix typo in comment."featugate" should be "feature gate".
Apply this diff:
- // when featugate AddonManager on hub is enabled + // when feature gate AddonManager on hub is enabled
🧹 Nitpick comments (1)
addon/v1alpha1/types_clustermanagementaddon.go (1)
197-200: Comment text inconsistent with field name casing.The comment says "DefaultconfigReferences" but the actual field is
DefaultConfigReferences(camelCase). Consider updating for consistency.- // DefaultconfigReferences is a list of current add-on default configuration references. + // DefaultConfigReferences is a list of current add-on default configuration references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
Makefile(1 hunks)addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(2 hunks)addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml(2 hunks)addon/v1alpha1/types_clustermanagementaddon.go(6 hunks)addon/v1alpha1/types_managedclusteraddon.go(1 hunks)addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(1 hunks)addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml(1 hunks)addon/v1beta1/doc.go(1 hunks)addon/v1beta1/register.go(1 hunks)addon/v1beta1/types_clustermanagementaddon.go(1 hunks)addon/v1beta1/types_managedclusteraddon.go(1 hunks)addon/v1beta1/zz_generated.deepcopy.go(1 hunks)client/addon/clientset/versioned/clientset.go(3 hunks)client/addon/clientset/versioned/fake/clientset_generated.go(2 hunks)client/addon/clientset/versioned/fake/register.go(1 hunks)client/addon/clientset/versioned/scheme/register.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/doc.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/doc.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_managedclusteraddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go(1 hunks)client/addon/informers/externalversions/addon/interface.go(2 hunks)client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/informers/externalversions/addon/v1beta1/interface.go(1 hunks)client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go(1 hunks)client/addon/informers/externalversions/generic.go(2 hunks)client/addon/listers/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/listers/addon/v1beta1/expansion_generated.go(1 hunks)client/addon/listers/addon/v1beta1/managedclusteraddon.go(1 hunks)hack/lib/init.sh(2 hunks)hack/verify-crds.sh(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- client/addon/clientset/versioned/typed/addon/v1beta1/fake/doc.go
- client/addon/clientset/versioned/typed/addon/v1beta1/doc.go
🚧 Files skipped from review as they are similar to previous changes (17)
- client/addon/listers/addon/v1beta1/expansion_generated.go
- addon/v1alpha1/types_managedclusteraddon.go
- client/addon/clientset/versioned/fake/register.go
- hack/lib/init.sh
- client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go
- client/addon/informers/externalversions/addon/interface.go
- Makefile
- hack/verify-crds.sh
- addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml
- client/addon/listers/addon/v1beta1/clustermanagementaddon.go
- client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_managedclusteraddon.go
- client/addon/informers/externalversions/generic.go
- client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go
- client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go
- addon/v1beta1/register.go
- client/addon/clientset/versioned/clientset.go
- client/addon/clientset/versioned/scheme/register.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-02T14:09:55.079Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 380
File: client/work/clientset/versioned/fake/clientset_generated.go:40-40
Timestamp: 2025-07-02T14:09:55.079Z
Learning: Generated code files marked with "Code generated by client-gen. DO NOT EDIT." should not be manually edited as they are automatically generated by Kubernetes code-generator tools and any changes would be overwritten.
Applied to files:
client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go
📚 Learning: 2025-09-02T11:59:50.703Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 392
File: operator/v1/types_clustermanager.go:178-195
Timestamp: 2025-09-02T11:59:50.703Z
Learning: XValidation markers in kubebuilder require Kubernetes 1.25+ and should not be added to the open-cluster-management-io/api project until minimum Kubernetes version requirements are established to avoid breaking compatibility with older clusters.
Applied to files:
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/types_clustermanagementaddon.go
🧬 Code graph analysis (6)
client/addon/informers/externalversions/addon/v1beta1/interface.go (3)
client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go (1)
ClusterManagementAddOnInformer(22-25)client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (1)
ManagedClusterAddOnInformer(22-25)client/addon/informers/externalversions/addon/interface.go (1)
Interface(13-18)
client/addon/clientset/versioned/fake/clientset_generated.go (3)
client/addon/clientset/versioned/clientset.go (1)
Clientset(24-28)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
AddonV1beta1Interface(14-18)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go (1)
FakeAddonV1beta1(12-14)
client/addon/listers/addon/v1beta1/managedclusteraddon.go (3)
addon/v1beta1/types_managedclusteraddon.go (1)
ManagedClusterAddOn(19-32)client/addon/listers/addon/v1beta1/expansion_generated.go (2)
ManagedClusterAddOnListerExpansion(12-12)ManagedClusterAddOnNamespaceListerExpansion(16-16)addon/v1beta1/register.go (1)
Resource(27-29)
client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go (4)
client/addon/listers/addon/v1beta1/clustermanagementaddon.go (2)
ClusterManagementAddOnLister(15-23)NewClusterManagementAddOnLister(31-33)client/addon/clientset/versioned/clientset.go (1)
Interface(17-21)client/addon/informers/externalversions/addon/interface.go (1)
Interface(13-18)client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
Interface(11-16)
client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (3)
client/addon/listers/addon/v1beta1/managedclusteraddon.go (2)
ManagedClusterAddOnLister(15-22)NewManagedClusterAddOnLister(30-32)client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
Interface(11-16)addon/v1beta1/types_managedclusteraddon.go (1)
ManagedClusterAddOn(19-32)
addon/v1beta1/zz_generated.deepcopy.go (2)
addon/v1beta1/types_managedclusteraddon.go (12)
AddOnConfig(95-101)CSRConfig(82-93)Subject(111-122)ConfigReference(204-218)HealthCheck(234-239)KubeClientConfig(76-80)ManagedClusterAddOn(19-32)ManagedClusterAddOnStatus(126-181)ObjectReference(184-200)RegistrationConfig(59-74)KubeClient(51-51)CSR(55-55)addon/v1beta1/types_clustermanagementaddon.go (14)
ConfigGroupResource(66-77)ConfigReferent(80-91)AddOnMeta(55-63)ClusterManagementAddOn(21-32)ClusterManagementAddOnList(214-223)ClusterManagementAddOnSpec(35-52)InstallStrategy(104-121)ClusterManagementAddOnStatus(158-165)DefaultConfigReference(183-189)InstallProgression(167-179)ConfigSpecHash(94-100)InstallConfigReference(193-210)PlacementRef(131-142)PlacementStrategy(144-155)
⏰ 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). (1)
- GitHub Check: verify
🔇 Additional comments (24)
client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go (1)
1-8: Generated expansion interfaces look correct and consistentThe v1beta1 expansion interfaces mirror the v1alpha1 pattern, use the expected names, and the file is clearly marked as client-gen output; no issues from a review standpoint. Based on learnings, this should remain generator‑owned and not be hand‑edited.
addon/v1alpha1/types_clustermanagementaddon.go (5)
48-56: LGTM!The deprecation comment typo has been fixed ("Will" instead of "Wil"), and the
SupportedConfigsfield with its deprecation notice is appropriately documented for the v1beta1 migration.
114-115: LGTM!The kubebuilder validation tag syntax is now correct (
MinLength=1instead ofMinLength:=1).
127-129: LGTM!Validation tag syntax corrected.
173-180: LGTM!Validation tag syntax corrected for both
NamespaceandNamefields inPlacementRef.
269-272: LGTM!Spelling fix from "featugate" to "featuregate" is correct.
addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (2)
246-247: LGTM!The
lastObservedGenerationdescription has been simplified appropriately.
304-313: LGTM!Deprecation notices for
signerNameandsubjectfields clearly indicate they will be replaced withtype: kubeClientandtype: csrin v1beta1, providing a good migration path for users.addon/v1beta1/doc.go (1)
1-8: LGTM!Standard API package boilerplate with appropriate code generation directives for the new v1beta1 API group.
client/addon/clientset/versioned/fake/clientset_generated.go (2)
16-17: LGTM!The v1beta1 imports are correctly added for both the interface and fake implementation packages.
82-86: LGTM!The
AddonV1beta1()method follows the same pattern asAddonV1alpha1()and correctly returns the fake v1beta1 client, properly implementing theclientset.Interface.addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (3)
28-31: LGTM!The v1alpha1 version is properly marked as deprecated with a clear deprecation warning directing users to v1beta1.
401-404: Storage version configuration aligns with PR objectives.v1alpha1 is correctly set as the storage version (
storage: true) while v1beta1 is served but not stored (storage: false). This matches the stated goal of keeping storage as v1alpha1 while serving both versions.Also applies to: 775-778
266-269: Inconsistent required fields inconfigReferencesbetween versions.The v1alpha1 schema requires both
nameandresourcefields (lines 266-268), but v1beta1 only requiresresource(lines 610-611). This inconsistency could cause issues when objects stored in v1alpha1 format are served as v1beta1 (or vice versa), as objects withoutnamewould be valid in v1beta1 but invalid in v1alpha1.Verify this is intentional. If the
namefield is optional in v1beta1, ensure conversion logic (if any) handles missingnamevalues appropriately when reading from v1alpha1 storage.Also applies to: 606-612
client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
1-37: LGTM - Generated informer interface follows standard patterns.The v1beta1 informer interface correctly mirrors the v1alpha1 structure. The namespace handling is appropriate:
ClusterManagementAddOns()doesn't use namespace (cluster-scoped), whileManagedClusterAddOns()passes namespace (namespace-scoped).addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml (2)
25-28: Deprecation notice looks correct.The v1alpha1 version correctly includes deprecation metadata directing users to migrate to v1beta1. This aligns with the PR objective of introducing v1beta1 while maintaining v1alpha1 for backward compatibility.
428-429: Confirm whetherdefaultconfigReferencesnaming is intentional or requires correction.The field name
defaultconfigReferences(lowercase 'c') deviates from Kubernetes camelCase conventions (should bedefaultConfigReferenceswith uppercase 'C'). This is inconsistent with the related spec fielddefaultConfigs, which follows proper camelCase.According to Kubernetes best practices, renaming a field after it's been stored in etcd would be a breaking change. If this field already exists in production resources with the current spelling, renaming it requires:
- Adding the new correctly-named field
- Implementing conversion logic between versions
- Migrating existing resources
- Marking the old field as deprecated
Clarify with the team: Is this lowercase spelling intentional for backward compatibility with existing stored resources, or is it an unconaught typo that should be corrected now before wider adoption?
client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
1-91: LGTM - Generated client follows standard patterns.The v1beta1 client implementation correctly:
- Uses
addonv1beta1.SchemeGroupVersionfor group version configuration- Provides standard constructors (
NewForConfig,NewForConfigAndClient,NewForConfigOrDie,New)- Includes proper nil-check in
RESTClient()accessor- Follows the same pattern as the v1alpha1 client
client/addon/listers/addon/v1beta1/managedclusteraddon.go (1)
1-55: LGTM - Generated lister follows standard patterns.The v1beta1 ManagedClusterAddOn lister correctly implements:
- Cluster-wide
ManagedClusterAddOnListerwithListand namespace accessor- Namespace-scoped
ManagedClusterAddOnNamespaceListerwithListandGet- Proper embedding of expansion interfaces for extensibility
- Uses generic
listers.ResourceIndexerwith correct resource typeclient/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go (1)
1-86: LGTM - Generated informer follows standard patterns.The v1beta1 ClusterManagementAddOn informer correctly:
- Provides both filtered and non-filtered constructors
- Implements context-aware
ListWithContextFuncandWatchFuncWithContextfor proper context propagation- Uses the v1beta1 clientset (
client.AddonV1beta1().ClusterManagementAddOns())- Returns proper lister from
NewClusterManagementAddOnListerThe
cache.NamespaceIndexindefaultInformer(line 77) is standard informer-gen output even for cluster-scoped resources and is harmless.client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (1)
1-87: LGTM! Generated informer code follows standard patterns.The informer implementation correctly wires List/Watch operations with context support and integrates with the v1beta1 ManagedClusterAddOn types, client, and lister.
client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go (1)
1-55: LGTM! Generated typed client follows standard patterns.The ClusterManagementAddOn client correctly implements the standard Kubernetes typed client interface using gentype.ClientWithList for all CRUD operations.
addon/v1beta1/types_managedclusteraddon.go (1)
1-445: LGTM! v1beta1 ManagedClusterAddOn types are well-structured.The type definitions correctly implement Kubernetes API conventions with:
- Proper kubebuilder markers for CRD generation and validation
- Comprehensive status fields including conditions, registrations, and health checks
- Well-documented constants for labels, annotations, and condition types
- Appropriate validation rules (e.g., SignerName pattern, enum types)
addon/v1beta1/zz_generated.deepcopy.go (1)
1-632: LGTM! Generated deepcopy implementations are correct.The autogenerated deepcopy methods correctly handle all v1beta1 addon types with proper deep copying of nested structs, slices, pointers, and conditions.
addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml
Outdated
Show resolved
Hide resolved
|
/hold |
haoqing0110
force-pushed
the
br_addon-v1beta1
branch
from
3fb6c88 to
edcf3a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
addon/v1beta1/types_clustermanagementaddon.go (1)
4-7: Avoid coupling v1beta1 addon API to cluster/v1alpha1 RolloutStrategy
types_clustermanagementaddon.goimportsclusterv1alpha1and usesclusterv1alpha1.RolloutStrategyinPlacementStrategy. This keeps the new v1beta1 addon API tied to the deprecated cluster/v1alpha1 API and will cause problems once cluster/v1alpha1 is dropped.Switch this dependency to the cluster v1beta1 RolloutStrategy type so the addon v1beta1 surface only depends on non-deprecated cluster APIs, and update the import alias and field type accordingly.
Example diff:
-import ( - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - clusterv1alpha1 "open-cluster-management.io/api/cluster/v1alpha1" -) +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1" +) … - // +kubebuilder:default={type: All} - // +optional - RolloutStrategy clusterv1alpha1.RolloutStrategy `json:"rolloutStrategy,omitempty"` + // +kubebuilder:default={type: All} + // +optional + RolloutStrategy clusterv1beta1.RolloutStrategy `json:"rolloutStrategy,omitempty"`Also applies to: 144-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
Makefile(1 hunks)addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(2 hunks)addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml(2 hunks)addon/v1alpha1/types_clustermanagementaddon.go(6 hunks)addon/v1alpha1/types_managedclusteraddon.go(1 hunks)addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(1 hunks)addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml(1 hunks)addon/v1beta1/doc.go(1 hunks)addon/v1beta1/register.go(1 hunks)addon/v1beta1/types_clustermanagementaddon.go(1 hunks)addon/v1beta1/types_managedclusteraddon.go(1 hunks)addon/v1beta1/zz_generated.deepcopy.go(1 hunks)client/addon/clientset/versioned/clientset.go(3 hunks)client/addon/clientset/versioned/fake/clientset_generated.go(2 hunks)client/addon/clientset/versioned/fake/register.go(1 hunks)client/addon/clientset/versioned/scheme/register.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/doc.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/doc.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_managedclusteraddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go(1 hunks)client/addon/informers/externalversions/addon/interface.go(2 hunks)client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/informers/externalversions/addon/v1beta1/interface.go(1 hunks)client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go(1 hunks)client/addon/informers/externalversions/generic.go(2 hunks)client/addon/listers/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/listers/addon/v1beta1/expansion_generated.go(1 hunks)client/addon/listers/addon/v1beta1/managedclusteraddon.go(1 hunks)hack/lib/init.sh(2 hunks)hack/verify-crds.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- addon/v1beta1/doc.go
- client/addon/informers/externalversions/addon/v1beta1/interface.go
- client/addon/listers/addon/v1beta1/expansion_generated.go
- client/addon/listers/addon/v1beta1/clustermanagementaddon.go
- hack/verify-crds.sh
- client/addon/informers/externalversions/addon/interface.go
- addon/v1beta1/register.go
- hack/lib/init.sh
- addon/v1alpha1/types_managedclusteraddon.go
- client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go
- client/addon/clientset/versioned/fake/register.go
- addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml
- addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml
- client/addon/clientset/versioned/typed/addon/v1beta1/fake/doc.go
- client/addon/clientset/versioned/typed/addon/v1beta1/doc.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.726Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
📚 Learning: 2025-12-01T01:10:01.726Z
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.726Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
Applied to files:
client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.goclient/addon/listers/addon/v1beta1/managedclusteraddon.goMakefileaddon/v1beta1/types_clustermanagementaddon.goclient/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_managedclusteraddon.goclient/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.goclient/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.goaddon/v1beta1/types_managedclusteraddon.goaddon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yamladdon/v1alpha1/types_clustermanagementaddon.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/zz_generated.deepcopy.go
📚 Learning: 2025-07-02T14:09:55.079Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 380
File: client/work/clientset/versioned/fake/clientset_generated.go:40-40
Timestamp: 2025-07-02T14:09:55.079Z
Learning: Generated code files marked with "Code generated by client-gen. DO NOT EDIT." should not be manually edited as they are automatically generated by Kubernetes code-generator tools and any changes would be overwritten.
Applied to files:
client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go
📚 Learning: 2025-09-02T11:59:50.703Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 392
File: operator/v1/types_clustermanager.go:178-195
Timestamp: 2025-09-02T11:59:50.703Z
Learning: XValidation markers in kubebuilder require Kubernetes 1.25+ and should not be added to the open-cluster-management-io/api project until minimum Kubernetes version requirements are established to avoid breaking compatibility with older clusters.
Applied to files:
addon/v1beta1/types_clustermanagementaddon.go
🧬 Code graph analysis (10)
client/addon/listers/addon/v1beta1/managedclusteraddon.go (4)
addon/v1beta1/types_managedclusteraddon.go (1)
ManagedClusterAddOn(19-32)client/addon/listers/addon/v1beta1/expansion_generated.go (2)
ManagedClusterAddOnListerExpansion(12-12)ManagedClusterAddOnNamespaceListerExpansion(16-16)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
New(69-71)addon/v1beta1/register.go (1)
Resource(27-29)
client/addon/informers/externalversions/generic.go (2)
addon/v1beta1/register.go (1)
SchemeGroupVersion(19-19)addon/v1alpha1/register.go (1)
SchemeGroupVersion(19-19)
addon/v1beta1/types_clustermanagementaddon.go (4)
addon/v1alpha1/types_clustermanagementaddon.go (6)
ClusterManagementAddOnSpec(36-63)AddOnMeta(66-74)InstallStrategy(143-160)ConfigReferent(119-130)ConfigGroupResource(105-116)ClusterManagementAddOn(22-33)addon/v1alpha1/types_managedclusteraddon.go (1)
AddOnConfig(78-84)addon/v1beta1/types_managedclusteraddon.go (1)
AddOnConfig(95-101)cluster/v1alpha1/types_rolloutstrategy.go (1)
RolloutStrategy(26-52)
client/addon/clientset/versioned/fake/clientset_generated.go (3)
client/addon/clientset/versioned/clientset.go (1)
Clientset(24-28)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
AddonV1beta1Interface(14-18)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go (1)
FakeAddonV1beta1(12-14)
client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go (4)
client/addon/listers/addon/v1beta1/clustermanagementaddon.go (2)
ClusterManagementAddOnLister(15-23)NewClusterManagementAddOnLister(31-33)client/addon/clientset/versioned/clientset.go (1)
Interface(17-21)client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
Interface(11-16)addon/v1beta1/types_clustermanagementaddon.go (1)
ClusterManagementAddOn(21-32)
client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go (4)
addon/v1beta1/types_managedclusteraddon.go (2)
ManagedClusterAddOn(19-32)ManagedClusterAddOnList(243-248)client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go (1)
ManagedClusterAddOnExpansion(8-8)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
AddonV1beta1Client(21-23)client/addon/clientset/versioned/scheme/register.go (1)
ParameterCodec(18-18)
client/addon/clientset/versioned/clientset.go (1)
client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (4)
AddonV1beta1Interface(14-18)AddonV1beta1Client(21-23)NewForConfigAndClient(48-56)New(69-71)
client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (4)
client/addon/listers/addon/v1beta1/managedclusteraddon.go (2)
ManagedClusterAddOnLister(15-22)NewManagedClusterAddOnLister(30-32)client/addon/clientset/versioned/clientset.go (1)
Interface(17-21)client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
Interface(11-16)addon/v1beta1/types_managedclusteraddon.go (1)
ManagedClusterAddOn(19-32)
addon/v1beta1/types_managedclusteraddon.go (1)
addon/v1alpha1/types_clustermanagementaddon.go (4)
ConfigGroupResource(105-116)ConfigReferent(119-130)AddOnMeta(66-74)ConfigSpecHash(133-139)
addon/v1beta1/zz_generated.deepcopy.go (2)
addon/v1beta1/types_managedclusteraddon.go (7)
AddOnConfig(95-101)CSRConfig(82-93)Subject(111-122)ConfigReference(204-218)HealthCheck(234-239)ManagedClusterAddOn(19-32)CSR(55-55)addon/v1beta1/types_clustermanagementaddon.go (11)
ConfigGroupResource(66-77)ConfigReferent(80-91)AddOnMeta(55-63)ClusterManagementAddOn(21-32)ClusterManagementAddOnList(214-223)ClusterManagementAddOnSpec(35-52)ClusterManagementAddOnStatus(158-165)ConfigSpecHash(94-100)InstallConfigReference(193-210)PlacementRef(131-142)PlacementStrategy(144-155)
⏰ 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 (10)
client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go (1)
1-8: Generated expansion interfaces look correct; keep changes generator-drivenThis matches the standard
client-genpattern of empty*Expansioninterfaces to allow future extension methods on the typed client. Since the file is marked as generated, any future adjustments should go through the code-generator config rather than manual edits. Based on learnings, this aligns with the existing practice for generated client code in this repo.client/addon/clientset/versioned/fake/clientset_generated.go (1)
16-17: LGTM!The v1beta1 fake client additions correctly mirror the existing v1alpha1 pattern. The
AddonV1beta1()method properly returns aFakeAddonV1beta1struct sharing the sameFakeinstance, ensuring consistent behavior across API versions in tests.Also applies to: 82-86
addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml (1)
377-382: LGTM!The deprecation notice for
supportedConfigsis properly documented, correctly referencingDefaultConfigsin v1beta1 as the replacement. The typo has been addressed.client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go (1)
1-37: LGTM!The fake ClusterManagementAddOn client implementation is correctly structured:
- Empty namespace (line 22) is appropriate for this cluster-scoped resource
- Object and list factories are properly wired for v1beta1 types
- Follows the standard
gentype.FakeClientWithListpatternMakefile (1)
39-39: LGTM!The v1beta1 CRD generation entry correctly includes both
./addon/v1alpha1and./addon/v1beta1as API sources, following the established pattern for multi-version CRDs (similar to line 34 forclusterv1beta2). This aligns with the PR objective to serve both v1alpha1 and v1beta1 API versions.client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go (1)
20-86: ClusterManagementAddOn informer wiring looks correctInformer and lister wiring follow the standard informer-gen pattern (including context-aware List/Watch) against
AddonV1beta1().ClusterManagementAddOns(). No issues from an API or concurrency perspective.client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (1)
20-87: ManagedClusterAddOn informer is consistent and correctNamespaced informer uses
AddonV1beta1().ManagedClusterAddOns(namespace)with both background and context-aware List/Watch functions, and exposes the expected lister. This matches the established informer patterns in the repo.client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go (1)
17-55: Typed client for ManagedClusterAddOn is correctly wiredThe v1beta1
ManagedClusterAddOnInterfaceandmanagedClusterAddOnsimplementation use the expected resource name ("managedclusteraddons"), REST client, parameter codec, and addon v1beta1 types. This matches the other generated clients in the repo.addon/v1beta1/types_managedclusteraddon.go (1)
8-445: ManagedClusterAddOn v1beta1 types look consistent with existing API conventionsThe v1beta1
ManagedClusterAddOnschema (spec/status, registration structs, config references, health check, and condition/annotation constants) aligns with the existing v1alpha1 shapes and kubebuilder marker usage, without introducing XValidation or other features that would tighten the Kubernetes version requirement. I don’t see schema or validation issues in this file.addon/v1beta1/zz_generated.deepcopy.go (1)
14-632: DeepCopy implementations match the v1beta1 type shapesAutogenerated
DeepCopy*andDeepCopyObjectmethods correctly deep-copy slices, maps, and pointer fields for the addon v1beta1 types (including nested structs likeInstallStrategy,PlacementStrategy,ManagedClusterAddOnStatus, and config reference types). No issues with shallow copies or missing fields observed.
haoqing0110
force-pushed
the
br_addon-v1beta1
branch
from
edcf3a4 to
538c3e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
addon/v1alpha1/types_clustermanagementaddon.go (1)
198-198: Fix casing inconsistency in comment.The comment refers to "DefaultconfigReferences" (lowercase 'c'), but the field name is
DefaultConfigReferences(uppercase 'C'). The comment should match the field name for clarity.Apply this diff:
- // DefaultconfigReferences is a list of current add-on default configuration references. + // DefaultConfigReferences is a list of current add-on default configuration references.addon/v1beta1/types_clustermanagementaddon.go (2)
6-6: Critical: Update cluster API import from v1alpha1 to v1beta1.The v1beta1 addon API imports
clusterv1alpha1(line 6) which has been deprecated. SinceRolloutStrategy(used at line 154) is available in the cluster v1beta1 API, this import should be updated to prevent breaking changes when v1alpha1 is removed.Apply this diff:
- clusterv1alpha1 "open-cluster-management.io/api/cluster/v1alpha1" + clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1"Then update line 154:
- RolloutStrategy clusterv1alpha1.RolloutStrategy `json:"rolloutStrategy,omitempty"` + RolloutStrategy clusterv1beta1.RolloutStrategy `json:"rolloutStrategy,omitempty"`
159-161: Fix casing in comment to match field name.The comment uses "DefaultconfigReferences" (lowercase 'c') but should be "DefaultConfigReferences" (uppercase 'C') to match the exported field name.
Apply this diff:
- // DefaultconfigReferences is a list of current add-on default configuration references. + // DefaultConfigReferences is a list of current add-on default configuration references.
🧹 Nitpick comments (2)
addon/v1beta1/types_managedclusteraddon.go (1)
16-445: v1beta1 ManagedClusterAddOn types look consistent with the CRD; only a tiny doc nit.The core v1beta1 API surface (spec, status, registration model, health-check mode, and related constants) is well-structured and matches the v1beta1 CRD schema you’re introducing. This should give controllers a clear contract for the new registration and health-check behavior.
One small documentation nit you might optionally clean up later: in
Subject, the comment says “organizationUnit is the ou of the addon agent” while the field isOrganizationUnits []stringwith JSON tagorganizationUnit. It’s minor, but you could rephrase to reflect that it’s a list (e.g., “organizationUnits are the OUs of the addon agent”) the next time you touch these types and regenerate CRDs.addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (1)
1-784: Multi-version ManagedClusterAddOn CRD wiring matches the v1alpha1→v1beta1 migration plan, with a minor doc mismatch.
- v1alpha1 is correctly marked
deprecated: trueand kept asstorage: true, while v1beta1 isserved: true,storage: false, which aligns with “keep storage at v1alpha1 while introducing v1beta1” from the PR description.- The v1alpha1 schema retains
installNamespaceand the flatsignerName/subjectregistration fields, now explicitly documented as deprecated in favor of the structuredtype/csr/kubeClientmodel in v1beta1.- The v1beta1 schema for spec and status lines up with the new Go types in
addon/v1beta1/types_managedclusteraddon.go, includingHealthCheck.modedefaults,registrationsshape, andsupportedConfigsas a map list.- Omitting
spec.conversionand webhook clientConfig here is fine given that registration-operator patches conversion webhook configuration into these addon CRDs at install time. Based on learnings, this separation of base schema from deployment-specific webhook wiring is intentional.One optional clean-up: in the v1beta1
status.registrationsdescription, the text still refers to creating secrets “in the installNamespace”, but that field only exists in v1alpha1 spec; in v1beta1 the canonical field isstatus.namespace. Consider updating the comment in the Go type and regenerating the CRD at some point to avoid confusion for consumers reading the v1beta1 CRD docs.Based on learnings, webhook configuration being patched by registration-operator is expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
Makefile(1 hunks)addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(2 hunks)addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml(2 hunks)addon/v1alpha1/types_clustermanagementaddon.go(6 hunks)addon/v1alpha1/types_managedclusteraddon.go(1 hunks)addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(1 hunks)addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml(1 hunks)addon/v1beta1/doc.go(1 hunks)addon/v1beta1/register.go(1 hunks)addon/v1beta1/types_clustermanagementaddon.go(1 hunks)addon/v1beta1/types_managedclusteraddon.go(1 hunks)addon/v1beta1/zz_generated.deepcopy.go(1 hunks)client/addon/clientset/versioned/clientset.go(3 hunks)client/addon/clientset/versioned/fake/clientset_generated.go(2 hunks)client/addon/clientset/versioned/fake/register.go(1 hunks)client/addon/clientset/versioned/scheme/register.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/doc.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/doc.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_managedclusteraddon.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go(1 hunks)client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go(1 hunks)client/addon/informers/externalversions/addon/interface.go(2 hunks)client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/informers/externalversions/addon/v1beta1/interface.go(1 hunks)client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go(1 hunks)client/addon/informers/externalversions/generic.go(2 hunks)client/addon/listers/addon/v1beta1/clustermanagementaddon.go(1 hunks)client/addon/listers/addon/v1beta1/expansion_generated.go(1 hunks)client/addon/listers/addon/v1beta1/managedclusteraddon.go(1 hunks)hack/lib/init.sh(2 hunks)hack/verify-crds.sh(1 hunks)test/integration/api/clustermanagementaddon_test.go(2 hunks)test/integration/api/managedclusteraddon_test.go(2 hunks)test/integration/api/suite_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- client/addon/clientset/versioned/typed/addon/v1beta1/doc.go
- client/addon/clientset/versioned/scheme/register.go
- client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_managedclusteraddon.go
- client/addon/listers/addon/v1beta1/managedclusteraddon.go
- client/addon/informers/externalversions/addon/v1beta1/interface.go
- client/addon/informers/externalversions/generic.go
- client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_clustermanagementaddon.go
- addon/v1beta1/doc.go
- addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml
- client/addon/informers/externalversions/addon/interface.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.726Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
📚 Learning: 2025-12-01T01:10:01.726Z
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.726Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
Applied to files:
Makefiletest/integration/api/clustermanagementaddon_test.gotest/integration/api/managedclusteraddon_test.goclient/addon/listers/addon/v1beta1/expansion_generated.gotest/integration/api/suite_test.goclient/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.goaddon/v1alpha1/types_managedclusteraddon.goaddon/v1beta1/types_clustermanagementaddon.goaddon/v1beta1/register.goaddon/v1beta1/types_managedclusteraddon.gohack/verify-crds.shclient/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.goclient/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.goaddon/v1alpha1/types_clustermanagementaddon.goclient/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yamladdon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yamlclient/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.gohack/lib/init.sh
📚 Learning: 2025-07-02T14:09:55.079Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 380
File: client/work/clientset/versioned/fake/clientset_generated.go:40-40
Timestamp: 2025-07-02T14:09:55.079Z
Learning: Generated code files marked with "Code generated by client-gen. DO NOT EDIT." should not be manually edited as they are automatically generated by Kubernetes code-generator tools and any changes would be overwritten.
Applied to files:
Makefileclient/addon/listers/addon/v1beta1/expansion_generated.goclient/addon/clientset/versioned/typed/addon/v1beta1/fake/doc.goclient/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go
📚 Learning: 2025-09-02T11:59:50.703Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 392
File: operator/v1/types_clustermanager.go:178-195
Timestamp: 2025-09-02T11:59:50.703Z
Learning: XValidation markers in kubebuilder require Kubernetes 1.25+ and should not be added to the open-cluster-management-io/api project until minimum Kubernetes version requirements are established to avoid breaking compatibility with older clusters.
Applied to files:
addon/v1beta1/types_clustermanagementaddon.go
🧬 Code graph analysis (9)
test/integration/api/clustermanagementaddon_test.go (1)
addon/v1beta1/types_clustermanagementaddon.go (12)
ClusterManagementAddOn(21-32)ClusterManagementAddOnSpec(35-52)AddOnMeta(55-63)ConfigGroupResource(66-77)ConfigReferent(80-91)InstallStrategy(104-121)AddonInstallStrategyManual(125-125)DefaultConfigReference(183-189)ConfigSpecHash(94-100)InstallProgression(167-179)PlacementRef(131-142)InstallConfigReference(193-210)
client/addon/listers/addon/v1beta1/clustermanagementaddon.go (3)
addon/v1beta1/types_clustermanagementaddon.go (1)
ClusterManagementAddOn(21-32)client/addon/listers/addon/v1beta1/expansion_generated.go (1)
ClusterManagementAddOnListerExpansion(8-8)addon/v1beta1/register.go (1)
Resource(27-29)
client/addon/clientset/versioned/fake/register.go (3)
addon/v1beta1/register.go (1)
AddToScheme(22-22)client/addon/clientset/versioned/scheme/register.go (1)
AddToScheme(38-38)addon/v1alpha1/register.go (1)
AddToScheme(22-22)
client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go (5)
client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go (1)
ClusterManagementAddOnInterface(24-36)client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go (1)
ManagedClusterAddOnInterface(24-36)client/addon/clientset/versioned/clientset.go (1)
Interface(17-21)client/addon/informers/externalversions/addon/interface.go (1)
Interface(13-18)client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
Interface(11-16)
test/integration/api/managedclusteraddon_test.go (3)
addon/v1beta1/types_managedclusteraddon.go (9)
ManagedClusterAddOn(19-32)ManagedClusterAddOnSpec(36-43)AddOnConfig(95-101)RegistrationConfig(59-74)CSR(55-55)CSRConfig(82-93)KubeClient(51-51)KubeClientConfig(76-80)Subject(111-122)addon/v1beta1/types_clustermanagementaddon.go (3)
ConfigGroupResource(66-77)ConfigReferent(80-91)ConfigSpecHash(94-100)addon/v1beta1/register.go (1)
Resource(27-29)
addon/v1beta1/register.go (5)
client/addon/clientset/versioned/fake/register.go (1)
AddToScheme(38-38)client/addon/clientset/versioned/scheme/register.go (2)
AddToScheme(38-38)Scheme(16-16)addon/v1alpha1/types_clustermanagementaddon.go (2)
ClusterManagementAddOn(22-33)ClusterManagementAddOnList(253-262)addon/v1beta1/types_clustermanagementaddon.go (2)
ClusterManagementAddOn(21-32)ClusterManagementAddOnList(214-223)addon/v1beta1/types_managedclusteraddon.go (2)
ManagedClusterAddOn(19-32)ManagedClusterAddOnList(243-248)
client/addon/clientset/versioned/fake/clientset_generated.go (3)
client/addon/clientset/versioned/clientset.go (1)
Clientset(24-28)client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
AddonV1beta1Interface(14-18)client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go (1)
FakeAddonV1beta1(12-14)
client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (5)
client/addon/listers/addon/v1beta1/managedclusteraddon.go (2)
ManagedClusterAddOnLister(15-22)NewManagedClusterAddOnLister(30-32)client/addon/clientset/versioned/clientset.go (1)
Interface(17-21)client/addon/informers/externalversions/addon/v1beta1/interface.go (1)
Interface(11-16)work/v1alpha1/types_manifestworkreplicaset.go (1)
Background(195-195)addon/v1beta1/types_managedclusteraddon.go (1)
ManagedClusterAddOn(19-32)
addon/v1beta1/zz_generated.deepcopy.go (1)
addon/v1beta1/types_clustermanagementaddon.go (14)
ConfigGroupResource(66-77)ConfigReferent(80-91)AddOnMeta(55-63)ClusterManagementAddOn(21-32)ClusterManagementAddOnList(214-223)ClusterManagementAddOnSpec(35-52)InstallStrategy(104-121)ClusterManagementAddOnStatus(158-165)DefaultConfigReference(183-189)InstallProgression(167-179)ConfigSpecHash(94-100)InstallConfigReference(193-210)PlacementRef(131-142)PlacementStrategy(144-155)
⏰ 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 (31)
addon/v1alpha1/types_managedclusteraddon.go (1)
57-75: LGTM: deprecation docs for registration fields look correct and non-breakingThe added
Deprecated:comments onSignerNameandSubjectfollow Go doc conventions, clearly indicate the v1beta1 migration path, and do not affect the serialized API surface. No changes needed.hack/verify-crds.sh (1)
18-18: Include addon v1beta1 CRDs in verification (LGTM).Adding
addon/v1beta1/*.crd.yamlkeeps the new ManagedClusterAddOn/ClusterManagementAddOn v1beta1 CRDs under the same validation guarantees as other groups; no further changes needed here.addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml (1)
379-427: Deprecation text and status label updates are aligned with v1beta1.The added deprecation note for
supportedConfigsand the clarifieddefaultconfigReferencesdescription correctly point users toward v1beta1DefaultConfigs/default references, matching the new v1beta1 API design. No further CRD-side changes needed here.client/addon/clientset/versioned/typed/addon/v1beta1/fake/doc.go (1)
1-5: Generated fake client doc looks standard.The header and package comment match typical
client-genoutput; keeping this file generated-only (no manual edits) is appropriate.client/addon/clientset/versioned/typed/addon/v1beta1/fake/fake_addon_client.go (1)
1-29: FakeAddonV1beta1 wiring is consistent with existing fake clients.Embedding
*testing.Fake, delegating tonewFakeClusterManagementAddOns/newFakeManagedClusterAddOns, and returning a nil RESTClient follow the usual fake client pattern and are suitable for tests.addon/v1beta1/register.go (1)
1-41: v1beta1 scheme registration is complete and matches patterns.Group/version constants, deprecated aliases,
Resource, andaddKnownTypesall line up with the v1beta1 types (ClusterManagementAddOn/ManagedClusterAddOn and their lists), so consumers can register this version with a scheme without extra work.addon/v1beta1/zz_generated.deepcopy.go (1)
1-632: Deepcopy implementations align with v1beta1 types and appear correct.The generated DeepCopy/DeepCopyInto/DeepCopyObject methods cover all the new v1beta1 addon types, handle slices and pointer fields appropriately (e.g.,
Registrations,ConfigReferences,Subjectgroups/OUs, conditions), and follow standard deepcopy-gen patterns. This file should stay generated-only with no manual edits.test/integration/api/suite_test.go (1)
56-56: LGTM! v1beta1 CRDs properly loaded for integration tests.The addition of the v1beta1 CRD directory ensures the test environment can validate both v1alpha1 and v1beta1 addon resources.
Makefile (1)
39-39: LGTM! CRD generation correctly configured for v1beta1.The multi-version source paths (v1alpha1 and v1beta1) align with the approach used for other multi-version APIs in this repository.
client/addon/clientset/versioned/fake/clientset_generated.go (1)
16-17: LGTM! Generated fake clientset correctly wired for v1beta1.The v1beta1 imports and accessor method follow the established pattern from v1alpha1, properly exposing the fake client for testing.
Also applies to: 83-86
client/addon/clientset/versioned/fake/register.go (1)
13-13: LGTM! v1beta1 scheme properly registered.The addition of v1beta1.AddToScheme ensures the fake clientset can serialize and deserialize v1beta1 addon resources correctly.
Also applies to: 21-21
test/integration/api/clustermanagementaddon_test.go (1)
16-16: LGTM! Comprehensive v1beta1 test coverage for ClusterManagementAddOn.The v1beta1 test suite provides thorough validation of create operations, spec constraints, and status updates. The tests properly mirror the v1alpha1 suite structure while correctly using v1beta1 types and client interfaces.
Also applies to: 228-427
client/addon/clientset/versioned/typed/addon/v1beta1/generated_expansion.go (1)
1-8: LGTM! Standard expansion interfaces for v1beta1.The empty expansion interfaces provide extension points for future custom client methods without requiring modifications to generated code.
hack/lib/init.sh (1)
22-22: LGTM! v1beta1 properly registered in code generation scripts.The additions to API_GROUP_VERSIONS and API_PACKAGES ensure v1beta1 is included in all code generation workflows for clients, informers, and listers.
Also applies to: 33-34
test/integration/api/managedclusteraddon_test.go (1)
14-14: LGTM! Thorough v1beta1 test coverage for ManagedClusterAddOn.The v1beta1 test suite comprehensively validates the new structured registration types (CSR and KubeClient), configuration constraints, and status updates. The tests properly adapt to v1beta1's enhanced schema while maintaining consistency with the v1alpha1 test patterns.
Also applies to: 327-623
client/addon/listers/addon/v1beta1/expansion_generated.go (1)
1-16: LGTM! Generated expansion interfaces.This generated file provides standard extension hooks for the v1beta1 listers, enabling custom methods to be added without modifying generated code.
client/addon/listers/addon/v1beta1/clustermanagementaddon.go (1)
1-33: LGTM! Generated lister implementation.This generated lister correctly wires the v1beta1 ClusterManagementAddOn resource using the standard Kubernetes client-go lister pattern.
client/addon/clientset/versioned/clientset.go (1)
14-114: LGTM! v1beta1 client wiring.The generated clientset correctly integrates v1beta1 addon client alongside the existing v1alpha1 client, following the standard Kubernetes client-gen pattern.
addon/v1alpha1/types_clustermanagementaddon.go (2)
48-56: Good deprecation notice for v1beta1 migration.The deprecation comment clearly indicates that
supportedConfigswill be replaced withDefaultConfigsin v1beta1, providing a clear migration path.
114-179: Validation marker syntax corrected.The kubebuilder validation markers now use the correct
=syntax instead of:=, ensuring validation rules are properly applied during CRD generation.addon/v1beta1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml (3)
1-16: LGTM! CRD metadata and scope configuration.The CRD is properly configured as cluster-scoped with appropriate names, shortnames, and preserveUnknownFields set to false.
17-632: v1alpha1 version correctly marked as deprecated.The v1alpha1 version includes proper deprecation warnings and remains served with storage enabled to support existing resources during the migration period.
633-1206: v1beta1 version schema is well-structured.The v1beta1 schema correctly:
- Omits the deprecated
addOnConfigurationandsupportedConfigsfields- Introduces
defaultConfigsas the replacement- Removes the "CRD NAME" printer column that referenced the deprecated field
- Is served but not the storage version, allowing gradual migration
Based on learnings, the intentional absence of conversion webhook configuration is expected, as it will be patched at deployment time by the registration-operator.
client/addon/clientset/versioned/typed/addon/v1beta1/addon_client.go (1)
1-91: LGTM! Generated v1beta1 REST client.This generated client correctly implements the standard Kubernetes REST client pattern for the addon.open-cluster-management.io/v1beta1 API group.
addon/v1beta1/types_clustermanagementaddon.go (3)
75-140: Validation marker syntax is correct.The kubebuilder validation markers correctly use
=syntax for MinLength constraints, ensuring proper CRD validation generation.
34-52: Well-structured v1beta1 spec.The
ClusterManagementAddOnSpeccorrectly introducesDefaultConfigs(line 45) as the replacement for the deprecated v1alpha1supportedConfigsfield, providing a clear migration path.
225-238: Lifecycle annotation constants maintained.The addon lifecycle annotation constants are properly carried over to v1beta1, ensuring backward compatibility for existing addons that rely on these annotations.
client/addon/informers/externalversions/addon/v1beta1/clustermanagementaddon.go (1)
1-86: LGTM! Generated v1beta1 informer.This generated informer correctly implements the standard Kubernetes informer pattern for ClusterManagementAddOn resources, with proper List/Watch wiring and context support.
client/addon/informers/externalversions/addon/v1beta1/managedclusteraddon.go (1)
1-87: LGTM! Generated informer follows standard patterns.This generated informer implementation correctly follows standard Kubernetes client-go patterns for namespace-scoped resources, with proper List/Watch lifecycle, context-aware variants, factory integration, and indexer setup.
client/addon/clientset/versioned/typed/addon/v1beta1/managedclusteraddon.go (1)
1-55: LGTM! Generated typed client follows standard patterns.This generated typed client implementation correctly follows standard Kubernetes client-go patterns for namespace-scoped resources, using the modern
gentype.ClientWithListapproach with proper resource naming and full CRUD interface.client/addon/clientset/versioned/typed/addon/v1beta1/clustermanagementaddon.go (1)
1-55: LGTM! Generated typed client follows standard patterns.This generated typed client implementation correctly follows standard Kubernetes client-go patterns for cluster-scoped resources, using the modern
gentype.ClientWithListapproach with proper resource naming and full CRUD interface.
Signed-off-by: Qing Hao <[email protected]>
haoqing0110
force-pushed
the
br_addon-v1beta1
branch
from
538c3e6 to
6f971dd
Compare
|
/unhold |
| // AddonLifecycleAnnotationKey is an annotation key on ClusterManagementAddon to indicate the installation | ||
| // and upgrade of addon should be handled by the general addon manager or addon itself. The valid values are | ||
| // addon-manager and self. If the annotation is not set, addon lifecycle is handled by addon itself. | ||
| AddonLifecycleAnnotationKey = "addon.open-cluster-management.io/lifecycle" |
There was a problem hiding this comment.
do we still need to keep this? Should all addons lifecycle managed by addon-manager in beta?
There was a problem hiding this comment.
Today the logic of ManagedByAddonManager is:
- if this annotation is empty or "addon-manager", it is managed by addon-manager
- if it's self, which means using the addon-framework before 0.10.0, will not be managed by addon-manager
We can remove this in beta, and once OCM (addon-manager) adopt v1beta1, we should claim that the addon developed with addon-framework 0.9.* can no longer be deployed in the latest OCM.
| const ( | ||
| // KubeClient represents the registration type for addon agents that need to access | ||
| // the hub kube-apiserver using kubeClient. | ||
| KubeClient RegistrationType = "KubeClient" |
| type KubeClientConfig struct { | ||
| // subject is the user subject of the addon agent to be registered to the hub. | ||
| // +optional | ||
| Subject Subject `json:"subject,omitempty"` |
There was a problem hiding this comment.
kubeClient subject should not have organization unit.
| // "subject": { | ||
| // "user": "system:open-cluster-management:cluster:{clusterName}:addon:{addonName}:agent:{agentName}", | ||
| // "groups: ["system:open-cluster-management:cluster:{clusterName}:addon:{addonName}", | ||
| // "system:open-cluster-management:addon:{addonName}", "system:authenticated"] |
There was a problem hiding this comment.
I do not think it should include "system:authenticated"? @zhujian7
|
|
||
| // AddonLabelKey is the label key to set addon name. It is to set on the resources on the hub relating | ||
| // to an addon | ||
| AddonLabelKey = "open-cluster-management.io/addon-name" |
There was a problem hiding this comment.
please also specify which resource this label will be added to.
| } | ||
|
|
||
| const ( | ||
| // Label and annotation keys set on ManagedClusterAddon. |
There was a problem hiding this comment.
I do not think all these are set a managedclusteraddon resource. It is better that we group them based on which object the labels/annotations will be set.
| $(call add-crd-gen,workv1,./work/v1,./work/v1,./work/v1) | ||
| $(call add-crd-gen,workv1alpha1,./work/v1alpha1,./work/v1alpha1,./work/v1alpha1) | ||
| $(call add-crd-gen,operator,./operator/v1,./operator/v1,./operator/v1) | ||
| $(call add-crd-gen,addonv1alpha1,./addon/v1alpha1,./addon/v1alpha1,./addon/v1alpha1) |
There was a problem hiding this comment.
should we include all crds here? I think we should not include msa and cma
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.