Skip to content
This repository was archived by the owner on Jul 30, 2021. It is now read-only.

Use CAPI utils to reduce boilerplate #106

Merged
merged 2 commits into from
Aug 9, 2019
Merged

Use CAPI utils to reduce boilerplate #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2374daa
Use GetClusterFromMetadata from CAPI
chuckha Aug 9, 2019
6bebe9e
Update to the newest cluster-api
chuckha Aug 9, 2019
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
45 changes: 38 additions & 7 deletions controllers/control_plane_init_locker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,42 @@ import (
)

func TestControlPlaneInitLockerAcquire(t *testing.T) {
tests := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

[1] Are you expecting more test case should be added? otherwise, we can get rid of subtests...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some context:

I took a test that was testing too many things (error case + non error case) and split it into these two test functions. I kept the structure identical to make sure the tests didn't actually change.

I think it's ok to leave the table structure even with only one test.

My general approach here is to: Write a test that does not use table testing. If I need a second or third case I will add table tests if it makes sense, but overall I prefer the look and readability of table tests.

Are you ok leaving this one as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok!

name string
getError error
}{
{
name: "create succeeds",
getError: apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "configmaps"}, "uid1-configmap"),
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
l := &controlPlaneInitLocker{
log: log.ZapLogger(true),
configMapClient: &configMapsGetter{
getError: tc.getError,
},
}

cluster := &clusterv2.Cluster{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns1",
Name: "name1",
UID: types.UID("uid1"),
},
}

acquired := l.Acquire(cluster)
if !acquired {
t.Fatal("acquired was false but it should have been true")
}
})
}
}

func TestControlPlaneInitLockerAcquireErrors(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[1]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one has 3 table tests, I don't think we should get rid of subtests.

Copy link
Contributor

Choose a reason for hiding this comment

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

time to take a break...

tests := []struct {
name string
configMap *v1.ConfigMap
Expand All @@ -49,11 +85,6 @@ func TestControlPlaneInitLockerAcquire(t *testing.T) {
getError: errors.New("get error"),
expectAcquire: false,
},
{
name: "create succeeds",
getError: apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "configmaps"}, "uid1-configmap"),
expectAcquire: true,
},
{
name: "create fails",
getError: apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "configmaps"}, "uid1-configmap"),
Expand Down Expand Up @@ -82,8 +113,8 @@ func TestControlPlaneInitLockerAcquire(t *testing.T) {
}

acquired := l.Acquire(cluster)
if tc.expectAcquire != acquired {
t.Errorf("expected %t, got %t", tc.expectAcquire, acquired)
if acquired {
t.Fatal("expected acquired to be false but it is true")
}
})
}
Expand Down
45 changes: 10 additions & 35 deletions controllers/kubeadmconfig_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,49 +76,24 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro
return ctrl.Result{}, nil
}

// Find the owner reference
// The cluster-api machine controller set this value.
var machineRef *v1.OwnerReference
for _, ref := range config.OwnerReferences {
if ref.Kind == machineKind.Kind && ref.APIVersion == machineKind.GroupVersion().String() {
machineRef = &ref
break
}
}
if machineRef == nil {
log.Info("did not find matching machine reference")
return ctrl.Result{}, nil
}

// Get the machine
machine := &capiv1alpha2.Machine{}
machineKey := client.ObjectKey{
Namespace: req.Namespace,
Name: machineRef.Name,
}

if err := r.Get(ctx, machineKey, machine); err != nil {
log.Error(err, "failed to get machine")
machine, err := util.GetOwnerMachine(ctx, r.Client, config.ObjectMeta)
if err != nil {
log.Error(err, "could not get owner machine")
return ctrl.Result{}, err
}
if machine == nil {
log.Info("Waiting for Machine Controller to set OwnerRef on the KubeadmConfig")
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}

// Ignore machines that already have bootstrap data
if machine.Spec.Bootstrap.Data != nil {
return ctrl.Result{}, nil
}

if machine.Labels[capiv1alpha2.MachineClusterLabelName] == "" {
return ctrl.Result{}, errors.New("machine has no associated cluster")
}

// Get the cluster
cluster := &capiv1alpha2.Cluster{}
clusterKey := client.ObjectKey{
Namespace: req.Namespace,
Name: machine.Labels[capiv1alpha2.MachineClusterLabelName],
}
if err := r.Get(ctx, clusterKey, cluster); err != nil {
log.Error(err, "failed to get cluster")
cluster, err := util.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta)
if err != nil {
log.Error(err, "could not get cluster by machine metadata")
return ctrl.Result{}, err
}

Expand Down
6 changes: 3 additions & 3 deletions controllers/kubeadmconfig_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func TestBailIfKubeadmConfigStatusReady(t *testing.T) {
}
}

