From 9f625cc6d172c8d0bcd85a9722961d2254224b03 Mon Sep 17 00:00:00 2001
From: Dasha Komsa <komsa.darya@gmail.com>
Date: Tue, 6 Feb 2024 14:00:28 +0000
Subject: [PATCH] fix ready condition not being set to true

---
 internal/controller/ansibleRun/ansibleRun.go  | 23 +++++++++++--------
 .../controller/ansibleRun/ansibleRun_test.go  | 16 +++++++++----
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/internal/controller/ansibleRun/ansibleRun.go b/internal/controller/ansibleRun/ansibleRun.go
index 3bfb6fa..adc9db4 100644
--- a/internal/controller/ansibleRun/ansibleRun.go
+++ b/internal/controller/ansibleRun/ansibleRun.go
@@ -395,7 +395,7 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext
 
 	// disable checkMode for real action
 	c.runner.EnableCheckMode(false)
-	if err := c.runAnsible(cr); err != nil {
+	if err := c.runAnsible(ctx, cr); err != nil {
 		return managed.ExternalUpdate{}, fmt.Errorf("running ansible: %w", err)
 	}
 
@@ -448,6 +448,10 @@ func (c *external) handleLastApplied(ctx context.Context, lastParameters *v1alph
 	isLastSyncOK := (desired.GetCondition(xpv1.TypeSynced).Status == v1.ConditionTrue)
 
 	if isUpToDate && isLastSyncOK {
+		desired.SetConditions(xpv1.Available())
+		if err := c.kube.Status().Update(ctx, desired); err != nil {
+			return managed.ExternalObservation{}, fmt.Errorf("updating status: %w", err)
+		}
 		// nothing to do for this run
 		return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
 	}
@@ -472,7 +476,7 @@ func (c *external) handleLastApplied(ctx context.Context, lastParameters *v1alph
 		return managed.ExternalObservation{}, err
 	}
 
-	if err := c.runAnsible(desired); err != nil {
+	if err := c.runAnsible(ctx, desired); err != nil {
 		return managed.ExternalObservation{}, fmt.Errorf("running ansible: %w", err)
 	}
 
@@ -483,7 +487,7 @@ func (c *external) handleLastApplied(ctx context.Context, lastParameters *v1alph
 	return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
 }
 
-func (c *external) runAnsible(cr *v1alpha1.AnsibleRun) error {
+func (c *external) runAnsible(ctx context.Context, cr *v1alpha1.AnsibleRun) error {
 	dc, _, err := c.runner.Run()
 	if err != nil {
 		return err
@@ -493,16 +497,15 @@ func (c *external) runAnsible(cr *v1alpha1.AnsibleRun) error {
 		cond := xpv1.Unavailable()
 		cond.Message = err.Error()
 		cr.SetConditions(cond)
-
-		return err
+	} else {
+		cr.SetConditions(xpv1.Available())
 	}
 
-	cr.SetConditions(xpv1.Available())
-
-	// no need to persist status update explicitly, cr modifications are in-place and will
-	// be persisted by crossplane-runtime
+	if err := c.kube.Status().Update(ctx, cr); err != nil {
+		return fmt.Errorf("updating status: %w", err)
+	}
 
-	return nil
+	return err
 }
 
 func addBehaviorVars(pc *v1alpha1.ProviderConfig) map[string]string {
diff --git a/internal/controller/ansibleRun/ansibleRun_test.go b/internal/controller/ansibleRun/ansibleRun_test.go
index 25964f4..702f8a6 100644
--- a/internal/controller/ansibleRun/ansibleRun_test.go
+++ b/internal/controller/ansibleRun/ansibleRun_test.go
@@ -611,8 +611,9 @@ func TestObserve(t *testing.T) {
 			reason: "We should not run ansible when spec has not changed and last sync was successful",
 			fields: fields{
 				kube: &test.MockClient{
-					MockGet:    test.NewMockGetFn(nil),
-					MockUpdate: test.NewMockUpdateFn(nil),
+					MockGet:          test.NewMockGetFn(nil),
+					MockUpdate:       test.NewMockUpdateFn(nil),
+					MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil),
 				},
 				runner: &MockRunner{
 					MockAnsibleRunPolicy: func() *ansible.RunPolicy {
@@ -639,8 +640,9 @@ func TestObserve(t *testing.T) {
 			reason: "We should run ansible when spec has not changed but last sync was unsuccessful",
 			fields: fields{
 				kube: &test.MockClient{
-					MockGet:    test.NewMockGetFn(nil),
-					MockUpdate: test.NewMockUpdateFn(nil),
+					MockGet:          test.NewMockGetFn(nil),
+					MockUpdate:       test.NewMockUpdateFn(nil),
+					MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil),
 				},
 				runner: &MockRunner{
 					MockAnsibleRunPolicy: func() *ansible.RunPolicy {
@@ -769,6 +771,9 @@ func TestCreateOrUpdate(t *testing.T) {
 				mg: &v1alpha1.AnsibleRun{},
 			},
 			fields: fields{
+				kube: &test.MockClient{
+					MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil),
+				},
 				runner: &MockRunner{
 					MockAnsibleRunPolicy: func() *ansible.RunPolicy {
 						return &ansible.RunPolicy{
@@ -816,6 +821,9 @@ func TestCreateOrUpdate(t *testing.T) {
 				mg: &v1alpha1.AnsibleRun{},
 			},
 			fields: fields{
+				kube: &test.MockClient{
+					MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil),
+				},
 				runner: &MockRunner{
 					MockAnsibleRunPolicy: func() *ansible.RunPolicy {
 						return &ansible.RunPolicy{