Repository: kubesphere/ks-devops
Pull request #518 (2022-03-25 09:59:59)

Fix an issue where the application status subresource was not updated
### What type of PR is this?

/kind bug

### What this PR does / why we need it:

Fix an issue where the application status subresource was not updated. 

This issue was produced by https://github.com/kubesphere/ks-devops/pull/506.

### Which issue(s) this PR fixes:

Fixes #

### Special notes for reviewers:

Please check the following list before waiting reviewers:

- [ ] Already committed the CRD files to [the Helm Chart](https://github.com/kubesphere-sigs/ks-devops-helm-chart/) if you created some new CRDs
- [ ] Already [added the permission](https://github.com/kubesphere/ks-installer/blob/9e063b085a0e43fdb3d0d9e3e7f4149146f14b9c/roles/ks-core/prepare/files/ks-init/role-templates.yaml) for the new API
- [ ] Already added the RBAC markers for the new controllers

### Does this PR introduce a user-facing change??

```release-note
None
```

/cc @kubesphere/sig-devops 


ks-ci-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubesphere/ks-devops/pull/518#" title="Author self-approved">JohnNiang</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **linuxsuren** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @linuxsuren` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubesphere%2Fks-devops).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubesphere/ks-devops/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["linuxsuren"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/kubesphere/ks-devops/pull/518?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) Report
> Merging [#518](https://codecov.io/gh/kubesphere/ks-devops/pull/518?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) (7e0e40a) into [master](https://codecov.io/gh/kubesphere/ks-devops/commit/1f560cac527872dfe90bd8c9872b57c4e049f2ac?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) (1f560ca) will **increase** coverage by `0.01%`.
> The diff coverage is `86.66%`.

```diff
@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
+ Coverage   77.67%   77.69%   +0.01%     
==========================================
  Files         140      140              
  Lines       22845    22842       -3     
==========================================
+ Hits        17745    17747       +2     
+ Misses       4908     4904       -4     
+ Partials      192      191       -1     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `77.69% <86.66%> (+0.01%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/kubesphere/ks-devops/pull/518?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) | Coverage Δ | |
|---|---|---|
| [...ers/argocd/argocd-application-status-controller.go](https://codecov.io/gh/kubesphere/ks-devops/pull/518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-Y29udHJvbGxlcnMvYXJnb2NkL2FyZ29jZC1hcHBsaWNhdGlvbi1zdGF0dXMtY29udHJvbGxlci5nbw==) | `76.19% <84.61%> (+1.83%)` | :arrow_up: |
| [controllers/argocd/application\_controller.go](https://codecov.io/gh/kubesphere/ks-devops/pull/518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-Y29udHJvbGxlcnMvYXJnb2NkL2FwcGxpY2F0aW9uX2NvbnRyb2xsZXIuZ28=) | `51.92% <100.00%> (-4.98%)` | :arrow_down: |
| [.../jenkins/devopsproject/devopsproject\_controller.go](https://codecov.io/gh/kubesphere/ks-devops/pull/518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-Y29udHJvbGxlcnMvamVua2lucy9kZXZvcHNwcm9qZWN0L2Rldm9wc3Byb2plY3RfY29udHJvbGxlci5nbw==) | `43.22% <0.00%> (+2.60%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kubesphere/ks-devops/pull/518?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kubesphere/ks-devops/pull/518?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere). Last update [1f560ca...7e0e40a](https://codecov.io/gh/kubesphere/ks-devops/pull/518?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere).


Patch:
diff --git a/controllers/argocd/application_controller.go b/controllers/argocd/application_controller.go
index 2f763d1..2970a07 100644
--- a/controllers/argocd/application_controller.go
+++ b/controllers/argocd/application_controller.go
@@ -22,7 +22,6 @@ import (
 	"github.com/go-logr/logr"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
 	"kubesphere.io/devops/pkg/api/gitops/v1alpha1"
@@ -61,9 +60,7 @@ func (r *ApplicationReconciler) Reconcile(req ctrl.Request) (result ctrl.Result,
 func (r *ApplicationReconciler) reconcileArgoApplication(app *v1alpha1.Application) (err error) {
 	ctx := context.Background()
 
-	argoApp := &unstructured.Unstructured{}
-	argoApp.SetKind("Application")
-	argoApp.SetAPIVersion("argoproj.io/v1alpha1")
+	argoApp := createBareArgoCDApplicationObject()
 
 	if err = r.Get(ctx, types.NamespacedName{
 		Namespace: app.GetNamespace(),
@@ -105,12 +102,7 @@ func createUnstructuredApplication(app *v1alpha1.Application) (result *unstructu
 		argoApp.Spec.Destination.Server = ""
 	}
 
-	newArgoApp := &unstructured.Unstructured{}
-	newArgoApp.SetGroupVersionKind(schema.GroupVersionKind{
-		Group:   "argoproj.io",
-		Version: "v1alpha1",
-		Kind:    "Application",
-	})
+	newArgoApp := createBareArgoCDApplicationObject()
 	newArgoApp.SetName(app.GetName())
 	newArgoApp.SetNamespace(app.GetNamespace())
 
diff --git a/controllers/argocd/argocd-application-status-controller.go b/controllers/argocd/argocd-application-status-controller.go
index 33aa611..da39981 100644
--- a/controllers/argocd/argocd-application-status-controller.go
+++ b/controllers/argocd/argocd-application-status-controller.go
@@ -21,9 +21,11 @@ import (
 	"encoding/json"
 	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
 	"kubesphere.io/devops/pkg/api/gitops/v1alpha1"
+	"reflect"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -57,8 +59,8 @@ func (r *ApplicationStatusReconciler) Reconcile(req ctrl.Request) (result ctrl.R
 	var status map[string]interface{}
 	if status, _, err = unstructured.NestedMap(argoCDApp.Object, "status"); err == nil {
 		var statusData []byte
-		if statusData, err = json.Marshal(status); err == nil {
-			app = app.DeepCopy()
+		if statusData, err = json.Marshal(status); err == nil && !reflect.DeepEqual([]byte(app.Status.ArgoApp), statusData) {
+			//app = app.DeepCopy()
 			if app.GetLabels() == nil {
 				// make sure the labels are not nil
 				app.SetLabels(map[string]string{})
@@ -71,15 +73,29 @@ func (r *ApplicationStatusReconciler) Reconcile(req ctrl.Request) (result ctrl.R
 			if healthStatus, found, _ := unstructured.NestedString(status, "health", "status"); found {
 				app.GetLabels()[v1alpha1.HealthStatusLabelKey] = healthStatus
 			}
+
+			// unset operation field if it was absent
+			if _, found, err := unstructured.NestedMap(argoCDApp.Object, "operation"); err != nil {
+				return ctrl.Result{}, err
+			} else if !found && app.Spec.ArgoApp != nil {
+				app.Spec.ArgoApp.Operation = nil
+			}
+
+			// update labels
+			if err = r.Update(ctx, app); err != nil {
+				return
+			}
+
+			// update status subresource
 			app.Status.ArgoApp = string(statusData)
-			err = r.Update(ctx, app)
+			err = r.Status().Update(ctx, app)
 		}
 	}
 	return
 }
 
 func getArgoCDApplication(client client.Reader, namespacedName types.NamespacedName) (app *unstructured.Unstructured, err error) {
-	app = getArgoCDApplicationObject()
+	app = createBareArgoCDApplicationObject()
 
 	if err = client.Get(context.Background(), namespacedName, app); err != nil {
 		app = nil
@@ -97,19 +113,22 @@ func (r *ApplicationStatusReconciler) GetGroupName() string {
 	return controllerGroupName
 }
 
-func getArgoCDApplicationObject() *unstructured.Unstructured {
-	cluster := &unstructured.Unstructured{}
-	cluster.SetKind("Application")
-	cluster.SetAPIVersion("argoproj.io/v1alpha1")
-	return cluster.DeepCopy()
+func createBareArgoCDApplicationObject() *unstructured.Unstructured {
+	argoApp := &unstructured.Unstructured{}
+	argoApp.SetGroupVersionKind(schema.GroupVersionKind{
+		Group:   "argoproj.io",
+		Version: "v1alpha1",
+		Kind:    "Application",
+	})
+	return argoApp
 }
 
 // SetupWithManager init the logger, recorder and filters
 func (r *ApplicationStatusReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	cluster := getArgoCDApplicationObject()
+	argoApp := createBareArgoCDApplicationObject()
 	r.log = ctrl.Log.WithName(r.GetName())
 	r.recorder = mgr.GetEventRecorderFor(r.GetName())
 	return ctrl.NewControllerManagedBy(mgr).
-		For(cluster).
+		For(argoApp).
 		Complete(r)
 }


Test patch:
diff --git a/controllers/argocd/argocd-application-status-controller_test.go b/controllers/argocd/argocd-application-status-controller_test.go
index 85abde7..35df0eb 100644
--- a/controllers/argocd/argocd-application-status-controller_test.go
+++ b/controllers/argocd/argocd-application-status-controller_test.go
@@ -99,7 +99,7 @@ func Test_getArgoCDApplicationObject(t *testing.T) {
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			tt.verify(t, getArgoCDApplicationObject())
+			tt.verify(t, createBareArgoCDApplicationObject())
 		})
 	}
 }