more tests, some docs

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

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, ""

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)
+		})
 	})
 }
 

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;
 }