🌱 Update godoc of APIs to follow jsonField pattern #395
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughLowercased the first word in numerous CRD/OpenAPI descriptions and Go doc comments across addon, cluster, operator, and work APIs. One schema note: clustersPerDecisionGroup in v1beta1 Placement CRD keeps IntOrString support (integer or percentage string) with expanded documentation. Fixed a “PacementRefs” typo in a v1alpha1 work CRD description. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
cluster/v1alpha1/types.go (1)
30-34: Propagate MinLength annotation to CRD
The// +kubebuilder:validation:MinLength=1annotation intypes.goisn’t reflected asminLength: 1incluster/v1alpha1/0000_02_clusters.open-cluster-management.io_clusterclaims.crd.yaml. Rerun code generation to sync the CRD schema.cluster/v1/types.go (1)
95-116: Fix kubebuilder markers to use “=” instead of “:=”
Kubebuilder controller-gen expects markers like+kubebuilder:validation:Enum=…and+kubebuilder:default=…. Replace all instances of:=in your markers (Enum, default, MinLength, Maximum, Minimum) with=. For example in cluster/v1/types.go:-// +kubebuilder:validation:Enum:=NoSelect;PreferNoSelect;NoSelectIfNew +// +kubebuilder:validation:Enum=NoSelect;PreferNoSelect;NoSelectIfNewRun
rg -nP '^\s*//\s*\+kubebuilder:[^=]*:='to locate and correct all occurrences.cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml (3)
91-110: Validation gap: allowlist integers ≥1; keep percent regex; small grammar fixes
- Integer variant currently allows 0/negatives; constrain to ≥1.
- Keep percent regex; minor wording adjustments and remove duplicate words later in the block.
clustersPerDecisionGroup: anyOf: - - type: integer - - type: string + - type: integer + minimum: 1 + - type: string default: 100% - description: |- - clustersPerDecisionGroup is a specific number or percentage of the total selected clusters. - The specific number will divide the placementDecisions to decisionGroups each group has max number of clusters - equal to that specific number. - The percentage will divide the placementDecisions to decisionGroups each group has max number of clusters based - on the total num of selected clusters and percentage. - ex; for a total 100 clusters selected, ClustersPerDecisionGroup equal to 20% will divide the placement decision - to 5 groups each group should have 20 clusters. - Default is having all clusters in a single group. - - The predefined decisionGroups is expected to be a subset of the selected clusters and the number of items in each - group SHOULD be less than ClustersPerDecisionGroup. Once the number of items exceeds the ClustersPerDecisionGroup, - the decisionGroups will also be be divided into multiple decisionGroups with same GroupName but different GroupIndex. + description: |- + clustersPerDecisionGroup is a specific number or a percentage of the total selected clusters. + A specific number divides the PlacementDecisions into decisionGroups where each group has at most + that number of clusters. + A percentage divides the PlacementDecisions into decisionGroups where each group has at most + the percentage of the total selected clusters. + For example, with 100 selected clusters and 20%, the PlacementDecisions are divided into 5 groups, + each with 20 clusters. + The default places all clusters into a single group. + + The predefined decisionGroups are expected to be a subset of the selected clusters, and the number of items in each + group SHOULD be less than or equal to clustersPerDecisionGroup. If the number of items exceeds clustersPerDecisionGroup, + the decisionGroups will also be divided into multiple decisionGroups with the same groupName but different groupIndex.
112-116: Casing and clarity for field referencesUse jsonField casing and tighten wording.
- Decision groups will be constructed based on the DecisionGroups field at first. The clusters not included in the - DecisionGroups will be divided to other decision groups afterwards. Each decision group should not have the number - of clusters larger than the ClustersPerDecisionGroup. + Decision groups are constructed from decisionGroups first. Clusters not included there + are then divided into other decision groups. Each decision group must not contain more + clusters than clustersPerDecisionGroup.
226-237: Fix repeated “nubmer” typos and field casing; tighten bulletsAlso reference the json field name consistently.
- numberOfClusters represents the desired number of ManagedClusters to be selected which meet the - placement requirements. + numberOfClusters represents the desired number of ManagedClusters to be selected that meet the + placement requirements. 1) If not specified, all ManagedClusters which meet the placement requirements (including ClusterSets, and Predicates) will be selected; - 2) Otherwise if the nubmer of ManagedClusters meet the placement requirements is larger than - NumberOfClusters, a random subset with desired number of ManagedClusters will be selected; - 3) If the nubmer of ManagedClusters meet the placement requirements is equal to NumberOfClusters, + 2) Otherwise, if the number of ManagedClusters that meet the placement requirements is larger than + numberOfClusters, a random subset with the desired number of ManagedClusters will be selected; + 3) If the number of ManagedClusters that meet the placement requirements is equal to numberOfClusters, all of them will be selected; - 4) If the nubmer of ManagedClusters meet the placement requirements is less than NumberOfClusters, + 4) If the number of ManagedClusters that meet the placement requirements is less than numberOfClusters, all of them will be selected, and the status of condition `PlacementConditionSatisfied` will be set to false;
🧹 Nitpick comments (26)
cluster/v1/types.go (1)
72-77: Grammar nit on taints sentence.Minor subject-verb agreement.
-// taints is a property of managed cluster that allow the cluster to be repelled when scheduling. +// taints is a property of managed cluster that allows the cluster to be repelled when scheduling.cluster/v1beta1/types_placementdecision.go (1)
41-46: Lowercased “decisions” doc; consider enforcing the 100 limit.If you want to guarantee the cap, add MaxItems validation.
// +kubebuilder:validation:Required // +required +// +kubebuilder:validation:MaxItems=100 Decisions []ClusterDecision `json:"decisions"`work/v1/types.go (1)
68-71: Fix typo: “kuberenetes” -> “Kubernetes”.User-facing docs; low-cost improvement.
-// manifests represents a list of kuberenetes resources to be deployed on a managed cluster. +// manifests represents a list of Kubernetes resources to be deployed on a managed cluster.addon/v1alpha1/types_addondeploymentconfig.go (2)
32-38: Tighten NodePlacement wording.
Streamline the nil/empty-object sentences for clarity.-// If the placement is nil, the placement is not specified, it will be omitted. -// If the placement is an empty object, the placement will match all nodes and tolerate nothing. +// If the placement is nil, it is omitted (unspecified). +// If the placement is an empty object, it matches all nodes and tolerates nothing.
40-49: Consistent capitalization and hyphenation in registries example.
Capitalize the new sentence and use “add-on” consistently.-// the following example will override image "quay.io/open-cluster-management/addon-agent" to -// "quay.io/ocm/addon-agent" when deploying the addon agent +// The following example overrides image "quay.io/open-cluster-management/addon-agent" to +// "quay.io/ocm/addon-agent" when deploying the add-on agentcluster/v1alpha1/types_rolloutstrategy.go (1)
142-149: Use consistent field-cased reference in prose.
Keep “maxConcurrency” cased like the json field.-// for MaxConcurrency is determined from the clustersPerDecisionGroup defined in the +// for maxConcurrency is determined from the clustersPerDecisionGroup defined in theaddon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml (1)
230-233: Keep field name casing consistent in description.
Use “maxConcurrency” instead of “MaxConcurrency”.- description: |- - maxConcurrency is the max number of clusters to deploy workload concurrently. The default value - for MaxConcurrency is determined from the clustersPerDecisionGroup defined in the - placement->DecisionStrategy. + description: |- + maxConcurrency is the max number of clusters to deploy workload concurrently. The default value + for maxConcurrency is determined from the clustersPerDecisionGroup defined in the + placement->decisionStrategy.cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml (2)
116-120: Fix minor grammar/typos in taints description.- taints is a property of managed cluster that allow the cluster to be repelled when scheduling. - Taints, including 'ManagedClusterUnavailable' and 'ManagedClusterUnreachable', can not be added/removed by agent - running on the managed cluster; while it's fine to add/remove other taints from either hub cluser or managed cluster. + taints is a property of a managed cluster that allows the cluster to be repelled when scheduling. + Taints, including 'ManagedClusterUnavailable' and 'ManagedClusterUnreachable', cannot be added/removed by the agent + running on the managed cluster; while it's fine to add/remove other taints from either hub cluster or managed cluster.
352-353: Add missing article for readability.- description: kubernetes is the kubernetes version of managed cluster. + description: kubernetes is the kubernetes version of the managed cluster.cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml (2)
68-73: Subject-verb agreement and wording.- description: clusterSelector represents a selector of ManagedClusters + description: clusterSelector represents a selector of ManagedClusters properties: labelSelector: - description: labelSelector define the general labelSelector which + description: labelSelector defines the general labelSelector which clusterset will use to select target managedClusters
120-123: Modal verb: “can only be” reads better.- selectorType could only be "ExclusiveClusterSetLabel" or "LabelSelector" + selectorType can only be "ExclusiveClusterSetLabel" or "LabelSelector"operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml (1)
48-52: Minor grammar improvement.- clusterName is the name of the managed cluster to be created on hub. + clusterName is the name of the managed cluster to be created on the hub.cluster/v1alpha1/types_addonplacementscore.go (1)
51-55: Add terminal period for consistency.- // name is the name of the score + // name is the name of the score.cluster/v1alpha1/0000_05_clusters.open-cluster-management.io_addonplacementscores.crd.yaml (1)
111-116: Match punctuation with Go doc.- description: name is the name of the score + description: name is the name of the score.cluster/v1beta2/types_managedclusterset.go (1)
71-74: Nit: consider grammar tweak for clarity.“labelSelector define the general labelSelector…” → “labelSelector defines the general label selector…”. If you want, I can sweep similar minor grammar issues across the file.
work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml (1)
48-50: Doc casing OK; keep “manifestwork” capitalization consistent repo-wide.No schema impact here.
addon/v1alpha1/types_addontemplate.go (2)
35-39: Casing updated correctly; slight grammar polish suggested.Consider “addonName represents the name of the add-on that this template belongs to.”
-// addonName represents the name of the addon which the template belongs to +// addonName represents the name of the add-on that this template belongs to.
40-44: Polish phrasing for agentSpec sentence.The current “what/how … to be deployed” is awkward.
-// agentSpec describes what/how the kubernetes resources of the addon agent to be deployed on a managed cluster. +// agentSpec describes how the add-on agent's Kubernetes resources are deployed on a managed cluster.addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yaml (1)
57-59: Doc casing matches json field; suggest grammar tweak in a follow-up.If desired, mirror the Go comment suggestion for clarity.
cluster/v1beta1/types_placement.go (2)
66-79: Lowercasing reads fine; retain “Predicates” capitalization only when referring to type names.No action required.
169-177: Doc casing for requiredClusterSelector OK; minor grammar nit available.“be selected or at least has a chance” → “be selected or at least have a chance”.
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml (5)
163-165: End punctuation and consistencyAdd period; reads more polished.
- description: labelSelector represents a selector - of ManagedClusters by label + description: labelSelector represents a selector + of ManagedClusters by label.
247-253: Wording consistencyConsider “PlacementDecisions” vs “placement decisions” consistency. Otherwise OK.
301-303: End punctuation- description: labelSelector represents a selector of ManagedClusters - by label + description: labelSelector represents a selector of ManagedClusters + by label.
409-415: Tighten wording; keep same meaning- weight defines the weight of the prioritizer score. The value must be ranged in [-10,10]. + weight defines the weight of the prioritizer score. The value must be in the range [-10, 10].
637-639: End punctuationAdd period for consistency.
- description: numberOfSelectedClusters represents the number of selected - ManagedClusters + description: numberOfSelectedClusters represents the number of selected + ManagedClusters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(6 hunks)addon/v1alpha1/0000_02_addon.open-cluster-management.io_addondeploymentconfigs.crd.yaml(3 hunks)addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yaml(4 hunks)addon/v1alpha1/types_addondeploymentconfig.go(1 hunks)addon/v1alpha1/types_addontemplate.go(1 hunks)cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml(6 hunks)cluster/v1/types.go(3 hunks)cluster/v1alpha1/0000_02_clusters.open-cluster-management.io_clusterclaims.crd.yaml(1 hunks)cluster/v1alpha1/0000_05_clusters.open-cluster-management.io_addonplacementscores.crd.yaml(2 hunks)cluster/v1alpha1/types.go(1 hunks)cluster/v1alpha1/types_addonplacementscore.go(2 hunks)cluster/v1alpha1/types_rolloutstrategy.go(3 hunks)cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml(18 hunks)cluster/v1beta1/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yaml(2 hunks)cluster/v1beta1/types_placement.go(9 hunks)cluster/v1beta1/types_placementdecision.go(2 hunks)cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml(2 hunks)cluster/v1beta2/0000_01_clusters.open-cluster-management.io_managedclustersetbindings.crd.yaml(1 hunks)cluster/v1beta2/types_managedclusterset.go(2 hunks)cluster/v1beta2/types_managedclustersetbinding.go(1 hunks)operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml(2 hunks)operator/v1/types_klusterlet.go(2 hunks)work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml(3 hunks)work/v1/types.go(2 hunks)work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml(10 hunks)work/v1alpha1/types_manifestworkreplicaset.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
addon/v1alpha1/types_addontemplate.go (1)
work/v1/types.go (1)
ManifestWorkSpec(37-57)
work/v1alpha1/types_manifestworkreplicaset.go (1)
work/v1/types.go (1)
ManifestWorkSpec(37-57)
⏰ 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 (81)
cluster/v1beta2/0000_01_clusters.open-cluster-management.io_managedclustersetbindings.crd.yaml (1)
50-53: CRD description lowercased; LGTM.Matches the corresponding Go doc in types_managedclustersetbinding.go.
operator/v1/types_klusterlet.go (2)
32-43: Lowercased “namespace” doc; LGTM.No behavior change; aligns with json tag.
60-66: Lowercased “clusterName” doc; LGTM.No behavior change; aligns with json tag.
cluster/v1alpha1/0000_02_clusters.open-cluster-management.io_clusterclaims.crd.yaml (1)
49-52: CRD spec.value description lowercased; LGTM.In sync with cluster/v1alpha1/types.go.
cluster/v1beta2/types_managedclustersetbinding.go (1)
33-39: Lowercased “clusterSet” doc; LGTM.Docs align with JSON tag; no functional changes.
cluster/v1/types.go (3)
66-71: Lowercased “leaseDurationSeconds” doc; LGTM.No functional changes.
180-183: Lowercased “kubernetes” doc; LGTM.Matches style used elsewhere.
187-197: Lowercased ManagedClusterClaim name/value docs; LGTM.Consistent with json tags; no behavior change.
cluster/v1beta1/types_placementdecision.go (2)
51-56: Lowercased “clusterName” doc; LGTM.
57-61: Lowercased “reason” doc; LGTM.work/v1/types.go (1)
38-49: Lowercased workload/deleteOption/manifestConfigs docs; LGTM.Descriptions now mirror JSON field names.
addon/v1alpha1/types_addondeploymentconfig.go (1)
24-30: LGTM on jsonField-cased doc for customizedVariables.cluster/v1alpha1/types_rolloutstrategy.go (2)
41-52: LGTM on jsonField-cased sub-struct docs (all/progressive/progressivePerGroup).
98-107: LGTM on groupName/groupIndex phrasing.addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml (5)
151-153: LGTM: lowercased “all” doc matches code style.
198-201: LGTM: lowercased “progressive” doc.
213-221: LGTM: groupIndex/groupName descriptions.
279-281: LGTM: lowercased “progressivePerGroup” doc.
295-303: LGTM: groupIndex/groupName descriptions.cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml (5)
87-92: LGTM: leaseDurationSeconds wording.
127-129: LGTM: effect description.
136-138: LGTM: key description.
142-149: LGTM: timeAdded/value descriptions.
195-202: LGTM: name/value lowercasing in ClusterClaim.operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml (1)
115-122: LGTM: namespace description lowercased.cluster/v1alpha1/types_addonplacementscore.go (2)
40-46: LGTM: validUntil doc matches json field casing.
56-62: LGTM: value doc and validation.cluster/v1alpha1/0000_05_clusters.open-cluster-management.io_addonplacementscores.crd.yaml (1)
129-133: LGTM: validUntil description alignment.cluster/v1beta2/types_managedclusterset.go (2)
48-52: Docs-only casing change looks good.Lowercasing to match json field names is consistent and keeps API surface unchanged.
63-69: Consistent lowercasing in selectorType doc.Matches the json tag and improves doc consistency.
work/v1alpha1/types_manifestworkreplicaset.go (2)
58-60: Good: jsonField-style doc casing for manifestWorkTemplate.No API changes; matches spec intent.
61-67: Typo fix and casing for placementRefs are correct.Reads better and aligns with json tag.
work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml (2)
159-160: Doc casing OK.Matches the field name.
374-376: Doc casing OK.Aligns with json field.
addon/v1alpha1/types_addontemplate.go (1)
45-48: Casing update looks good.No functional change.
addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yaml (5)
53-55: Doc casing matches json field.Looks good.
62-64: Doc casing OK for deleteOption.No schema impact.
174-176: Doc casing OK for manifestConfigs.No schema impact.
391-393: Doc casing OK for workload.Matches other files.
407-409: Doc casing OK for registration.Consistent with Go docs.
cluster/v1beta1/types_placement.go (14)
59-65: Doc casing for clusterSets is consistent.Matches json tag and intent.
80-83: Doc casing for predicates OK.No API change.
84-89: Doc casing for prioritizerPolicy OK.No API change.
90-94: Doc casing for spreadPolicy OK.No API change.
95-99: Doc casing for tolerations OK.No API change.
100-103: Doc casing for decisionStrategy OK.No API change.
133-139: DecisionGroups docs improved; ensure label keys match constants in controllers.Label names look correct; just flagging to double-check controller code.
162-165: Doc casing for groupStrategy OK.Matches json tag.
182-193: Doc casing for labelSelector/claimSelector/celSelector OK.No API change.
237-253: Doc casing for scoreCoordinate/weight OK.Constraints and defaults look consistent.
257-266: Doc casing for ScoreCoordinate.type OK; ensure enums align with constants.BuiltIn/AddOn match declared constants.
288-300: Doc casing for AddOnScore fields OK.No API change.
431-434: Doc casing for numberOfSelectedClusters OK.Matches json tag.
140-158: Confirm CRD schema and verify consumers handle IntOrString
- CRD for
clustersPerDecisionGroupnow defines an anyOf [int, string], includesx-kubernetes-int-or-string, enforces the percentage/integer pattern, and defaults to"100%".- Manually verify all controllers, webhooks and tests correctly accept and round-trip both integer and percentage values (e.g. existing persisted integers or new “%” strings) without errors.
addon/v1alpha1/0000_02_addon.open-cluster-management.io_addondeploymentconfigs.crd.yaml (3)
51-54: Doc casing for customizedVariables OK.Matches field name.
76-81: Doc casing for nodePlacement OK.Reads clearly.
159-166: Doc casing for registries OK.Example is helpful; no schema change.
work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml (9)
76-78: Doc casing for manifestWorkTemplate OK.No schema change.
81-83: Doc casing for deleteOption OK.No schema change.
193-195: Doc casing for manifestConfigs OK.No schema change.
410-412: Doc casing for workload OK.Matches json field.
427-429: Doc casing for placementRefs OK.The description reads better now.
447-448: Lowercased “all” label heading; OK.Doc-only.
494-496: Lowercased “progressive” section heading; OK.Doc-only.
510-517: Label-key references correct; ensure controller uses exact keys.Names match cluster.open-cluster-management.io/decision-group-index|name.
525-529: Reference to clustersPerDecisionGroup default ties to v1beta1 Placement change.Good cross-doc link; once the IntOrString change is confirmed, this looks consistent.
cluster/v1beta1/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yaml (2)
51-53: Lowercasing field description — LGTMMatches the jsonField pattern and reads well.
64-66: Lowercasing field description — LGTMcluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml (13)
75-81: Lowercasing “clusterSets” description — LGTM
125-127: Lowercasing alignment — LGTM
240-242: Lowercasing alignment — LGTM
255-257: Lowercasing alignment — LGTM
264-266: Lowercasing alignment — LGTM
352-355: Lowercasing alignment — LGTM
362-364: Lowercasing alignment — LGTM
371-373: Lowercasing alignment — LGTM
395-399: Lowercasing alignment — LGTM
437-439: Lowercasing alignment — LGTM
493-495: Lowercasing alignment — LGTM
91-110: Verify controller handling of clustersPerDecisionGroup parsing and rounding
I didn’t find any code in the scheduler/controller that reads or converts theclustersPerDecisionGroupIntOrString field. Please confirm it accepts both integer and percentage inputs and applies the documented rounding rules when dividing placement decisions.
96-104: Clarify rounding behavior for percentage-based clustersPerDecisionGroup
WhentotalSelectedClusters × percentageisn’t an integer (e.g. 33 × 20% = 6.6), state in the CRD whether to round up, down, or to the nearest integer (for example, “rounded up using ceiling”). Confirm that the placement controller/scheduler implements the same rule.
| description: manifests represents a list of kuberenetes resources | ||
| to be deployed on a managed cluster. |
There was a problem hiding this comment.
Fix misspelling: “kuberenetes” → “Kubernetes”.
CRD descriptions are user-facing.
- description: manifests represents a list of kuberenetes resources
+ description: manifests represents a list of Kubernetes resources📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: manifests represents a list of kuberenetes resources | |
| to be deployed on a managed cluster. | |
| description: manifests represents a list of Kubernetes resources | |
| to be deployed on a managed cluster. |
🤖 Prompt for AI Agents
In
addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yaml
around lines 395 to 396, fix the misspelling in the CRD description by changing
"kuberenetes" to "Kubernetes" (capitalize properly) so the description reads
"...represents a list of Kubernetes resources to be deployed on a managed
cluster."
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml
Outdated
Show resolved
Hide resolved
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml
Outdated
Show resolved
Hide resolved
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml
Outdated
Show resolved
Hide resolved
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml
Outdated
Show resolved
Hide resolved
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml
Outdated
Show resolved
Hide resolved
cluster/v1beta1/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yaml
Outdated
Show resolved
Hide resolved
| description: manifests represents a list of kuberenetes resources | ||
| to be deployed on a managed cluster. |
There was a problem hiding this comment.
Fix misspelling: “kuberenetes” → “Kubernetes”.
User-facing CRD docs should avoid typos.
Apply this diff to the description line:
- description: manifests represents a list of kuberenetes resources
+ description: manifests represents a list of Kubernetes resources📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: manifests represents a list of kuberenetes resources | |
| to be deployed on a managed cluster. | |
| description: manifests represents a list of Kubernetes resources | |
| to be deployed on a managed cluster. |
🤖 Prompt for AI Agents
In work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml around
lines 378 to 379, the description contains a typo "kuberenetes"; update the
description text to replace "kuberenetes" with the correct "Kubernetes"
(preserve surrounding wording and punctuation so the sentence reads
"...represents a list of Kubernetes resources to be deployed on a managed
cluster.").
| description: manifests represents a list of kuberenetes resources | ||
| to be deployed on a managed cluster. | ||
| items: |
There was a problem hiding this comment.
Fix misspelling: “kuberenetes” → “Kubernetes”.
User-facing docs.
- description: manifests represents a list of kuberenetes resources
+ description: manifests represents a list of Kubernetes resources📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: manifests represents a list of kuberenetes resources | |
| to be deployed on a managed cluster. | |
| items: | |
| @@ -414,3 +414,3 @@ | |
| description: manifests represents a list of Kubernetes resources | |
| to be deployed on a managed cluster. | |
| items: |
🤖 Prompt for AI Agents
In
work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml
around lines 414 to 416, the description contains a misspelling: "kuberenetes"
should be corrected to "Kubernetes"; update the string to read "manifests
represents a list of Kubernetes resources to be deployed on a managed cluster."
and ensure capitalization of "Kubernetes" is preserved in the user-facing
description.
Signed-off-by: Jian Qiu <[email protected]>
qiujian16
force-pushed
the
fix-type-description
branch
from
73b771d to
4eedbaa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml (2)
105-108: Fix typos and agreement; remove duplicate “be”.“decisionGroups is” → “decisionGroups are” and “be be” duplication.
Apply:
- The predefined decisionGroups is expected to be a subset of the selected clusters and the number of items in each - group SHOULD be less than ClustersPerDecisionGroup. Once the number of items exceeds the ClustersPerDecisionGroup, - the decisionGroups will also be be divided into multiple decisionGroups with same GroupName but different GroupIndex. + The predefined decisionGroups are expected to be a subset of the selected clusters, and the number of items in each + group SHOULD be less than clustersPerDecisionGroup. Once the number of items exceeds clustersPerDecisionGroup, + the decisionGroups will be divided into multiple decisionGroups with the same groupName but different groupIndex.
226-237: Fix repeated typo “nubmer” and tighten bullets.Multiple occurrences of “nubmer”; also streamline phrasing.
Apply:
- 2) Otherwise if the nubmer of ManagedClusters meet the placement requirements is larger than - NumberOfClusters, a random subset with desired number of ManagedClusters will be selected; - 3) If the nubmer of ManagedClusters meet the placement requirements is equal to NumberOfClusters, - all of them will be selected; - 4) If the nubmer of ManagedClusters meet the placement requirements is less than NumberOfClusters, - all of them will be selected, and the status of condition `PlacementConditionSatisfied` will be - set to false; + 2) If the number of ManagedClusters that meet the placement requirements is greater than + numberOfClusters, a random subset of the desired size will be selected; + 3) If the number of ManagedClusters that meet the placement requirements equals numberOfClusters, + all of them will be selected; + 4) If the number of ManagedClusters that meet the placement requirements is less than numberOfClusters, + all of them will be selected, and the condition `PlacementConditionSatisfied` will be set to False;
♻️ Duplicate comments (5)
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml (5)
83-85: Grammar fix applied correctly for decisionStrategy.The subject–verb agreement and phrasing look good now.
87-89: Grammar fix applied correctly for groupStrategy.Reads clearly and uses the right preposition “into.”
121-123: groupClusterSelector wording looks good.Matches prior suggestion and is clear.
212-215: groupName clarification looks good and uses correct Kind.
376-379: scoreName grammar fix looks good.Pluralization and verb agreement are correct now.
🧹 Nitpick comments (8)
cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml (8)
96-104: Clarify clustersPerDecisionGroup description; fix awkward phrasing and example.Current text mixes pluralization, uses “num,” and reads awkwardly. Suggest clearer wording.
Apply:
- clustersPerDecisionGroup is a specific number or percentage of the total selected clusters. - The specific number will divide the placementDecisions to decisionGroups each group has max number of clusters - equal to that specific number. - The percentage will divide the placementDecisions to decisionGroups each group has max number of clusters based - on the total num of selected clusters and percentage. - ex; for a total 100 clusters selected, ClustersPerDecisionGroup equal to 20% will divide the placement decision - to 5 groups each group should have 20 clusters. - Default is having all clusters in a single group. + clustersPerDecisionGroup may be either a specific number or a percentage of the total selected clusters. + A specific number divides the placement decisions into decision groups, each with at most that many clusters. + A percentage divides the placement decisions into decision groups, each with at most + a number of clusters derived from the total selected clusters and the given percentage. + For example, with 100 total selected clusters, clustersPerDecisionGroup = 20% yields 5 groups + with up to 20 clusters each. + The default places all clusters into a single group.
112-116: Tighten decisionGroups wording and prepositions.Use “into,” avoid redundancy, and prefer “must not exceed.”
Apply:
- decisionGroups represents a list of predefined groups to put decision results. - Decision groups will be constructed based on the DecisionGroups field at first. The clusters not included in the - DecisionGroups will be divided to other decision groups afterwards. Each decision group should not have the number - of clusters larger than the ClustersPerDecisionGroup. + decisionGroups represents a list of predefined groups for decision results. + Decision groups are constructed from the DecisionGroups field first. Clusters not included there + are then divided into other decision groups. Each decision group must not exceed clustersPerDecisionGroup.
125-126: Be explicit about the field path for clarity.Apply:
- description: claimSelector represents a selector - of ManagedClusters by clusterClaims in status + description: claimSelector represents a selector + of ManagedClusters by claims in status.clusterClaims
163-164: Pluralize for consistency with similar text above.Apply:
- description: labelSelector represents a selector - of ManagedClusters by label + description: labelSelector represents a selector + of ManagedClusters by labels
352-355: Smoother cross-reference phrasing.Apply:
- Referring to PrioritizerPolicy to see more description about Mode and Configurations. + Refer to PrioritizerPolicy for details about Mode and Configurations.
395-399: Use normative “must” instead of “need to.”Apply:
- When the type is "BuiltIn", need to specify a BuiltIn prioritizer name in BuiltIn. - When the type is "AddOn", need to configure the score source in AddOn. + When the type is "BuiltIn", you must specify a BuiltIn prioritizer name in BuiltIn. + When the type is "AddOn", you must configure the score source in AddOn.
409-415: Minor grammar and math notation.Apply:
- weight defines the weight of the prioritizer score. The value must be ranged in [-10,10]. + weight defines the weight of the prioritizer score. The value must be in the range [-10, 10]. Each prioritizer will calculate an integer score of a cluster in the range of [-100, 100]. - The final score of a cluster will be sum(weight * prioritizer_score). + The final score of a cluster is the sum of (weight × prioritizer_score).
637-639: Punctuation consistency.Apply:
- description: numberOfSelectedClusters represents the number of selected - ManagedClusters + description: numberOfSelectedClusters represents the number of selected + ManagedClusters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml(6 hunks)addon/v1alpha1/0000_02_addon.open-cluster-management.io_addondeploymentconfigs.crd.yaml(3 hunks)addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yaml(4 hunks)addon/v1alpha1/types_addondeploymentconfig.go(1 hunks)addon/v1alpha1/types_addontemplate.go(1 hunks)cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml(6 hunks)cluster/v1/types.go(3 hunks)cluster/v1alpha1/0000_02_clusters.open-cluster-management.io_clusterclaims.crd.yaml(1 hunks)cluster/v1alpha1/0000_05_clusters.open-cluster-management.io_addonplacementscores.crd.yaml(2 hunks)cluster/v1alpha1/types.go(1 hunks)cluster/v1alpha1/types_addonplacementscore.go(2 hunks)cluster/v1alpha1/types_rolloutstrategy.go(3 hunks)cluster/v1beta1/0000_02_clusters.open-cluster-management.io_placements.crd.yaml(17 hunks)cluster/v1beta1/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yaml(2 hunks)cluster/v1beta1/types_placement.go(9 hunks)cluster/v1beta1/types_placementdecision.go(2 hunks)cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml(2 hunks)cluster/v1beta2/0000_01_clusters.open-cluster-management.io_managedclustersetbindings.crd.yaml(1 hunks)cluster/v1beta2/types_managedclusterset.go(2 hunks)cluster/v1beta2/types_managedclustersetbinding.go(1 hunks)operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml(2 hunks)operator/v1/types_klusterlet.go(2 hunks)work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml(3 hunks)work/v1/types.go(2 hunks)work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml(10 hunks)work/v1alpha1/types_manifestworkreplicaset.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- addon/v1alpha1/0000_02_addon.open-cluster-management.io_addondeploymentconfigs.crd.yaml
- work/v1alpha1/types_manifestworkreplicaset.go
- addon/v1alpha1/types_addondeploymentconfig.go
- work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (21)
- cluster/v1alpha1/0000_02_clusters.open-cluster-management.io_clusterclaims.crd.yaml
- cluster/v1alpha1/types.go
- addon/v1alpha1/0000_00_addon.open-cluster-management.io_clustermanagementaddons.crd.yaml
- addon/v1alpha1/types_addontemplate.go
- cluster/v1beta2/0000_01_clusters.open-cluster-management.io_managedclustersetbindings.crd.yaml
- cluster/v1beta2/types_managedclusterset.go
- cluster/v1alpha1/types_rolloutstrategy.go
- cluster/v1/types.go
- cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml
- addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yaml
- work/v1/types.go
- work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml
- operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml
- cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml
- cluster/v1alpha1/types_addonplacementscore.go
- cluster/v1beta1/types_placement.go
- cluster/v1beta1/types_placementdecision.go
- operator/v1/types_klusterlet.go
- cluster/v1beta2/types_managedclustersetbinding.go
- cluster/v1alpha1/0000_05_clusters.open-cluster-management.io_addonplacementscores.crd.yaml
- cluster/v1beta1/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yaml
⏰ 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
|
LGTM |
|
/lgtm |
openshift-merge-bot
bot
merged commit 3b7c6be
into
open-cluster-management-io:main
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Documentation