From 8ef4b2da47037653bdaad335fbabde4b3188335e Mon Sep 17 00:00:00 2001 From: Alexandre Gaudreault Date: Tue, 1 Jul 2025 15:44:33 -0400 Subject: [PATCH 1/2] refactor: compare comparedTo source with source to compare Signed-off-by: Alexandre Gaudreault --- controller/state.go | 12 ++++++------ pkg/apis/application/v1alpha1/types.go | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/controller/state.go b/controller/state.go index a24856eeb498d..61ad5caafe44a 100644 --- a/controller/state.go +++ b/controller/state.go @@ -553,7 +553,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 if hasMultipleSources { return &comparisonResult{ syncStatus: &v1alpha1.SyncStatus{ - ComparedTo: app.Spec.BuildComparedToStatus(), + ComparedTo: app.Spec.BuildComparedToStatus(sources), Status: v1alpha1.SyncStatusCodeUnknown, Revisions: revisions, }, @@ -562,7 +562,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 } return &comparisonResult{ syncStatus: &v1alpha1.SyncStatus{ - ComparedTo: app.Spec.BuildComparedToStatus(), + ComparedTo: app.Spec.BuildComparedToStatus(sources), Status: v1alpha1.SyncStatusCodeUnknown, Revision: revisions[0], }, @@ -1062,7 +1062,7 @@ func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sou return false } - if !specEqualsCompareTo(app.Spec, app.Status.Sync.ComparedTo) { + if !specEqualsCompareTo(app.Spec, sources, app.Status.Sync.ComparedTo) { log.WithField("useDiffCache", "false").Debug("specChanged") return false } @@ -1073,11 +1073,11 @@ func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sou // specEqualsCompareTo compares the application spec to the comparedTo status. It normalizes the destination to match // the comparedTo destination before comparing. It does not mutate the original spec or comparedTo. -func specEqualsCompareTo(spec v1alpha1.ApplicationSpec, comparedTo v1alpha1.ComparedTo) bool { +func specEqualsCompareTo(spec v1alpha1.ApplicationSpec, sources []v1alpha1.ApplicationSource, comparedTo v1alpha1.ComparedTo) bool { // Make a copy to be sure we don't mutate the original. specCopy := spec.DeepCopy() - currentSpec := specCopy.BuildComparedToStatus() - return reflect.DeepEqual(comparedTo, currentSpec) + compareToSpec := specCopy.BuildComparedToStatus(sources) + return reflect.DeepEqual(comparedTo, compareToSpec) } func (m *appStateManager) persistRevisionHistory( diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index 5f1f4ee6a2ac4..fbc79f9104837 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -1241,15 +1241,15 @@ func (status *ApplicationStatus) GetRevisions() []string { // BuildComparedToStatus will build a ComparedTo object based on the current // Application state. -func (spec *ApplicationSpec) BuildComparedToStatus() ComparedTo { +func (spec *ApplicationSpec) BuildComparedToStatus(sources []ApplicationSource) ComparedTo { ct := ComparedTo{ Destination: spec.Destination, IgnoreDifferences: spec.IgnoreDifferences, } if spec.HasMultipleSources() { - ct.Sources = spec.Sources + ct.Sources = sources } else { - ct.Source = spec.GetSource() + ct.Source = sources[0] } return ct } From 49a71b14a2f755058689ec02db7f0a94f3f472d8 Mon Sep 17 00:00:00 2001 From: Alexandre Gaudreault Date: Tue, 1 Jul 2025 16:58:43 -0400 Subject: [PATCH 2/2] unit tests Signed-off-by: Alexandre Gaudreault --- controller/state_test.go | 47 ++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/controller/state_test.go b/controller/state_test.go index e6636b95f1e41..df6b4f7de4dfb 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -25,6 +25,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" "github.com/argoproj/argo-cd/v3/common" "github.com/argoproj/argo-cd/v3/controller/testdata" @@ -1481,7 +1482,6 @@ func TestIsLiveResourceManaged(t *testing.T) { func TestUseDiffCache(t *testing.T) { t.Parallel() - type fixture struct { testName string noCache bool @@ -1493,7 +1493,6 @@ func TestUseDiffCache(t *testing.T) { expectedUseCache bool serverSideDiff bool } - manifestInfos := func(revision string) []*apiclient.ManifestResponse { return []*apiclient.ManifestResponse{ { @@ -1509,15 +1508,16 @@ func TestUseDiffCache(t *testing.T) { }, } } - sources := func() []v1alpha1.ApplicationSource { - return []v1alpha1.ApplicationSource{ - { - RepoURL: "https://some-repo.com", - Path: "argocd/httpbin", - TargetRevision: "HEAD", - }, + source := func() v1alpha1.ApplicationSource { + return v1alpha1.ApplicationSource{ + RepoURL: "https://some-repo.com", + Path: "argocd/httpbin", + TargetRevision: "HEAD", } } + sources := func() []v1alpha1.ApplicationSource { + return []v1alpha1.ApplicationSource{source()} + } app := func(namespace string, revision string, refresh bool, a *v1alpha1.Application) *v1alpha1.Application { app := &v1alpha1.Application{ @@ -1526,11 +1526,7 @@ func TestUseDiffCache(t *testing.T) { Namespace: namespace, }, Spec: v1alpha1.ApplicationSpec{ - Source: &v1alpha1.ApplicationSource{ - RepoURL: "https://some-repo.com", - Path: "argocd/httpbin", - TargetRevision: "HEAD", - }, + Source: ptr.To(source()), Destination: v1alpha1.ApplicationDestination{ Server: "https://kubernetes.default.svc", Namespace: "httpbin", @@ -1548,11 +1544,7 @@ func TestUseDiffCache(t *testing.T) { Sync: v1alpha1.SyncStatus{ Status: v1alpha1.SyncStatusCodeSynced, ComparedTo: v1alpha1.ComparedTo{ - Source: v1alpha1.ApplicationSource{ - RepoURL: "https://some-repo.com", - Path: "argocd/httpbin", - TargetRevision: "HEAD", - }, + Source: source(), Destination: v1alpha1.ApplicationDestination{ Server: "https://kubernetes.default.svc", Namespace: "httpbin", @@ -1577,7 +1569,6 @@ func TestUseDiffCache(t *testing.T) { } return app } - cases := []fixture{ { testName: "will use diff cache", @@ -1594,7 +1585,7 @@ func TestUseDiffCache(t *testing.T) { testName: "will use diff cache with sync policy", noCache: false, manifestInfos: manifestInfos("rev1"), - sources: sources(), + sources: []v1alpha1.ApplicationSource{test.YamlToApplication(testdata.DiffCacheYaml).Status.Sync.ComparedTo.Source}, app: test.YamlToApplication(testdata.DiffCacheYaml), manifestRevisions: []string{"rev1"}, statusRefreshTimeout: time.Hour * 24, @@ -1604,8 +1595,15 @@ func TestUseDiffCache(t *testing.T) { { testName: "will use diff cache for multisource", noCache: false, - manifestInfos: manifestInfos("rev1"), - sources: sources(), + manifestInfos: append(manifestInfos("rev1"), manifestInfos("rev2")...), + sources: v1alpha1.ApplicationSources{ + { + RepoURL: "multisource repo1", + }, + { + RepoURL: "multisource repo2", + }, + }, app: app("httpbin", "", false, &v1alpha1.Application{ Spec: v1alpha1.ApplicationSpec{ Source: nil, @@ -1743,16 +1741,13 @@ func TestUseDiffCache(t *testing.T) { } for _, tc := range cases { - tc := tc t.Run(tc.testName, func(t *testing.T) { // Given t.Parallel() logger, _ := logrustest.NewNullLogger() log := logrus.NewEntry(logger) - // When useDiffCache := useDiffCache(tc.noCache, tc.manifestInfos, tc.sources, tc.app, tc.manifestRevisions, tc.statusRefreshTimeout, tc.serverSideDiff, log) - // Then assert.Equal(t, tc.expectedUseCache, useDiffCache) })