review

Signed-off-by: Feny Mehta <[email protected]>

controllers/toolchaincluster/healthchecker_test.go

@@ -2,6 +2,7 @@ package toolchaincluster
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
@@ -52,6 +53,7 @@ func TestClusterHealthChecks(t *testing.T) {
 			tctype:      "Notfound",
 			apiendpoint: "http://not-found.com",
 			healthcheck: false,
+			errh:        fmt.Errorf("the server could not find the requested resource"),
 		},
 	}
 	for k, tc := range tests {
@@ -71,8 +73,8 @@ func TestClusterHealthChecks(t *testing.T) {
 
 			//then
 			require.Equal(t, tc.healthcheck, healthcheck)
-			if tc.tctype == "Notfound" {
-				require.Error(t, errh)
+			if tc.errh != nil {
+				require.EqualError(t, errh, tc.errh.Error())
 			} else {
 				require.NoError(t, errh)
 			}

controllers/toolchaincluster/toolchaincluster_controller.go

@@ -27,7 +27,7 @@ type Reconciler struct {
 	Client      client.Client
 	Scheme      *runtime.Scheme
 	RequeAfter  time.Duration
-	CheckHealth func(context.Context, *kubeclientset.Clientset) (bool, error)
+	checkHealth func(context.Context, *kubeclientset.Clientset) (bool, error)
 }
 
 // SetupWithManager sets up the controller with the Manager.
@@ -84,7 +84,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 	healthcheckResult := r.getClusterHealthCondition(ctx, clientSet)
 
 	// update the status of the individual cluster.
-	if err := r.updateStatus(ctx, toolchainCluster, healthcheckResult...); err != nil {
+	if err := r.updateStatus(ctx, toolchainCluster, healthcheckResult); err != nil {
 		reqLogger.Error(err, "unable to update cluster status of ToolchainCluster")
 		return reconcile.Result{}, err
 	}
@@ -99,22 +99,33 @@ func (r *Reconciler) updateStatus(ctx context.Context, toolchainCluster *toolcha
 	return nil
 }
 
-func (r *Reconciler) getClusterHealthCondition(ctx context.Context, remoteClusterClientset *kubeclientset.Clientset) []v1alpha1.Condition {
+func (r *Reconciler) getClusterHealthCondition(ctx context.Context, remoteClusterClientset *kubeclientset.Clientset) v1alpha1.Condition {
 
-	healthcheck, errhealth := r.getClusterHealth(ctx, remoteClusterClientset)
-	if errhealth != nil || !healthcheck {
-		return []v1alpha1.Condition{clusterNotReadyCondition()}
+	ishealthy, err := r.getClusterHealth(ctx, remoteClusterClientset)
+	if err != nil {
+		return clusterOfflineCondition(err.Error())
+	} else {
+		if !ishealthy {
+			return clusterNotReadyCondition()
+		}
+		return clusterReadyCondition()
 	}
-	return []v1alpha1.Condition{clusterReadyCondition()}
-
 }
 
 func (r *Reconciler) getClusterHealth(ctx context.Context, remoteClusterClientset *kubeclientset.Clientset) (bool, error) {
-	if r.CheckHealth != nil {
-		return r.CheckHealth(ctx, remoteClusterClientset)
+	if r.checkHealth != nil {
+		return r.checkHealth(ctx, remoteClusterClientset)
 	}
 	return getClusterHealthStatus(ctx, remoteClusterClientset)
 }
+func clusterOfflineCondition(errMsg string) toolchainv1alpha1.Condition {
+	return toolchainv1alpha1.Condition{
+		Type:    toolchainv1alpha1.ConditionReady,
+		Status:  corev1.ConditionFalse,
+		Reason:  toolchainv1alpha1.ToolchainClusterClusterNotReachableReason,
+		Message: errMsg,
+	}
+}
 
 func clusterReadyCondition() toolchainv1alpha1.Condition {
 	return toolchainv1alpha1.Condition{

controllers/toolchaincluster/toolchaincluster_controller_test.go

@@ -137,19 +137,19 @@ func TestClusterControllerChecks(t *testing.T) {
 		controller, req := prepareReconcile(stable, cl, requeAfter)
 
 		// when
-		recresult, err := controller.Reconcile(context.TODO(), req)
+		recResult, err := controller.Reconcile(context.TODO(), req)
 
 		// then
 		require.EqualError(t, err, "cluster stable not found in cache")
-		require.Equal(t, reconcile.Result{}, recresult)
+		require.Equal(t, reconcile.Result{}, recResult)
 
 		actualtoolchaincluster := &toolchainv1alpha1.ToolchainCluster{}
 		err = cl.Client.Get(context.TODO(), types.NamespacedName{Name: "stable", Namespace: tcNs}, actualtoolchaincluster)
 		require.NoError(t, err)
 		assertClusterStatus(t, cl, "stable", clusterReadyCondition())
 	})
 
-	t.Run("error while updating a toolchain cluster status", func(t *testing.T) {
+	t.Run("error while updating a toolchain cluster status when health-check failed", func(t *testing.T) {
 		// given
 		stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{})
 		serr := fmt.Errorf("my test error")
@@ -161,7 +161,7 @@ func TestClusterControllerChecks(t *testing.T) {
 
 		defer reset()
 		controller, req := prepareReconcile(stable, cl, requeAfter)
-		controller.CheckHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) {
+		controller.checkHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) {
 			return false, serr
 		}
 		// when
@@ -217,7 +217,7 @@ func TestGetClusterHealth(t *testing.T) {
 
 		defer reset()
 		controller, req := prepareReconcile(stable, cl, requeAfter)
-		controller.CheckHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) {
+		controller.checkHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) {
 			return true, nil
 		}
 
@@ -238,7 +238,7 @@ func TestGetClusterHealth(t *testing.T) {
 
 		defer reset()
 		controller, req := prepareReconcile(stable, cl, requeAfter)
-		controller.CheckHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) {
+		controller.checkHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) {
 			return false, nil
 		}
 
@@ -314,12 +314,10 @@ func assertClusterStatus(t *testing.T, cl client.Client, clusterName string, clu
 ExpConditions:
 	for _, expCond := range clusterConds {
 		for _, cond := range tc.Status.Conditions {
-			if expCond.Type == cond.Type {
-				assert.Equal(t, expCond.Status, cond.Status)
-				assert.Equal(t, expCond.Reason, cond.Reason)
-				assert.Equal(t, expCond.Message, cond.Message)
-				continue ExpConditions
-			}
+			assert.Equal(t, expCond.Status, cond.Status)
+			assert.Equal(t, expCond.Reason, cond.Reason)
+			assert.Equal(t, expCond.Message, cond.Message)
+			continue ExpConditions
 		}
 		assert.Failf(t, "condition not found", "the list of conditions %v doesn't contain the expected condition %v", tc.Status.Conditions, expCond)
 	}