From d3d8f375b0b3a7fd1aa6273b718eebbd47b3defc Mon Sep 17 00:00:00 2001 From: rumstead <37445536+rumstead@users.noreply.github.com> Date: Tue, 23 Apr 2024 16:50:11 -0400 Subject: [PATCH] fix: use cmp vs reflect.DeepEqual for comparing Applications (#17861) (#17940) * fix(compare): appset compare the child apps with cmp vs reflect Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * remove debug lines Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * remove debug lines Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> --------- Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> --- .../controllers/applicationset_controller.go | 15 +++-- .../applicationset_controller_test.go | 56 +++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index e640e78a896bf..f8492a998eec0 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -17,9 +17,10 @@ package controllers import ( "context" "fmt" - "reflect" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" log "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" apierr "k8s.io/apimachinery/pkg/api/errors" @@ -1613,10 +1614,14 @@ func shouldRequeueApplicationSet(appOld *argov1alpha1.Application, appNew *argov } // the applicationset controller owns the application spec, labels, annotations, and finalizers on the applications - if !reflect.DeepEqual(appOld.Spec, appNew.Spec) || - !reflect.DeepEqual(appOld.ObjectMeta.GetAnnotations(), appNew.ObjectMeta.GetAnnotations()) || - !reflect.DeepEqual(appOld.ObjectMeta.GetLabels(), appNew.ObjectMeta.GetLabels()) || - !reflect.DeepEqual(appOld.ObjectMeta.GetFinalizers(), appNew.ObjectMeta.GetFinalizers()) { + // reflect.DeepEqual considers nil slices/maps not equal to empty slices/maps + // https://pkg.go.dev/reflect#DeepEqual + // ApplicationDestination has an unexported field so we can just use the == for comparsion + if !cmp.Equal(appOld.Spec, appNew.Spec, cmpopts.EquateEmpty(), cmpopts.EquateComparable(argov1alpha1.ApplicationDestination{})) || + !cmp.Equal(appOld.ObjectMeta.GetAnnotations(), appNew.ObjectMeta.GetAnnotations(), cmpopts.EquateEmpty()) || + !cmp.Equal(appOld.ObjectMeta.GetLabels(), appNew.ObjectMeta.GetLabels(), cmpopts.EquateEmpty()) || + !cmp.Equal(appOld.ObjectMeta.GetFinalizers(), appNew.ObjectMeta.GetFinalizers(), cmpopts.EquateEmpty()) { + return true } diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index a5e5858bd9c44..244b2797ac684 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -6385,14 +6385,70 @@ func TestOwnsHandler(t *testing.T) { ObjectOld: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}}, ObjectNew: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"bar": "foo"}}}, }}, want: true}, + {name: "DifferentApplicationLabelsNil", args: args{e: event.UpdateEvent{ + ObjectOld: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, + ObjectNew: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Labels: nil}}, + }}, want: false}, {name: "DifferentApplicationAnnotations", args: args{e: event.UpdateEvent{ ObjectOld: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"foo": "bar"}}}, ObjectNew: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"bar": "foo"}}}, }}, want: true}, + {name: "DifferentApplicationAnnotationsNil", args: args{e: event.UpdateEvent{ + ObjectOld: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}}, + ObjectNew: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: nil}}, + }}, want: false}, {name: "DifferentApplicationFinalizers", args: args{e: event.UpdateEvent{ ObjectOld: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{"argo"}}}, ObjectNew: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{"none"}}}, }}, want: true}, + {name: "DifferentApplicationFinalizersNil", args: args{e: event.UpdateEvent{ + ObjectOld: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{}}}, + ObjectNew: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Finalizers: nil}}, + }}, want: false}, + {name: "ApplicationDestinationSame", args: args{e: event.UpdateEvent{ + ObjectOld: &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "server", + Namespace: "ns", + Name: "name", + }, + }, + }, + ObjectNew: &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "server", + Namespace: "ns", + Name: "name", + }, + }, + }, + }, + enableProgressiveSyncs: true, + }, want: false}, + {name: "ApplicationDestinationDiff", args: args{e: event.UpdateEvent{ + ObjectOld: &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "server", + Namespace: "ns", + Name: "name", + }, + }, + }, + ObjectNew: &v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Destination: v1alpha1.ApplicationDestination{ + Server: "notSameServer", + Namespace: "ns", + Name: "name", + }, + }, + }, + }, + enableProgressiveSyncs: true, + }, want: true}, {name: "NotAnAppOld", args: args{e: event.UpdateEvent{ ObjectOld: &v1alpha1.AppProject{}, ObjectNew: &v1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"bar": "foo"}}},