diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 0b7de5b18defb..ac5b43014bcbf 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -2097,7 +2097,7 @@ func (ctrl *ApplicationController) autoSync(app *appv1.Application, syncStatus * } // alreadyAttemptedSync returns whether the most recent sync was performed against the -// commitSHA and with the same app source config which are currently set in the app +// commitSHA and with the same app source config which are currently set in the app. func alreadyAttemptedSync(app *appv1.Application, commitSHA string, commitSHAsMS []string, hasMultipleSources bool, revisionUpdated bool) (bool, synccommon.OperationPhase) { if app.Status.OperationState == nil || app.Status.OperationState.Operation.Sync == nil || app.Status.OperationState.SyncResult == nil { return false, "" diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 6e965fc8f0972..6adb5cc02d2fa 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -2430,42 +2430,70 @@ func TestAlreadyAttemptSync(t *testing.T) { assert.False(t, attempted) }) - t.Run("same manifest with sync result", func(t *testing.T) { - attempted, _ := alreadyAttemptedSync(app, "sha", []string{}, false, false) - assert.True(t, attempted) - }) + t.Run("single source", func(t *testing.T) { + t.Run("same manifest with sync result", func(t *testing.T) { + attempted, _ := alreadyAttemptedSync(app, "sha", []string{}, false, false) + assert.True(t, attempted) + }) - t.Run("different manifest with sync result, different SHA", func(t *testing.T) { - app := app.DeepCopy() - app.Status.OperationState.SyncResult.Revision = "sha1" - attempted, _ := alreadyAttemptedSync(app, "sha2", []string{}, false, true) - assert.False(t, attempted) - }) + t.Run("same manifest with sync result different targetRevision, same SHA", func(t *testing.T) { + // This test represents the case where the user changed a source's target revision to a new branch, but it + // points to the same revision as the old branch. We currently do not consider this as having been "already + // attempted." In the future we may want to short-circuit the auto-sync in these cases. + app := app.DeepCopy() + app.Status.OperationState.SyncResult.Source = v1alpha1.ApplicationSource{TargetRevision: "branch1"} + app.Spec.Source = &v1alpha1.ApplicationSource{TargetRevision: "branch2"} + app.Status.OperationState.SyncResult.Revision = "sha" + attempted, _ := alreadyAttemptedSync(app, "sha", []string{}, false, false) + assert.False(t, attempted) + }) - t.Run("different manifest with sync result, same SHA", func(t *testing.T) { - app := app.DeepCopy() - app.Status.OperationState.SyncResult.Revision = "sha" - attempted, _ := alreadyAttemptedSync(app, "sha", []string{}, false, true) - assert.True(t, attempted) - }) + t.Run("different manifest with sync result, different SHA", func(t *testing.T) { + app := app.DeepCopy() + app.Status.OperationState.SyncResult.Revision = "sha1" + attempted, _ := alreadyAttemptedSync(app, "sha2", []string{}, false, true) + assert.False(t, attempted) + }) - t.Run("same manifest with sync result", func(t *testing.T) { - attempted, _ := alreadyAttemptedSync(app, "", []string{"sha"}, true, false) - assert.True(t, attempted) + t.Run("different manifest with sync result, same SHA", func(t *testing.T) { + app := app.DeepCopy() + app.Status.OperationState.SyncResult.Revision = "sha" + attempted, _ := alreadyAttemptedSync(app, "sha", []string{}, false, true) + assert.True(t, attempted) + }) }) - t.Run("different manifest with sync result, different SHAs", func(t *testing.T) { - app := app.DeepCopy() - app.Status.OperationState.SyncResult.Revisions = []string{"sha_a_=", "sha_b_1"} - attempted, _ := alreadyAttemptedSync(app, "", []string{"sha_a_2", "sha_b_2"}, true, true) - assert.False(t, attempted) - }) + t.Run("multi-source", func(t *testing.T) { + t.Run("same manifest with sync result", func(t *testing.T) { + attempted, _ := alreadyAttemptedSync(app, "", []string{"sha"}, true, false) + assert.True(t, attempted) + }) - t.Run("different manifest with sync result, same SHAs", func(t *testing.T) { - app := app.DeepCopy() - app.Status.OperationState.SyncResult.Revisions = []string{"sha_a", "sha_b"} - attempted, _ := alreadyAttemptedSync(app, "", []string{"sha_a", "sha_b"}, true, true) - assert.True(t, attempted) + t.Run("same manifest with sync result, different targetRevision, same SHA", func(t *testing.T) { + // This test represents the case where the user changed a source's target revision to a new branch, but it + // points to the same revision as the old branch. We currently do not consider this as having been "already + // attempted." In the future we may want to short-circuit the auto-sync in these cases. + app := app.DeepCopy() + app.Status.OperationState.SyncResult.Sources = []v1alpha1.ApplicationSource{{TargetRevision: "branch1"}} + app.Spec.Sources = []v1alpha1.ApplicationSource{{TargetRevision: "branch2"}} + app.Status.OperationState.SyncResult.Revisions = []string{"sha"} + attempted, _ := alreadyAttemptedSync(app, "", []string{"sha"}, true, false) + assert.False(t, attempted) + }) + + t.Run("different manifest with sync result, different SHAs", func(t *testing.T) { + app := app.DeepCopy() + app.Status.OperationState.SyncResult.Revisions = []string{"sha_a_=", "sha_b_1"} + attempted, _ := alreadyAttemptedSync(app, "", []string{"sha_a_2", "sha_b_2"}, true, true) + assert.False(t, attempted) + }) + + t.Run("different manifest with sync result, same SHAs", func(t *testing.T) { + app := app.DeepCopy() + app.Status.OperationState.SyncResult.Revisions = []string{"sha_a", "sha_b"} + attempted, _ := alreadyAttemptedSync(app, "", []string{"sha_a", "sha_b"}, true, true) + assert.True(t, attempted) + }) }) } diff --git a/reposerver/apiclient/repository.pb.go b/reposerver/apiclient/repository.pb.go index 1e4083d989769..34adb3bc07fdc 100644 --- a/reposerver/apiclient/repository.pb.go +++ b/reposerver/apiclient/repository.pb.go @@ -2371,6 +2371,9 @@ func (m *UpdateRevisionForPathsRequest) GetInstallationID() string { } type UpdateRevisionForPathsResponse struct { + // Changes indicates whether any changes were detected in the provided paths. If false, it means that the manifest + // cache was updated to the new revision. If true, it means that there are relevant changes in the repo files and + // that new manifests should be generated. Changes bool `protobuf:"varint,1,opt,name=changes,proto3" json:"changes,omitempty"` Revision string `protobuf:"bytes,2,opt,name=revision,proto3" json:"revision,omitempty"` XXX_NoUnkeyedLiteral struct{} `json:"-"` diff --git a/reposerver/repository/repository.proto b/reposerver/repository/repository.proto index 2f45007d0e884..784ef0cafa12b 100644 --- a/reposerver/repository/repository.proto +++ b/reposerver/repository/repository.proto @@ -289,6 +289,9 @@ message UpdateRevisionForPathsRequest { } message UpdateRevisionForPathsResponse { + // Changes indicates whether any changes were detected in the provided paths. If false, it means that the manifest + // cache was updated to the new revision. If true, it means that there are relevant changes in the repo files and + // that new manifests should be generated. bool changes = 1; string revision = 2; }