Skip to content

Commit

Permalink
Cleanups and todos on Topology tests and builders
Browse files Browse the repository at this point in the history 
Signed-off-by: killianmuldoon <[email protected]>
  • Loading branch information
@killianmuldoon
killianmuldoon committed Oct 22, 2021
1 parent 1e7b195 commit e14d96f
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 70 deletions.
2 changes: 0 additions & 2 deletions controllers/topology/cluster_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ func TestClusterReconciler_reconcile(t *testing.T) {
// 1) Templates for Machine, Cluster, ControlPlane and Bootstrap.
infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(ns.Name, "inframachinetemplate").Build()
infrastructureClusterTemplate := builder.InfrastructureClusterTemplate(ns.Name, "infraclustertemplate").
// Create spec fake setting to assert that template spec is non-empty for tests.
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
controlPlaneTemplate := builder.ControlPlaneTemplate(ns.Name, "cp1").
WithInfrastructureMachineTemplate(infrastructureMachineTemplate).
Expand Down
2 changes: 0 additions & 2 deletions controllers/topology/current_state_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,10 @@ func TestGetCurrentState(t *testing.T) {

// InfrastructureCluster objects.
infraCluster := builder.InfrastructureCluster(metav1.NamespaceDefault, "infraOne").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()

// ControlPlane and ControlPlaneInfrastructureMachineTemplate objects.
controlPlaneInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfraTemplate").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
controlPlaneTemplateWithInfrastructureMachine := builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cpTemplateWithInfra1").
WithInfrastructureMachineTemplate(controlPlaneInfrastructureMachineTemplate).
Expand Down
4 changes: 0 additions & 4 deletions controllers/topology/desired_state_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ var (
func TestComputeInfrastructureCluster(t *testing.T) {
// templates and ClusterClass
infrastructureClusterTemplate := builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "template1").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
clusterClass := builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithInfrastructureClusterTemplate(infrastructureClusterTemplate).
Expand Down Expand Up @@ -143,7 +142,6 @@ func TestComputeControlPlaneInfrastructureMachineTemplate(t *testing.T) {
}

infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "template1").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
clusterClass := builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithControlPlaneMetadata(labels, annotations).
Expand Down Expand Up @@ -215,7 +213,6 @@ func TestComputeControlPlane(t *testing.T) {
annotations := map[string]string{"a1": ""}

controlPlaneTemplate := builder.ControlPlaneTemplate(metav1.NamespaceDefault, "template1").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
clusterClass := builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithControlPlaneMetadata(labels, annotations).
Expand Down Expand Up @@ -666,7 +663,6 @@ func TestComputeCluster(t *testing.T) {

func TestComputeMachineDeployment(t *testing.T) {
workerInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "linux-worker-inframachinetemplate").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
workerBootstrapTemplate := builder.BootstrapTemplate(metav1.NamespaceDefault, "linux-worker-bootstraptemplate").
Build()
Expand Down
82 changes: 25 additions & 57 deletions controllers/topology/reconcile_state_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ func TestReconcileCluster(t *testing.T) {

s := scope.New(tt.current)

// TODO: stop setting ResourceVersion when building objects
tt.desired.SetResourceVersion("")
s.Desired = &scope.ClusterState{Cluster: tt.desired}

r := ClusterReconciler{
Expand Down Expand Up @@ -188,8 +186,6 @@ func TestReconcileInfrastructureCluster(t *testing.T) {
s := scope.New(nil)
s.Current.InfrastructureCluster = tt.current

// TODO: stop setting ResourceVersion when building objects
tt.desired.SetResourceVersion("")
s.Desired = &scope.ClusterState{InfrastructureCluster: tt.desired}

r := ClusterReconciler{
Expand Down Expand Up @@ -266,47 +262,47 @@ func TestReconcileControlPlaneObject(t *testing.T) {
name: "Should create desired ControlPlane if the current does not exist",
class: ccWithoutControlPlaneInfrastructure,
current: nil,
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
{
name: "Fail on updating ControlPlaneObject with incompatible changes, here a different Kind for the infrastructureMachineTemplate",
class: ccWithoutControlPlaneInfrastructure,
current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane2, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane2.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: true,
},
{
name: "Update to ControlPlaneObject with no update to the underlying infrastructure",
class: ccWithoutControlPlaneInfrastructure,
current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: infrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
{
name: "Update to ControlPlaneObject with underlying infrastructure.",
class: ccWithControlPlaneInfrastructure,
current: &scope.ControlPlaneState{InfrastructureMachineTemplate: nil},
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
{
name: "Update to ControlPlaneObject with no underlying infrastructure",
class: ccWithoutControlPlaneInfrastructure,
current: &scope.ControlPlaneState{Object: controlPlane1},
desired: &scope.ControlPlaneState{Object: controlPlane3},
want: &scope.ControlPlaneState{Object: controlPlane3},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy()},
wantErr: false,
},
{
name: "Preserve specific changes to the ControlPlaneObject",
class: ccWithoutControlPlaneInfrastructure,
current: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
}
Expand Down Expand Up @@ -338,13 +334,6 @@ func TestReconcileControlPlaneObject(t *testing.T) {
WithObjects(fakeObjs...).
Build()

// TODO: stop setting ResourceVersion when building objects
if tt.desired.InfrastructureMachineTemplate != nil {
tt.desired.InfrastructureMachineTemplate.SetResourceVersion("")
}
if tt.desired.Object != nil {
tt.desired.Object.SetResourceVersion("")
}
r := ClusterReconciler{
Client: fakeClient,
}
Expand Down Expand Up @@ -393,10 +382,8 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) {

// Create InfrastructureMachineTemplates for test cases
infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
infrastructureMachineTemplate2 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra2").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()

// Create the blueprint mandating controlPlaneInfrastructure.
Expand Down Expand Up @@ -443,22 +430,22 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) {
}{
{
name: "Create desired InfrastructureMachineTemplate where it doesn't exist",
current: &scope.ControlPlaneState{Object: controlPlane1},
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
{
name: "Update desired InfrastructureMachineTemplate connected to controlPlane",
current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: updatedInfrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: updatedInfrastructureMachineTemplate},
wantErr: false,
},
{
name: "Fail on updating infrastructure with incompatible changes",
current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: incompatibleInfrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: incompatibleInfrastructureMachineTemplate},
wantErr: true,
},
}
Expand All @@ -481,13 +468,6 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) {
WithObjects(fakeObjs...).
Build()

// TODO: stop setting ResourceVersion when building objects
if tt.desired.InfrastructureMachineTemplate != nil {
tt.desired.InfrastructureMachineTemplate.SetResourceVersion("")
}
if tt.desired.Object != nil {
tt.desired.Object.SetResourceVersion("")
}
r := ClusterReconciler{
Client: fakeClient,
}
Expand Down Expand Up @@ -732,12 +712,6 @@ func TestReconcileMachineDeployments(t *testing.T) {
s := scope.New(builder.Cluster(metav1.NamespaceDefault, "cluster-1").Build())
s.Current.MachineDeployments = currentMachineDeploymentStates

// TODO: stop setting ResourceVersion when building objects
for _, md := range tt.desired {
md.Object.SetResourceVersion("")
md.BootstrapTemplate.SetResourceVersion("")
md.InfrastructureMachineTemplate.SetResourceVersion("")
}
s.Desired = &scope.ClusterState{MachineDeployments: toMachineDeploymentTopologyStateMap(tt.desired)}

r := ClusterReconciler{
Expand Down Expand Up @@ -779,11 +753,8 @@ func TestReconcileMachineDeployments(t *testing.T) {
}, &gotBootstrapTemplate)

g.Expect(err).ToNot(HaveOccurred())
// We don't want to compare resourceVersions as they are slightly different between the test cases
// and it's not worth the effort.
gotBootstrapTemplate.SetResourceVersion("")
wantMachineDeploymentState.BootstrapTemplate.SetResourceVersion("")
g.Expect(gotBootstrapTemplate).To(Equal(*wantMachineDeploymentState.BootstrapTemplate))

g.Expect(&gotBootstrapTemplate).To(EqualObject(wantMachineDeploymentState.BootstrapTemplate, IgnoreAutogeneratedMetadata))

// Check BootstrapTemplate rotation if there was a previous MachineDeployment/Template.
if currentMachineDeploymentState != nil && currentMachineDeploymentState.BootstrapTemplate != nil {
Expand All @@ -806,11 +777,8 @@ func TestReconcileMachineDeployments(t *testing.T) {
}, &gotInfrastructureMachineTemplate)

g.Expect(err).ToNot(HaveOccurred())
// We don't want to compare resourceVersions as they are slightly different between the test cases
// and it's not worth the effort.
gotInfrastructureMachineTemplate.SetResourceVersion("")
wantMachineDeploymentState.InfrastructureMachineTemplate.SetResourceVersion("")
g.Expect(gotInfrastructureMachineTemplate).To(Equal(*wantMachineDeploymentState.InfrastructureMachineTemplate))

g.Expect(&gotInfrastructureMachineTemplate).To(EqualObject(wantMachineDeploymentState.InfrastructureMachineTemplate, IgnoreAutogeneratedMetadata))

// Check InfrastructureMachineTemplate rotation if there was a previous MachineDeployment/Template.
if currentMachineDeploymentState != nil && currentMachineDeploymentState.InfrastructureMachineTemplate != nil {
Expand Down
49 changes: 44 additions & 5 deletions internal/builder/builders.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,14 @@ func InfrastructureMachineTemplate(namespace, name string) *InfrastructureMachin
}
}

// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object.
// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds
// to the value of the spec field.
//
// Note: all the paths should start with "spec."
//
// Example map: map[string]interface{}{
// "spec.version": "v1.2.3",
// }.
func (i *InfrastructureMachineTemplateBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureMachineTemplateBuilder {
i.specFields = fields
return i
Expand All @@ -359,6 +366,9 @@ func (i *InfrastructureMachineTemplateBuilder) Build() *unstructured.Unstructure
obj.SetNamespace(i.namespace)
obj.SetName(i.name)

// Initialize the spec.template.spec to make the object valid in reconciliation.
setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}})

setSpecFields(obj, i.specFields)
return obj
}
Expand All @@ -385,6 +395,9 @@ func (b *BootstrapTemplateBuilder) Build() *unstructured.Unstructured {
obj.SetNamespace(b.namespace)
obj.SetName(b.name)

// Initialize the spec.template.spec to make the object valid in reconciliation.
setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}})

return obj
}

Expand All @@ -403,7 +416,14 @@ func InfrastructureClusterTemplate(namespace, name string) *InfrastructureCluste
}
}

// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object.
// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds
// to the value of the spec field.
//
// Note: all the paths should start with "spec."
//
// Example map: map[string]interface{}{
// "spec.version": "v1.2.3",
// }.
func (i *InfrastructureClusterTemplateBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureClusterTemplateBuilder {
i.specFields = fields
return i
Expand All @@ -417,6 +437,9 @@ func (i *InfrastructureClusterTemplateBuilder) Build() *unstructured.Unstructure
obj.SetNamespace(i.namespace)
obj.SetName(i.name)

// Initialize the spec.template.spec to make the object valid in reconciliation.
setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}})

setSpecFields(obj, i.specFields)

return obj
Expand All @@ -438,7 +461,14 @@ func ControlPlaneTemplate(namespace, name string) *ControlPlaneTemplateBuilder {
}
}

// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object.
// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds
// to the value of the spec field.
//
// Note: all the paths should start with "spec."
//
// Example map: map[string]interface{}{
// "spec.version": "v1.2.3",
// }.
func (c *ControlPlaneTemplateBuilder) WithSpecFields(fields map[string]interface{}) *ControlPlaneTemplateBuilder {
c.specFields = fields
return c
Expand All @@ -458,6 +488,9 @@ func (c *ControlPlaneTemplateBuilder) Build() *unstructured.Unstructured {
obj.SetNamespace(c.namespace)
obj.SetName(c.name)

// Initialize the spec.template.spec to make the object valid in reconciliation.
setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}})

setSpecFields(obj, c.specFields)

if c.infrastructureMachineTemplate != nil {
Expand All @@ -475,7 +508,14 @@ type InfrastructureClusterBuilder struct {
specFields map[string]interface{}
}

// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object.
// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds
// to the value of the spec field.
//
// Note: all the paths should start with "spec."
//
// Example map: map[string]interface{}{
// "spec.version": "v1.2.3",
// }.
func (i *InfrastructureClusterBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureClusterBuilder {
i.specFields = fields
return i
Expand Down Expand Up @@ -628,7 +668,6 @@ func (m *MachineDeploymentBuilder) WithGeneration(generation int64) *MachineDepl
}

// WithStatus sets the passed status object as the status of the machine deployment object.
// TODO (killianmuldoon): Revise making this method consistent with WithSpec fields in objectbuilders.
func (m *MachineDeploymentBuilder) WithStatus(status clusterv1.MachineDeploymentStatus) *MachineDeploymentBuilder {
m.status = &status
return m
Expand Down

0 comments on commit e14d96f

Please sign in to comment.