func TestBailIfNoMachineRefIsSet(t *testing.T) {
func TestRequeueIfNoMachineRefIsSet(t *testing.T) {
config := newKubeadmConfig(nil, "cfg") // intentionally omitting machine
objects := []runtime.Object{
config,
Expand All @@ -99,8 +99,8 @@ func TestBailIfNoMachineRefIsSet(t *testing.T) {
if result.Requeue == true {
t.Fatal("did not expected to requeue")
}
if result.RequeueAfter != time.Duration(0) {
t.Fatal("did not expected to requeue after")
if result.RequeueAfter == time.Duration(0) {
t.Fatal("Expected a requeue but did not get one")
}
}

Expand Down
5 changes: 3 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
k8s.io/client-go v11.0.1-0.20190409021438-1a26190bd76a+incompatible
k8s.io/cluster-bootstrap v0.0.0-20190516232516-d7d78ab2cfe7
k8s.io/klog v0.3.3
sigs.k8s.io/cluster-api v0.0.0-20190725170330-835ee872f98d
sigs.k8s.io/controller-runtime v0.2.0-beta.4
k8s.io/utils v0.0.0-20190607212802-c55fbcfc754a // indirect
sigs.k8s.io/cluster-api v0.0.0-20190809134526-b8af96f0a42a
sigs.k8s.io/controller-runtime v0.2.0-beta.5
)
13 changes: 8 additions & 5 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ github.com/golang/protobuf v1.3.1 h1:YF8+flBXS5eO826T4nzqPrxfhQThhXl0YzfuUPu4SBg
github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
github.com/google/go-cmp v0.3.0 h1:crn/baboCvb5fXaQ0IJ1SGTsTVrWpDsCWC8EGETZijY=
github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
github.com/google/gofuzz v0.0.0-20170612174753-24818f796faf h1:+RRA9JqSOZFfKrOeqr2z77+8R2RKyh8PG66dcu1V0ck=
github.com/google/gofuzz v0.0.0-20170612174753-24818f796faf/go.mod h1:HP5RmnzzSNb993RKQDq4+1A4ia9nllfqcQFTQJedwGI=
Expand Down Expand Up @@ -248,11 +249,13 @@ k8s.io/kube-openapi v0.0.0-20180731170545-e3762e86a74c h1:3KSCztE7gPitlZmWbNwue/
k8s.io/kube-openapi v0.0.0-20180731170545-e3762e86a74c/go.mod h1:BXM9ceUBTj2QnfH2MK1odQs778ajze1RxcmP6S8RVVc=
k8s.io/utils v0.0.0-20190506122338-8fab8cb257d5 h1:VBM/0P5TWxwk+Nw6Z+lAw3DKgO76g90ETOiA6rfLV1Y=
k8s.io/utils v0.0.0-20190506122338-8fab8cb257d5/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew=
sigs.k8s.io/cluster-api v0.0.0-20190725170330-835ee872f98d h1:DjeaMyqyw5tpps/prbudaojzp8gRMPV6L6MU4C8M8Eg=
sigs.k8s.io/cluster-api v0.0.0-20190725170330-835ee872f98d/go.mod h1:DIiQEP2FjsAiEgBTgT5EEUAVM7XRy7aLgeERFXvZHaQ=
sigs.k8s.io/controller-runtime v0.2.0-beta.4 h1:S1XVfRWR1MuIXZdkYx3jN8JDw+bbQxmWZroy0i87z/A=
sigs.k8s.io/controller-runtime v0.2.0-beta.4/go.mod h1:HweyYKQ8fBuzdu2bdaeBJvsFgAi/OqBBnrVGXcqKhME=
sigs.k8s.io/controller-tools v0.2.0-beta.4/go.mod h1:8t/X+FVWvk6TaBcsa+UKUBbn7GMtvyBKX30SGl4em6Y=
k8s.io/utils v0.0.0-20190607212802-c55fbcfc754a h1:2jUDc9gJja832Ftp+QbDV0tVhQHMISFn01els+2ZAcw=
k8s.io/utils v0.0.0-20190607212802-c55fbcfc754a/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew=
sigs.k8s.io/cluster-api v0.0.0-20190809134526-b8af96f0a42a h1:aDiczQxkpWlxe5yzqm6rJNe5orMuHTt7Q0MqDJOJ/bg=
sigs.k8s.io/cluster-api v0.0.0-20190809134526-b8af96f0a42a/go.mod h1:CgxuxJy6W8onO8duP2ED+s0kwNxsNWIGEXMnSUsxGcw=
sigs.k8s.io/controller-runtime v0.2.0-beta.5 h1:W2jTb239QEwQ+HejhTCF9GriFPy2zmo1I6pPmJTeEy8=
sigs.k8s.io/controller-runtime v0.2.0-beta.5/go.mod h1:HweyYKQ8fBuzdu2bdaeBJvsFgAi/OqBBnrVGXcqKhME=
sigs.k8s.io/controller-tools v0.2.0-beta.5/go.mod h1:8t/X+FVWvk6TaBcsa+UKUBbn7GMtvyBKX30SGl4em6Y=
sigs.k8s.io/testing_frameworks v0.1.1/go.mod h1:VVBKrHmJ6Ekkfz284YKhQePcdycOzNH9qL6ht1zEr/U=
sigs.k8s.io/testing_frameworks v0.1.2-0.20190130140139-57f07443c2d4 h1:GtDhkj3cF4A4IW+A9LScsuxvJqA9DE7G7PGH1f8B07U=
sigs.k8s.io/testing_frameworks v0.1.2-0.20190130140139-57f07443c2d4/go.mod h1:VVBKrHmJ6Ekkfz284YKhQePcdycOzNH9qL6ht1zEr/U=
Expand Down