refactor: simplify UpdateRevisionForPaths logic and add early returns (#27190)

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

controller/state.go

@@ -242,56 +242,20 @@ func (m *appStateManager) GetRepoObjs(ctx context.Context, app *v1alpha1.Applica
 			return nil, nil, false, fmt.Errorf("failed to get repo %q: %w", source.RepoURL, err)
 		}
 
-		syncedRevision := app.Status.Sync.Revision
-		if app.Spec.HasMultipleSources() {
-			if i < len(app.Status.Sync.Revisions) {
-				syncedRevision = app.Status.Sync.Revisions[i]
-			} else {
-				syncedRevision = ""
-			}
-		}
-
 		revision := revisions[i]
 
 		appNamespace := app.Spec.Destination.Namespace
 		apiVersions := argo.APIResourcesToStrings(apiResources, true)
 
-		if repo.Depth == 0 && syncedRevision != "" && !source.IsRef() && keyManifestGenerateAnnotationExists && keyManifestGenerateAnnotationVal != "" && (syncedRevision != revision || app.Spec.HasMultipleSources()) {
+		// Evaluate if the revision has changes
-			// Validate the manifest-generate-path annotation to avoid generating manifests if it has not changed.
+		resolvedRevision, hasChanges, err := m.evaluateRevisionChanges(ctx, repoClient, app, &source, i, repo, revision, refSources, syncedRefSources, noRevisionCache, appLabelKey, serverVersion, apiVersions, trackingMethod, installationID, keyManifestGenerateAnnotationExists, keyManifestGenerateAnnotationVal)
-			updateRevisionResult, err := repoClient.UpdateRevisionForPaths(ctx, &apiclient.UpdateRevisionForPathsRequest{
+		if err != nil {
-				Repo:               repo,
+			return nil, nil, false, fmt.Errorf("failed to evaluate revision changes for source %d of %d: %w", i+1, len(sources), err)
-				Revision:           revision,
+		}
-				SyncedRevision:     syncedRevision,
+		if hasChanges {
-				NoRevisionCache:    noRevisionCache,
-				Paths:              path.GetSourceRefreshPaths(app, source),
-				AppLabelKey:        appLabelKey,
-				AppName:            app.InstanceName(m.namespace),
-				Namespace:          appNamespace,
-				ApplicationSource:  &source,
-				KubeVersion:        serverVersion,
-				ApiVersions:        apiVersions,
-				TrackingMethod:     trackingMethod,
-				RefSources:         refSources,
-				SyncedRefSources:   syncedRefSources,
-				HasMultipleSources: app.Spec.HasMultipleSources(),
-				InstallationID:     installationID,
-			})
-			if err != nil {
-				return nil, nil, false, fmt.Errorf("failed to compare revisions for source %d of %d: %w", i+1, len(sources), err)
-			}
-
-			if updateRevisionResult.Changes {
-				revisionsMayHaveChanges = true
-			}
-
-			// Generate manifests should use same revision as updateRevisionForPaths, because HEAD revision may be different between these two calls
-			if updateRevisionResult.Revision != "" {
-				revision = updateRevisionResult.Revision
-			}
-		} else if !source.IsRef() {
-			// revisionsMayHaveChanges is set to true if at least one revision is not possible to be updated
 			revisionsMayHaveChanges = true
 		}
+		revision = resolvedRevision
 
 		repos := permittedHelmRepos
 		helmRepoCreds := permittedHelmCredentials
@@ -358,6 +322,86 @@ func (m *appStateManager) GetRepoObjs(ctx context.Context, app *v1alpha1.Applica
 	return targetObjs, manifestInfos, revisionsMayHaveChanges, nil
 }
 
+// evaluateRevisionChanges determines if a source revision has changes compared to the synced revision.
+// Returns the resolved revision, whether changes were detected, and any error.
+func (m *appStateManager) evaluateRevisionChanges(
+	ctx context.Context,
+	repoClient apiclient.RepoServerServiceClient,
+	app *v1alpha1.Application,
+	source *v1alpha1.ApplicationSource,
+	sourceIndex int,
+	repo *v1alpha1.Repository,
+	revision string,
+	refSources map[string]*v1alpha1.RefTarget,
+	syncedRefSources v1alpha1.RefTargetRevisionMapping,
+	noRevisionCache bool,
+	appLabelKey string,
+	serverVersion string,
+	apiVersions []string,
+	trackingMethod string,
+	installationID string,
+	keyManifestGenerateAnnotationExists bool,
+	keyManifestGenerateAnnotationVal string,
+) (string, bool, error) {
+	// For ref source specifically, we always return false since their change are evaluated as part of the source
+	// referencing them.
+	if source.IsRef() {
+		return revision, false, nil
+	}
+
+	// Determine the synced revision and source type for this specific source
+	var syncedRevision string
+	if app.Spec.HasMultipleSources() {
+		if sourceIndex < len(app.Status.Sync.Revisions) {
+			syncedRevision = app.Status.Sync.Revisions[sourceIndex]
+		}
+	} else {
+		syncedRevision = app.Status.Sync.Revision
+	}
+
+	// if revisions are the same (and we are not using reference sources), we know there is no changes
+	if syncedRevision == revision && revision != "" && len(refSources) == 0 {
+		return revision, false, nil
+	}
+
+	appNamespace := app.Spec.Destination.Namespace
+
+	if repo.Depth == 0 && syncedRevision != "" && keyManifestGenerateAnnotationExists && keyManifestGenerateAnnotationVal != "" {
+		// Validate the manifest-generate-path annotation to avoid generating manifests if it has not changed.
+		updateRevisionResult, err := repoClient.UpdateRevisionForPaths(ctx, &apiclient.UpdateRevisionForPathsRequest{
+			Repo:               repo,
+			Revision:           revision,
+			SyncedRevision:     syncedRevision,
+			NoRevisionCache:    noRevisionCache,
+			Paths:              path.GetSourceRefreshPaths(app, *source),
+			AppLabelKey:        appLabelKey,
+			AppName:            app.InstanceName(m.namespace),
+			Namespace:          appNamespace,
+			ApplicationSource:  source,
+			KubeVersion:        serverVersion,
+			ApiVersions:        apiVersions,
+			TrackingMethod:     trackingMethod,
+			RefSources:         refSources,
+			SyncedRefSources:   syncedRefSources,
+			HasMultipleSources: app.Spec.HasMultipleSources(),
+			InstallationID:     installationID,
+		})
+		if err != nil {
+			return "", false, err
+		}
+
+		// Generate manifests should use same revision as updateRevisionForPaths, because HEAD revision may be different between these two calls
+		if updateRevisionResult.Revision != "" {
+			revision = updateRevisionResult.Revision
+		}
+
+		return revision, updateRevisionResult.Changes, nil
+	}
+
+	// revisionsMayHaveChanges is set to true if at least one revision is not possible to be updated
+	return revision, true, nil
+}
+
 func unmarshalManifests(manifests []string) ([]*unstructured.Unstructured, error) {
 	targetObjs := make([]*unstructured.Unstructured, 0)
 	for _, manifest := range manifests {

controller/state_test.go

@@ -1,6 +1,7 @@
 package controller
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"os"
@@ -31,6 +32,7 @@ import (
 	"github.com/argoproj/argo-cd/v3/controller/testdata"
 	"github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
 	"github.com/argoproj/argo-cd/v3/reposerver/apiclient"
+	"github.com/argoproj/argo-cd/v3/reposerver/apiclient/mocks"
 	"github.com/argoproj/argo-cd/v3/test"
 )
 
@@ -2163,3 +2165,190 @@ func Test_isObjRequiresDeletionConfirmation(t *testing.T) {
 		})
 	}
 }
+
+func Test_evaluateRevisionChanges(t *testing.T) {
+	tests := []struct {
+		name                                string
+		source                              *v1alpha1.ApplicationSource
+		sourceType                          v1alpha1.ApplicationSourceType
+		syncPolicy                          *v1alpha1.SyncPolicy
+		revision                            string
+		appSyncedRevision                   string
+		refSources                          map[string]*v1alpha1.RefTarget
+		repoDepth                           int64
+		keyManifestGenerateAnnotationExists bool
+		keyManifestGenerateAnnotationVal    string
+		updateRevisionForPathsResponse      *apiclient.UpdateRevisionForPathsResponse
+		expectedRevision                    string
+		expectedHasChanges                  bool
+		expectUpdateRevisionForPathsCalled  bool
+	}{
+		{
+			name: "Ref source returns early with no changes",
+			source: &v1alpha1.ApplicationSource{
+				RepoURL: "https://github.com/example/repo",
+				Ref:     "main",
+			},
+			sourceType:         v1alpha1.ApplicationSourceTypeHelm,
+			revision:           "abc123",
+			appSyncedRevision:  "def456",
+			expectedRevision:   "abc123",
+			expectedHasChanges: false,
+		},
+		{
+			name: "Same revision with no ref sources returns early",
+			source: &v1alpha1.ApplicationSource{
+				RepoURL: "https://github.com/example/repo",
+				Path:    "manifests",
+			},
+			sourceType:         v1alpha1.ApplicationSourceTypeKustomize,
+			revision:           "abc123",
+			appSyncedRevision:  "abc123",
+			refSources:         map[string]*v1alpha1.RefTarget{},
+			expectedRevision:   "abc123",
+			expectedHasChanges: false,
+		},
+		{
+			name: "Same revision with ref sources continues to evaluation",
+			source: &v1alpha1.ApplicationSource{
+				RepoURL: "https://github.com/example/repo",
+				Path:    "manifests",
+			},
+			sourceType:        v1alpha1.ApplicationSourceTypeKustomize,
+			revision:          "abc123",
+			appSyncedRevision: "abc123",
+			refSources: map[string]*v1alpha1.RefTarget{
+				"ref1": {Repo: v1alpha1.Repository{Repo: "https://github.com/example/ref"}},
+			},
+			repoDepth:                           0,
+			keyManifestGenerateAnnotationExists: true,
+			keyManifestGenerateAnnotationVal:    ".",
+			updateRevisionForPathsResponse: &apiclient.UpdateRevisionForPathsResponse{
+				Revision: "abc123",
+				Changes:  false,
+			},
+			expectedRevision:                   "abc123",
+			expectedHasChanges:                 false,
+			expectUpdateRevisionForPathsCalled: true,
+		},
+		{
+			name: "Shallow clone skips UpdateRevisionForPaths",
+			source: &v1alpha1.ApplicationSource{
+				RepoURL: "https://github.com/example/repo",
+				Path:    "manifests",
+			},
+			sourceType: v1alpha1.ApplicationSourceTypeKustomize,
+			syncPolicy: &v1alpha1.SyncPolicy{
+				Automated: &v1alpha1.SyncPolicyAutomated{},
+			},
+			revision:                            "abc123",
+			appSyncedRevision:                   "def456",
+			repoDepth:                           1,
+			keyManifestGenerateAnnotationExists: true,
+			keyManifestGenerateAnnotationVal:    ".",
+			expectedRevision:                    "abc123",
+			expectedHasChanges:                  true,
+			expectUpdateRevisionForPathsCalled:  false,
+		},
+		{
+			name: "Missing annotation skips UpdateRevisionForPaths",
+			source: &v1alpha1.ApplicationSource{
+				RepoURL: "https://github.com/example/repo",
+				Path:    "manifests",
+			},
+			sourceType: v1alpha1.ApplicationSourceTypeKustomize,
+			syncPolicy: &v1alpha1.SyncPolicy{
+				Automated: &v1alpha1.SyncPolicyAutomated{},
+			},
+			revision:                            "abc123",
+			appSyncedRevision:                   "def456",
+			repoDepth:                           0,
+			keyManifestGenerateAnnotationExists: false,
+			keyManifestGenerateAnnotationVal:    "",
+			expectedRevision:                    "abc123",
+			expectedHasChanges:                  true,
+			expectUpdateRevisionForPathsCalled:  false,
+		},
+		{
+			name: "UpdateRevisionForPaths returns updated revision",
+			source: &v1alpha1.ApplicationSource{
+				RepoURL: "https://github.com/example/repo",
+				Path:    "manifests",
+			},
+			sourceType: v1alpha1.ApplicationSourceTypeKustomize,
+			syncPolicy: &v1alpha1.SyncPolicy{
+				Automated: &v1alpha1.SyncPolicyAutomated{},
+			},
+			revision:                            "HEAD",
+			appSyncedRevision:                   "def456",
+			repoDepth:                           0,
+			keyManifestGenerateAnnotationExists: true,
+			keyManifestGenerateAnnotationVal:    ".",
+			updateRevisionForPathsResponse: &apiclient.UpdateRevisionForPathsResponse{
+				Revision: "abc123resolved",
+				Changes:  true,
+			},
+			expectedRevision:                   "abc123resolved",
+			expectedHasChanges:                 true,
+			expectUpdateRevisionForPathsCalled: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			app := newFakeApp()
+			app.Spec.SyncPolicy = tt.syncPolicy
+			app.Status.Sync.Revision = tt.appSyncedRevision
+			app.Status.SourceType = tt.sourceType
+			if tt.keyManifestGenerateAnnotationExists {
+				app.Annotations = map[string]string{
+					v1alpha1.AnnotationKeyManifestGeneratePaths: tt.keyManifestGenerateAnnotationVal,
+				}
+			}
+
+			repo := &v1alpha1.Repository{
+				Repo:  tt.source.RepoURL,
+				Depth: tt.repoDepth,
+			}
+
+			mockRepoClient := &mocks.RepoServerServiceClient{}
+			if tt.expectUpdateRevisionForPathsCalled {
+				mockRepoClient.On("UpdateRevisionForPaths", mock.Anything, mock.Anything).Return(tt.updateRevisionForPathsResponse, nil)
+			}
+
+			mgr := &appStateManager{
+				namespace: "test-namespace",
+			}
+
+			resolvedRevision, hasChanges, err := mgr.evaluateRevisionChanges(
+				context.Background(),
+				mockRepoClient,
+				app,
+				tt.source,
+				0, // sourceIndex
+				repo,
+				tt.revision,
+				tt.refSources,
+				nil,
+				false,
+				"app.kubernetes.io/instance",
+				"v1.28.0",
+				[]string{"v1"},
+				"label",
+				"test-installation",
+				tt.keyManifestGenerateAnnotationExists,
+				tt.keyManifestGenerateAnnotationVal,
+			)
+
+			require.NoError(t, err)
+			assert.Equal(t, tt.expectedRevision, resolvedRevision)
+			assert.Equal(t, tt.expectedHasChanges, hasChanges)
+
+			if tt.expectUpdateRevisionForPathsCalled {
+				mockRepoClient.AssertExpectations(t)
+			} else {
+				mockRepoClient.AssertNotCalled(t, "UpdateRevisionForPaths")
+			}
+		})
+	}
+}