fix(appset): When Appset is deleted, the controller should reconcile applicationset argoproj#23723 (argoproj#23823)

Signed-off-by: Sangeetha Madamanchi <[email protected]>
Co-authored-by: Sangeetha Madamanchi <[email protected]>

Author: sangdammad

applicationset/controllers/applicationset_controller.go

@@ -1696,24 +1696,21 @@ func shouldRequeueForApplicationSet(appSetOld, appSetNew *argov1alpha1.Applicati
 	}
 
 	// Requeue if any ApplicationStatus.Status changed for Progressive sync strategy
-	// Requeue if deletionTimestamp added
 	if enableProgressiveSyncs {
 		if !cmp.Equal(appSetOld.Status.ApplicationStatus, appSetNew.Status.ApplicationStatus, cmpopts.EquateEmpty()) {
 			return true
 		}
-		if !cmp.Equal(appSetOld.DeletionTimestamp, appSetNew.DeletionTimestamp, cmpopts.EquateEmpty()) {
-			return true
-		}
 	}
 
-	// only compare the applicationset spec, annotations, labels and finalizers, specifically avoiding
+	// only compare the applicationset spec, annotations, labels and finalizers, deletionTimestamp, specifically avoiding
 	// the status field. status is owned by the applicationset controller,
 	// and we do not need to requeue when it does bookkeeping
 	// NB: the ApplicationDestination comes from the ApplicationSpec being embedded
 	// in the ApplicationSetTemplate from the generators
 	if !cmp.Equal(appSetOld.Spec, appSetNew.Spec, cmpopts.EquateEmpty(), cmpopts.EquateComparable(argov1alpha1.ApplicationDestination{})) ||
 		!cmp.Equal(appSetOld.GetLabels(), appSetNew.GetLabels(), cmpopts.EquateEmpty()) ||
-		!cmp.Equal(appSetOld.GetFinalizers(), appSetNew.GetFinalizers(), cmpopts.EquateEmpty()) {
+		!cmp.Equal(appSetOld.GetFinalizers(), appSetNew.GetFinalizers(), cmpopts.EquateEmpty()) ||
+		!cmp.Equal(appSetOld.DeletionTimestamp, appSetNew.DeletionTimestamp, cmpopts.EquateEmpty()) {
 		return true
 	}
 

applicationset/controllers/applicationset_controller_test.go

@@ -7126,7 +7126,7 @@ func TestApplicationSetOwnsHandlerUpdate(t *testing.T) {
 				},
 			},
 			enableProgressiveSyncs: false,
-			want:                   false,
+			want:                   true,
 		},
 	}
 
@@ -7276,6 +7276,36 @@ func TestShouldRequeueForApplicationSet(t *testing.T) {
 			},
 			want: true,
 		},
+		{
+			name: "ApplicationSetWithDeletionTimestamp",
+			args: args{
+				appSetOld: &v1alpha1.ApplicationSet{
+					Status: v1alpha1.ApplicationSetStatus{
+						ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
+							{
+								Application: "app1",
+								Status:      "Healthy",
+							},
+						},
+					},
+				},
+				appSetNew: &v1alpha1.ApplicationSet{
+					ObjectMeta: metav1.ObjectMeta{
+						DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					},
+					Status: v1alpha1.ApplicationSetStatus{
+						ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
+							{
+								Application: "app1",
+								Status:      "Waiting",
+							},
+						},
+					},
+				},
+				enableProgressiveSyncs: false,
+			},
+			want: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {