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

🐛 Fix error-return for machine-cluster association #226

Merged
merged 1 commit into from
Sep 13, 2019
+18 −10
Merged

🐛 Fix error-return for machine-cluster association #226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions controllers/kubeadmconfig_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
// Lookup the cluster the machine is associated with
cluster, err := util.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do y'all think about a switch statement instead?

switch {
case errors.Cause(err) == util.ErrNoCluster:
case apierrors.IsNotFound(err):
case err != nil:
}

?

if errors.Cause(err) == util.ErrNoCluster {
log.Info("Machine does not belong to a cluster yet, waiting until its part of a cluster")
return ctrl.Result{}, nil
}

if apierrors.IsNotFound(err) {
log.Info("Cluster does not exist yet , waiting until it is created")
return ctrl.Result{}, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing a clean way to switch this into a switch statement to appease the linter, but since each branch has a return, you can remove the else clauses:

		if errors.Cause(err) == util.ErrNoCluster {
			log.Info("Machine does not belong to a cluster yet, waiting until its part of a cluster")
			return ctrl.Result{}, nil
		}

		if apierrors.IsNotFound(err) {
			log.Info("Cluster does not exist yet , waiting until it is created")
			return ctrl.Result{}, nil
		}

		log.Error(err, "could not get cluster by machine metadata")
		return ctrl.Result{}, err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@detiber Thanks. I will do the change


log.Error(err, "could not get cluster by machine metadata")
return ctrl.Result{}, err
}
Expand Down
18 changes: 8 additions & 10 deletions controllers/kubeadmconfig_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,8 @@ func TestKubeadmConfigReconciler_Reconcile_ReturnEarlyIfMachineHasBootstrapData(
}
}

// This returns an error since nothing can proceed without an associated cluster.
// TODO: This should probably not return an error
func TestKubeadmConfigReconciler_Reconcile_ReturnErrorIfMachineDoesNotHaveAssociatedCluster(t *testing.T) {
// This does not expect an error, hoping the machine gets updated with a cluster
func TestKubeadmConfigReconciler_Reconcile_ReturnNilIfMachineDoesNotHaveAssociatedCluster(t *testing.T) {
machine := newMachine(nil, "machine") // intentionally omitting cluster
config := newKubeadmConfig(machine, "cfg")

Expand All @@ -176,14 +175,13 @@ func TestKubeadmConfigReconciler_Reconcile_ReturnErrorIfMachineDoesNotHaveAssoci
},
}
_, err := k.Reconcile(request)
if err == nil {
t.Fatal("Expected error, got nil")
if err != nil {
t.Fatal("Not Expecting error, got an error")
}
}

// If the associated cluster is not found then there is no way to proceed.
// TODO: This should probably not be an error
func TestKubeadmConfigReconciler_Reconcile_ReturnErrorIfAssociatedClusterIsNotFound(t *testing.T) {
// This does not expect an error, hoping that the associated cluster will be created
func TestKubeadmConfigReconciler_Reconcile_ReturnNilIfAssociatedClusterIsNotFound(t *testing.T) {
cluster := newCluster("cluster")
machine := newMachine(cluster, "machine")
config := newKubeadmConfig(machine, "cfg")
Expand All @@ -207,8 +205,8 @@ func TestKubeadmConfigReconciler_Reconcile_ReturnErrorIfAssociatedClusterIsNotFo
},
}
_, err := k.Reconcile(request)
if err == nil {
t.Fatal("Expected error, got nil")
if err != nil {
t.Fatal("Not Expecting error, got an error")
}
}

Expand Down