Add cluster reference to ClusterOrder.status - #8
Conversation
Add kubebuilder validation markers [1] to ClusterOrder type definitions. [1]: https://book.kubebuilder.io/reference/markers/crd-validation.html
WalkthroughThe changes modify the Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@mhrivnak good catch; I've also marked |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/v1alpha1/zz_generated.deepcopy.go (1)
1-139: Reminder: This is an auto-generated fileThis file is auto-generated by controller-gen as indicated by the comment on line 19. The changes appear to be correctly generated based on the addition of the
ClusterReferencefield toClusterOrderStatus. In normal circumstances, this file should not be manually edited.If any modifications are needed to the deep copy behavior, they should be made in the source files defining the types (like
clusterorder_types.go), and then the deep copy code should be regenerated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/v1alpha1/clusterorder_types.go(2 hunks)api/v1alpha1/zz_generated.deepcopy.go(2 hunks)config/crd/bases/cloudkit.openshift.io_clusterorders.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- config/crd/bases/cloudkit.openshift.io_clusterorders.yaml
- api/v1alpha1/clusterorder_types.go
🧰 Additional context used
🧬 Code Definitions (1)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/clusterorder_types.go (1)
ClusterOrderClusterReferenceType(74-79)
🔇 Additional comments (2)
api/v1alpha1/zz_generated.deepcopy.go (2)
55-68: Looks good: DeepCopy methods for ClusterOrderClusterReferenceTypeThe newly added methods for deep copying the
ClusterOrderClusterReferenceTypeare consistent with the standard auto-generated pattern. Since this struct contains only string fields (as seen in the relevant code snippet), the simple copy implementation is sufficient.
127-127: Correctly handles new ClusterReference fieldThe update to
ClusterOrderStatus.DeepCopyIntocorrectly adds the handling of the newClusterReferencefield, ensuring proper deep copying of the entire status object.
Add a `clusterReference` field to ClusterOrder.status that records the namespace, cluster name, and service account name created by the controller in response to a ClusterOrder.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/v1alpha1/clusterorder_types.go (1)
73-79: Structured cluster reference typeThe new
ClusterOrderClusterReferenceTypestruct provides a clean, structured way to reference the created cluster resources.One potential improvement would be to add some kubebuilder validation annotations to ensure these fields are required when the ClusterReference is present.
type ClusterOrderClusterReferenceType struct { // Namespace that contains the HostedCluster resource + // +kubebuilder:validation:Required Namespace string `json:"namespace"` + // +kubebuilder:validation:Required HostedClusterName string `json:"hostedClusterName"` + // +kubebuilder:validation:Required ServiceAccountName string `json:"serviceAccountName"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/v1alpha1/clusterorder_types.go(2 hunks)api/v1alpha1/zz_generated.deepcopy.go(2 hunks)config/crd/bases/cloudkit.openshift.io_clusterorders.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/crd/bases/cloudkit.openshift.io_clusterorders.yaml
🧰 Additional context used
🧬 Code Definitions (1)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/clusterorder_types.go (1)
ClusterOrderClusterReferenceType(74-79)
🔇 Additional comments (6)
api/v1alpha1/zz_generated.deepcopy.go (2)
55-68: The new deep copy functions for ClusterOrderClusterReferenceType look goodThe implementation correctly handles deep copying for the new
ClusterOrderClusterReferenceTypestruct. Since this struct only contains primitive string types, the simple*out = *inassignment inDeepCopyIntois sufficient.
127-131: Proper handling of the new ClusterReference fieldThe conditional update to
ClusterOrderStatus.DeepCopyIntocorrectly handles the new optionalClusterReferencefield by creating a new instance when the source field is non-nil and copying its contents.api/v1alpha1/clusterorder_types.go (4)
30-33: Good validation for TemplateIDThe added validation annotations for the
TemplateIDfield are appropriate:
- Making it required ensures every ClusterOrder has a template
- The pattern validation enforces a valid naming convention (must start with lowercase letter, contain only lowercase letters, numbers, and hyphens, and end with a lowercase letter or number)
84-87: Appropriate enum validation for PhaseThe validation annotations for the
Phasefield properly restrict it to the defined enum values, which matches the constants defined above. Marking it as optional aligns with Kubernetes API conventions for status fields.
90-91: Optional conditions field follows K8s conventionsMaking the
Conditionsfield optional aligns with Kubernetes API conventions, as mentioned in the PR comments. This matches how condition fields are handled in other Kubernetes resources like Pods.
93-96: ClusterReference implementation follows best practicesThe new
ClusterReferencefield:
- Is properly marked as optional with both the annotation and pointer type
- Has a clear comment explaining its purpose
- Uses a pointer type which follows K8s conventions for optional fields
This implementation satisfies the PR objective of tracking the namespace, cluster name, and service account created by the controller.
2 participants
Add a
clusterReferencefield to clusterOrder.status that records the namespace, cluster name, and service account name created by the controller in response to a ClusterOrder.Summary by CodeRabbit
ClusterOrder.