From 032d9e1e8057c4f6f1c2bbcb60e9ee61ae3474a3 Mon Sep 17 00:00:00 2001 From: Alexandre Gaudreault Date: Mon, 20 Apr 2026 10:03:55 -0400 Subject: [PATCH] refactor: simplify UpdateRevisionForPaths logic and add early returns (#27190) Signed-off-by: Alexandre Gaudreault --- controller/state.go | 130 ++++++++++++++++++--------- controller/state_test.go | 189 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 276 insertions(+), 43 deletions(-) diff --git a/controller/state.go b/controller/state.go index 331d784097..1d6df2dbe2 100644 --- a/controller/state.go +++ b/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()) { - // 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 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 + // Evaluate if the revision has changes + resolvedRevision, hasChanges, err := m.evaluateRevisionChanges(ctx, repoClient, app, &source, i, repo, revision, refSources, syncedRefSources, noRevisionCache, appLabelKey, serverVersion, apiVersions, trackingMethod, installationID, keyManifestGenerateAnnotationExists, keyManifestGenerateAnnotationVal) + if err != nil { + return nil, nil, false, fmt.Errorf("failed to evaluate revision changes for source %d of %d: %w", i+1, len(sources), err) + } + if hasChanges { 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 { diff --git a/controller/state_test.go b/controller/state_test.go index 013689b016..ee3af91eb6 100644 --- a/controller/state_test.go +++ b/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") + } + }) + } +}