From 90da9668e485fdc24331dec3b7cc6325305e5001 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 14 Jun 2021 14:15:27 +1200 Subject: [PATCH] Allow resources to have additional properties (#1558) * Add Property() to PropertyContainer * Implement Property() on ResourceType for Spec and Status * Implement WithProperty() for ResourceType * Implement WithoutProperty() and test * Prevent modification of Spec and Status properties * go fmt * Fix bug in Properties() --- .../pkg/astmodel/property_container.go | 3 + hack/generator/pkg/astmodel/resource_type.go | 77 +++++++++++-- .../pkg/astmodel/resource_type_test.go | 105 +++++++++++++++++- 3 files changed, 172 insertions(+), 13 deletions(-) diff --git a/hack/generator/pkg/astmodel/property_container.go b/hack/generator/pkg/astmodel/property_container.go index 6c9da7079af..beb54226967 100644 --- a/hack/generator/pkg/astmodel/property_container.go +++ b/hack/generator/pkg/astmodel/property_container.go @@ -11,4 +11,7 @@ type PropertyContainer interface { // Properties returns all the properties from this container // A sorted slice is returned to preserve immutability and provide determinism Properties() []*PropertyDefinition + + // Property returns the property and true if the named property is found, nil and false otherwise + Property(name PropertyName) (*PropertyDefinition, bool) } diff --git a/hack/generator/pkg/astmodel/resource_type.go b/hack/generator/pkg/astmodel/resource_type.go index 76a32186388..7cdfcad9c10 100644 --- a/hack/generator/pkg/astmodel/resource_type.go +++ b/hack/generator/pkg/astmodel/resource_type.go @@ -24,6 +24,7 @@ type ResourceType struct { status Type isStorageVersion bool owner *TypeName + properties map[PropertyName]*PropertyDefinition functions map[string]Function testcases map[string]TestCase InterfaceImplementer @@ -34,6 +35,7 @@ func NewResourceType(specType Type, statusType Type) *ResourceType { result := &ResourceType{ isStorageVersion: false, owner: nil, + properties: make(map[PropertyName]*PropertyDefinition), functions: make(map[string]Function), testcases: make(map[string]TestCase), InterfaceImplementer: MakeInterfaceImplementer(), @@ -286,26 +288,82 @@ func (resource *ResourceType) EmbeddedProperties() []*PropertyDefinition { } } +// WithProperty creates a new ResourceType with another property attached to it +// Properties are unique by name, so this can be used to Add and Replace a property +func (resource *ResourceType) WithProperty(property *PropertyDefinition) *ResourceType { + if property.HasName("Status") || property.HasName("Spec") { + panic(fmt.Sprintf("May not modify property %q on a resource", property.PropertyName())) + } + + // Create a copy to preserve immutability + result := resource.copy() + result.properties[property.propertyName] = property + + return result +} + +// WithoutProperty creates a new ResourceType that's a copy without the specified property +func (resource *ResourceType) WithoutProperty(name PropertyName) *ResourceType { + if name == "Status" || name == "Spec" { + panic(fmt.Sprintf("May not remove property %q from a resource", name)) + } + + // Create a copy to preserve immutability + result := resource.copy() + delete(result.properties, name) + + return result +} + // Properties returns all the properties from this resource type // An ordered slice is returned to preserve immutability and provide determinism func (resource *ResourceType) Properties() []*PropertyDefinition { - specProperty := NewPropertyDefinition("Spec", "spec", resource.spec). - WithTag("json", "omitempty") - result := []*PropertyDefinition{ - specProperty, + resource.createSpecProperty(), } if resource.status != nil { - statusProperty := NewPropertyDefinition("Status", "status", resource.status). - WithTag("json", "omitempty") - result = append(result, statusProperty) + result = append(result, resource.createStatusProperty()) } + for _, property := range resource.properties { + result = append(result, property) + } + + // Sorted so that it's always consistent + sort.Slice(result, func(left int, right int) bool { + return result[left].propertyName < result[right].propertyName + }) + return result } +func (resource *ResourceType) createStatusProperty() *PropertyDefinition { + statusProperty := NewPropertyDefinition("Status", "status", resource.status). + WithTag("json", "omitempty") + return statusProperty +} + +func (resource *ResourceType) createSpecProperty() *PropertyDefinition { + return NewPropertyDefinition("Spec", "spec", resource.spec). + WithTag("json", "omitempty") +} + +// Property returns the property and true if the named property is found, nil and false otherwise +func (resource *ResourceType) Property(name PropertyName) (*PropertyDefinition, bool) { + if name == "Spec" { + return resource.createSpecProperty(), true + } + + if name == "Status" { + return resource.createStatusProperty(), true + } + + prop, ok := resource.properties[name] + return prop, ok +} + // Functions returns all the function implementations // A sorted slice is returned to preserve immutability and provide determinism func (resource *ResourceType) Functions() []Function { @@ -541,11 +599,16 @@ func (resource *ResourceType) copy() *ResourceType { status: resource.status, isStorageVersion: resource.isStorageVersion, owner: resource.owner, + properties: make(map[PropertyName]*PropertyDefinition), functions: make(map[string]Function), testcases: make(map[string]TestCase), InterfaceImplementer: resource.InterfaceImplementer.copy(), } + for key, property := range resource.properties { + result.properties[key] = property + } + for key, testcase := range resource.testcases { result.testcases[key] = testcase } diff --git a/hack/generator/pkg/astmodel/resource_type_test.go b/hack/generator/pkg/astmodel/resource_type_test.go index 5b6bce02c39..1be05e3525b 100644 --- a/hack/generator/pkg/astmodel/resource_type_test.go +++ b/hack/generator/pkg/astmodel/resource_type_test.go @@ -13,6 +13,11 @@ import ( //TODO (theunrepentantgeek): MOAR TESTS +var ( + emptySpec = NewObjectType() + emptyStatus = NewObjectType() +) + /* * WithTestCase() tests */ @@ -20,11 +25,9 @@ import ( func TestResourceType_WithTestCase_ReturnsExpectedInstance(t *testing.T) { g := NewGomegaWithT(t) - spec := NewObjectType() - status := NewObjectType() name := "assertStuff" fake := NewFakeTestCase(name) - base := NewResourceType(spec, status) + base := NewResourceType(emptySpec, emptyStatus) resource := base.WithTestCase(fake) @@ -40,12 +43,10 @@ func TestResourceType_WithTestCase_ReturnsExpectedInstance(t *testing.T) { func TestResourceType_WithFunction_ReturnsExpectedInstance(t *testing.T) { g := NewGomegaWithT(t) - spec := NewObjectType() - status := NewObjectType() name := "assertStuff" fake := NewFakeFunction(name) - base := NewResourceType(spec, status) + base := NewResourceType(emptySpec, emptyStatus) resource := base.WithFunction(fake) @@ -54,3 +55,95 @@ func TestResourceType_WithFunction_ReturnsExpectedInstance(t *testing.T) { g.Expect(resource.functions[name]).To(Equal(fake)) } + +/* + * Properties() tests + */ + +func TestResourceType_Properties_ReturnsExpectedCount(t *testing.T) { + g := NewGomegaWithT(t) + base := NewResourceType(emptySpec, emptyStatus) + g.Expect(base.Properties()).To(HaveLen(2)) +} + +/* + * Property() tests + */ + +func TestResourceType_Property_ForStatus_ReturnsProperty(t *testing.T) { + g := NewGomegaWithT(t) + base := NewResourceType(emptySpec, emptyStatus) + prop, ok := base.Property("Spec") + g.Expect(ok).To(BeTrue()) + g.Expect(prop).NotTo(BeNil()) +} + +func TestResourceType_Property_ForSpec_ReturnsProperty(t *testing.T) { + g := NewGomegaWithT(t) + base := NewResourceType(emptySpec, emptyStatus) + prop, ok := base.Property("Spec") + g.Expect(ok).To(BeTrue()) + g.Expect(prop).NotTo(BeNil()) +} + +/* + * WithProperty() tests + */ + +func TestResourceType_WithProperty_HasExpectedLength(t *testing.T) { + g := NewGomegaWithT(t) + base := NewResourceType(emptySpec, emptyStatus) + ownerProp := NewPropertyDefinition("Owner", "owner", StringType) + resource := base.WithProperty(ownerProp) + g.Expect(resource.Properties()).To(HaveLen(3)) +} + +func TestResourceType_WithProperty_IncludesProperty(t *testing.T) { + g := NewGomegaWithT(t) + base := NewResourceType(emptySpec, emptyStatus) + ownerProp := NewPropertyDefinition("Owner", "owner", StringType) + resource := base.WithProperty(ownerProp) + prop, ok := resource.Property("Owner") + g.Expect(ok).To(BeTrue()) + g.Expect(prop).NotTo(BeNil()) +} + +func TestResourceType_WithProperty_OverridingSpec_Panics(t *testing.T) { + g := NewGomegaWithT(t) + base := NewResourceType(emptySpec, emptyStatus) + specProp := NewPropertyDefinition("Spec", "spec", StringType) + g.Expect(func() { base.WithProperty(specProp) }).To(Panic()) +} + +func TestResourceType_WithProperty_OverridingStatus_Panics(t *testing.T) { + g := NewGomegaWithT(t) + base := NewResourceType(emptySpec, emptyStatus) + statusProp := NewPropertyDefinition("Status", "status", StringType) + g.Expect(func() { base.WithProperty(statusProp) }).To(Panic()) +} + +/* + * WithoutProperty() tests + */ + +func TestResourceType_WithoutProperty_ExcludesProperty(t *testing.T) { + g := NewGomegaWithT(t) + ownerProp := NewPropertyDefinition("Owner", "owner", StringType) + base := NewResourceType(emptySpec, emptyStatus).WithProperty(ownerProp) + resource := base.WithoutProperty("Owner") + prop, ok := resource.Property("Owner") + g.Expect(ok).To(BeFalse()) + g.Expect(prop).To(BeNil()) +} + +func TestResourceType_WithoutSpecProperty_Panics(t *testing.T) { + g := NewGomegaWithT(t) + base := NewResourceType(emptySpec, emptyStatus) + g.Expect(func() { base.WithoutProperty("Spec") }).To(Panic()) +} + +func TestResourceType_WithoutStatusProperty_Panics(t *testing.T) { + g := NewGomegaWithT(t) + base := NewResourceType(emptySpec, emptyStatus) + g.Expect(func() { base.WithoutProperty("Status") }).To(Panic()) +}