k8s API for healtheventwithstatus model #640
Conversation
Signed-off-by: Avinash Yeddula <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
data-models/Makefile
Outdated
| include ../make/go.mk | ||
|
|
||
| # Version of controller-gen to use for generating CRD deepcopy, client, etc. | ||
| CONTROLLER_GEN_VERSION := v0.17.2 |
There was a problem hiding this comment.
v0.20.0 is latest, if we're going to pin we should pickup the newest one
There was a problem hiding this comment.
also, janitor has a controller-gen as well. It's using latest rather than a pinned version though. Checkout how the main makfile in the root dir is handling versions:
Line 24 in 82e7180
We should probably put controller-gen there with that same pattern since we're using it in multiple modules now
|
|
||
| var ( | ||
| // GroupVersion is group version used to register these objects | ||
| GroupVersion = schema.GroupVersion{Group: "data-models.dgxc.nvidia.com", Version: "v1alpha1"} |
There was a problem hiding this comment.
I was thinking maybe we do healthevents.dgxc.nvidia.com but open to ideas here too. Just because health-events is a little more descriptive than data-models
| ) | ||
|
|
||
| // HealthEventSpec defines the desired state of HealthStatus | ||
| type HealthEventSpec struct { |
There was a problem hiding this comment.
We don't want to manage two copies of the same struct just to make it a kubernetes type.
Can we point directly to the objects that already exist? So it would look something like this
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
type HealthEvent struct {
Spec HealthEvent // this is the existing health event object
Status HealthEventStatus // this is the existing status object
}
Signed-off-by: Avinash Yeddula <[email protected]>
yavinash007
force-pushed
the
healtheventwithstatus
branch
from
aa400bd to
14aa429
Compare
| EventID string `json:"eventID"` | ||
|
|
||
| // Node associated with this health event | ||
| // +kubebuilder:validation:Required |
There was a problem hiding this comment.
This still looks like we're redefining the types, we ideally don't want to maintain two copies of the same types and keep them in sync
| Message string `bson:"message,omitempty" json:"message,omitempty"` | ||
| } | ||
|
|
||
| type HealthEventStatus struct { |
There was a problem hiding this comment.
I was thinking we could just stuff this exact object to be the status for example
|
|
||
| // +kubebuilder:object:root=true | ||
| // +kubebuilder:subresource:status | ||
| type HealthStatus struct { |
There was a problem hiding this comment.
so my thought was this struct would look like this:
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
type HealthStatus struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec model.HealthEvent `json:"spec,omitempty"`
Status model.HealthEventStatus `json:"status,omitempty"`
}
So you would not define your own spec and status objects we would just use the existing ones.
There's some implications for api-versioning going forward if we want to adhere to best practices that we should discuss with the NVIDIA folks if we do it this way but it seems like a clean way to share the object that the other datasources use.
Signed-off-by: Avinash Yeddula <[email protected]>
3 participants
Summary
Type of Change
Component(s) Affected
Testing
Checklist