Skip to content
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

Allow resources to have additional properties #1558

Merged
merged 7 commits into from
Jun 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions hack/generator/pkg/astmodel/property_container.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
77 changes: 70 additions & 7 deletions hack/generator/pkg/astmodel/resource_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type ResourceType struct {
status Type
isStorageVersion bool
owner *TypeName
properties map[PropertyName]*PropertyDefinition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theunrepentantgeek - Sorry I missed looking at this PR on my Friday, but why do we need this change for the storage versions? The property doesn't have to be on the top level resource type I don't think. It really should be in spec in the storage version too, shouldn't it?

Maybe we can sync up offline or in the meeting today?

functions map[string]Function
testcases map[string]TestCase
InterfaceImplementer
Expand All @@ -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(),
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't Properties() return the non-spec/status properties too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thus we prove that I need to write the test even for code I think I can't possibly get wrong! 😊

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}

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 {
Expand Down Expand Up @@ -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
}
Expand Down
105 changes: 99 additions & 6 deletions hack/generator/pkg/astmodel/resource_type_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,21 @@ import (

//TODO (theunrepentantgeek): MOAR TESTS

var (
emptySpec = NewObjectType()
emptyStatus = NewObjectType()
)

/*
* WithTestCase() tests
*/

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)

Expand All @@ -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)

Expand All @@ -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())
}