-
Notifications
You must be signed in to change notification settings - Fork 21
HYPERFLEET-1084 - feat: add generic resource data layer with entity registry #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| package api | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "gorm.io/datatypes" | ||
| "gorm.io/gorm" | ||
|
|
||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" | ||
| ) | ||
|
|
||
| // Resource is the generic GORM model for entity types managed by the entity | ||
| // registry (Channel, Version, WIF Config, etc.). Entity kinds are | ||
| // differentiated by the Kind field. Existing Cluster and NodePool types | ||
| // are NOT migrated to this model. | ||
| type Resource struct { | ||
| Meta | ||
| Kind string `json:"kind" gorm:"size:100;not null"` | ||
| Name string `json:"name" gorm:"size:100;not null"` | ||
| Href string `json:"href,omitempty" gorm:"size:500"` | ||
| CreatedBy string `json:"created_by" gorm:"size:255;not null"` | ||
| UpdatedBy string `json:"updated_by" gorm:"size:255;not null"` | ||
| DeletedBy *string `json:"deleted_by,omitempty" gorm:"size:255"` | ||
| DeletedTime *time.Time `json:"deleted_time,omitempty"` | ||
| OwnerID *string `json:"owner_id,omitempty" gorm:"size:255"` | ||
| OwnerKind *string `json:"owner_kind,omitempty" gorm:"size:100"` | ||
| OwnerHref *string `json:"owner_href,omitempty" gorm:"size:500"` | ||
| Spec datatypes.JSON `json:"spec" gorm:"type:jsonb;not null"` | ||
| Labels datatypes.JSON `json:"labels,omitempty" gorm:"type:jsonb"` | ||
| Generation int32 `json:"generation" gorm:"default:1;not null"` | ||
| } | ||
|
|
||
| type ( | ||
| ResourceList []*Resource | ||
| ResourceIndex map[string]*Resource | ||
| ) | ||
|
|
||
| func (l ResourceList) Index() ResourceIndex { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dead code? Appears only in the test itself 🙂 |
||
| index := ResourceIndex{} | ||
| for _, o := range l { | ||
| index[o.ID] = o | ||
| } | ||
| return index | ||
| } | ||
|
|
||
| func (r Resource) TableName() string { | ||
| return "resources" | ||
| } | ||
|
|
||
| // BeforeCreate TODO: Validate the necessity for this as part of https://redhat.atlassian.net/browse/HYPERFLEET-1085 | ||
| func (r *Resource) BeforeCreate(tx *gorm.DB) error { | ||
| if r.ID == "" { | ||
| id, err := NewID() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate resource ID: %w", err) | ||
| } | ||
| r.ID = id | ||
| } | ||
|
|
||
| now := time.Now() | ||
| if r.CreatedTime.IsZero() { | ||
| r.CreatedTime = now | ||
| } | ||
| r.UpdatedTime = now | ||
| if r.Generation == 0 { | ||
| r.Generation = 1 | ||
| } | ||
|
|
||
| if r.Href == "" { | ||
| desc := registry.MustGet(r.Kind) | ||
| if r.OwnerID != nil && *r.OwnerID != "" { | ||
| if r.OwnerKind == nil || *r.OwnerKind == "" { | ||
| return fmt.Errorf("owner_kind is required when owner_id is set") | ||
| } | ||
| if r.OwnerHref == nil { | ||
| parentDesc := registry.MustGet(*r.OwnerKind) | ||
| ownerHref := fmt.Sprintf("/api/hyperfleet/v1/%s/%s", | ||
| parentDesc.Plural, *r.OwnerID) | ||
| r.OwnerHref = &ownerHref | ||
| } | ||
| r.Href = fmt.Sprintf("%s/%s/%s", *r.OwnerHref, desc.Plural, r.ID) | ||
| } else { | ||
| r.Href = fmt.Sprintf("/api/hyperfleet/v1/%s/%s", desc.Plural, r.ID) | ||
|
Comment on lines
+72
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normalize owner fields and treat empty On Line 76 and Line 82, an explicitly empty Suggested fix import (
"fmt"
+ "strings"
"time"
@@
- if r.OwnerID != nil && *r.OwnerID != "" {
+ if r.OwnerID != nil && strings.TrimSpace(*r.OwnerID) != "" {
if r.OwnerKind == nil || *r.OwnerKind == "" {
return fmt.Errorf("owner_kind is required when owner_id is set")
}
- if r.OwnerHref == nil {
+ if r.OwnerHref == nil || strings.TrimSpace(*r.OwnerHref) == "" {
parentDesc := registry.MustGet(*r.OwnerKind)
ownerHref := fmt.Sprintf("/api/hyperfleet/v1/%s/%s",
parentDesc.Plural, *r.OwnerID)
r.OwnerHref = &ownerHref
}
r.Href = fmt.Sprintf("%s/%s/%s", *r.OwnerHref, desc.Plural, r.ID)
} else {
+ r.OwnerID = nil
+ r.OwnerKind = nil
+ r.OwnerHref = nil
r.Href = fmt.Sprintf("/api/hyperfleet/v1/%s/%s", desc.Plural, r.ID)
}As per coding guidelines "Validate changes against HyperFleet architecture standards from the linked architecture repository." 🤖 Prompt for AI Agents |
||
| } | ||
|
tirthct marked this conversation as resolved.
|
||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (r *Resource) BeforeUpdate(tx *gorm.DB) error { | ||
| r.UpdatedTime = time.Now() | ||
| return nil | ||
| } | ||
|
|
||
| func (r *Resource) MarkDeleted(by string, t time.Time) { | ||
| r.DeletedTime = &t | ||
| r.DeletedBy = &by | ||
| } | ||
|
|
||
| func (r *Resource) IncrementGeneration() { | ||
| r.Generation++ | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,258 @@ | ||
| package api | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| . "github.com/onsi/gomega" | ||
|
|
||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" | ||
| ) | ||
|
|
||
| func setupTestRegistry() { | ||
| registry.Reset() | ||
| registry.Register(registry.EntityDescriptor{ | ||
| Kind: "Channel", | ||
| Plural: "channels", | ||
| }) | ||
| registry.Register(registry.EntityDescriptor{ | ||
| Kind: "Version", | ||
| Plural: "versions", | ||
| ParentKind: "Channel", | ||
| }) | ||
| } | ||
|
|
||
| func strPtr(s string) *string { | ||
| return &s | ||
| } | ||
|
|
||
| func TestResourceList_Index(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| emptyList := ResourceList{} | ||
| emptyIndex := emptyList.Index() | ||
| Expect(len(emptyIndex)).To(Equal(0)) | ||
|
|
||
| r1 := &Resource{} | ||
| r1.ID = "res-1" | ||
| r1.Name = "test-resource-1" | ||
|
|
||
| r2 := &Resource{} | ||
| r2.ID = "res-2" | ||
| r2.Name = "test-resource-2" | ||
|
|
||
| multiList := ResourceList{r1, r2} | ||
| multiIndex := multiList.Index() | ||
| Expect(len(multiIndex)).To(Equal(2)) | ||
| Expect(multiIndex["res-1"]).To(Equal(r1)) | ||
| Expect(multiIndex["res-2"]).To(Equal(r2)) | ||
|
|
||
| r1Dup := &Resource{} | ||
| r1Dup.ID = "res-1" | ||
| r1Dup.Name = "duplicate" | ||
|
|
||
| dupList := ResourceList{r1, r1Dup} | ||
| dupIndex := dupList.Index() | ||
| Expect(len(dupIndex)).To(Equal(1)) | ||
| Expect(dupIndex["res-1"].Name).To(Equal("duplicate")) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_IDGeneration(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{Name: "test", Kind: "Channel"} | ||
|
|
||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.ID).ToNot(BeEmpty()) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_IDPreservation(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{Name: "test", Kind: "Channel"} | ||
| r.ID = "pre-set-id" | ||
|
|
||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.ID).To(Equal("pre-set-id")) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_GenerationDefault(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{Name: "test", Kind: "Channel"} | ||
|
|
||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.Generation).To(Equal(int32(1))) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_GenerationPreserved(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{Name: "test", Kind: "Channel", Generation: 5} | ||
|
|
||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.Generation).To(Equal(int32(5))) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_Timestamps(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| before := time.Now() | ||
| r := &Resource{Name: "test", Kind: "Channel"} | ||
|
|
||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
|
|
||
| Expect(r.CreatedTime).ToNot(BeZero()) | ||
| Expect(r.UpdatedTime).ToNot(BeZero()) | ||
| Expect(r.CreatedTime.After(before) || r.CreatedTime.Equal(before)).To(BeTrue()) | ||
| Expect(r.UpdatedTime.After(before) || r.UpdatedTime.Equal(before)).To(BeTrue()) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_CreatedTimePreserved(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| fixedTime := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC) | ||
| r := &Resource{Name: "test", Kind: "Channel"} | ||
| r.CreatedTime = fixedTime | ||
|
|
||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.CreatedTime).To(Equal(fixedTime)) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_HrefTopLevel(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{Name: "stable", Kind: "Channel"} | ||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.Href).To(Equal("/api/hyperfleet/v1/channels/" + r.ID)) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_HrefChild(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{ | ||
| Name: "4-17-12", | ||
| Kind: "Version", | ||
| OwnerID: strPtr("ch-1"), | ||
| OwnerKind: strPtr("Channel"), | ||
| } | ||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.Href).To(Equal("/api/hyperfleet/v1/channels/ch-1/versions/" + r.ID)) | ||
| Expect(*r.OwnerHref).To(Equal("/api/hyperfleet/v1/channels/ch-1")) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_OwnerKindMissing(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{ | ||
| Name: "4-17-12", | ||
| Kind: "Version", | ||
| OwnerID: strPtr("ch-1"), | ||
| } | ||
| err := r.BeforeCreate(nil) | ||
| Expect(err).ToNot(BeNil()) | ||
| Expect(err.Error()).To(ContainSubstring("owner_kind is required")) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_OwnerKindEmpty(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{ | ||
| Name: "4-17-12", | ||
| Kind: "Version", | ||
| OwnerID: strPtr("ch-1"), | ||
| OwnerKind: strPtr(""), | ||
| } | ||
| err := r.BeforeCreate(nil) | ||
| Expect(err).ToNot(BeNil()) | ||
| Expect(err.Error()).To(ContainSubstring("owner_kind is required")) | ||
| } | ||
|
|
||
| func TestResource_BeforeCreate_HrefChildWithPresetOwnerHref(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{ | ||
| Name: "some-label", | ||
| Kind: "Version", | ||
| OwnerID: strPtr("v-1"), | ||
| OwnerKind: strPtr("Version"), | ||
| OwnerHref: strPtr("/api/hyperfleet/v1/channels/ch-1/versions/v-1"), | ||
| } | ||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.Href).To(Equal("/api/hyperfleet/v1/channels/ch-1/versions/v-1/versions/" + r.ID)) | ||
| Expect(*r.OwnerHref).To(Equal("/api/hyperfleet/v1/channels/ch-1/versions/v-1")) | ||
| } | ||
|
Comment on lines
+189
to
+204
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Line 197 sets As per coding guidelines "Validate changes against HyperFleet architecture standards from the linked architecture repository." 🤖 Prompt for AI Agents |
||
|
|
||
| func TestResource_BeforeCreate_HrefPreserved(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestRegistry() | ||
|
|
||
| r := &Resource{Name: "test", Kind: "Channel", Href: "/custom/href"} | ||
| err := r.BeforeCreate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.Href).To(Equal("/custom/href")) | ||
| } | ||
|
Comment on lines
+60
to
+214
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift Refactor These cases are currently split into many implementation-named tests, which makes maintenance harder and drifts from test conventions for multi-scenario behavior. As per coding guidelines "Test names describe the scenario, not the implementation" and "Table-driven tests used for multiple cases". 🤖 Prompt for AI Agents |
||
|
|
||
| func TestResource_BeforeUpdate_UpdatesTimestamp(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| r := &Resource{Name: "test", Kind: "Channel"} | ||
| r.UpdatedTime = time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC) | ||
|
|
||
| before := time.Now() | ||
| err := r.BeforeUpdate(nil) | ||
| Expect(err).To(BeNil()) | ||
| Expect(r.UpdatedTime.After(before) || r.UpdatedTime.Equal(before)).To(BeTrue()) | ||
| } | ||
|
|
||
| func TestResource_MarkDeleted(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| r := &Resource{Name: "test", Kind: "Channel"} | ||
| now := time.Now() | ||
|
|
||
| r.MarkDeleted("admin", now) | ||
|
|
||
| Expect(r.DeletedTime).ToNot(BeNil()) | ||
| Expect(*r.DeletedTime).To(Equal(now)) | ||
| Expect(r.DeletedBy).ToNot(BeNil()) | ||
| Expect(*r.DeletedBy).To(Equal("admin")) | ||
| } | ||
|
|
||
| func TestResource_IncrementGeneration(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| r := &Resource{Name: "test", Kind: "Channel", Generation: 1} | ||
| r.IncrementGeneration() | ||
| Expect(r.Generation).To(Equal(int32(2))) | ||
|
|
||
| r.IncrementGeneration() | ||
| Expect(r.Generation).To(Equal(int32(3))) | ||
| } | ||
|
|
||
| func TestResource_TableName(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| r := Resource{} | ||
| Expect(r.TableName()).To(Equal("resources")) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be string
https://github.com/rh-amarin/hyperfleet-architecture/blob/a587810415cb18b896f450a2b1890dddad75a529/hyperfleet/docs/generic-resource-registry-design.md#41-gorm-types-pkgapiresourcego