Address PR asks

Signed-off-by: Gyorgy Nadaban <gyorgy.nadaban@gmail.com>

internal/argocd/health.go

@@ -255,8 +255,17 @@ func stageHealthForAppSync(
 	revisions []string) (kargoapi.HealthState, error) {
 	logger := logging.LoggerFromContext(ctx).WithValues("appName", app.GetName(), "revisions", revisions)
 	logger.Debug("About to determine stage health based on app sync status.")
+
+	allRevisionsEmpty := true
+	for _, r := range revisions {
+		if r != "" {
+			allRevisionsEmpty = false
+			break
+		}
+	}
+
 	switch {
-	case revisions == nil || (len(revisions) == 1 && revisions[0] == ""):
+	case allRevisionsEmpty:
 		logger.Debug("Desired revision not set, assuming healthy.")
 		return kargoapi.HealthStateHealthy, nil
 	case app.Operation != nil && app.Operation.Sync != nil,
@@ -274,7 +283,7 @@ func stageHealthForAppSync(
 
 		// Trivial case where app has only a single source and revision is set.
 		singleSourceRevision := app.Status.Sync.Revision
-		if !app.IsMultisource() {
+		if len(app.Spec.Sources) == 0 {
 			if len(revisions) == 1 && revisions[0] == singleSourceRevision {
 				return kargoapi.HealthStateHealthy, nil
 			}
@@ -294,10 +303,9 @@ func stageHealthForAppSync(
 		multiSourceRevisions := app.Status.Sync.Revisions
 		// Apps with multiple sources pointed at the same Git repository can only have the same revision
 		// for all sources because ArgoCD does not support the alternative.
-		// We follow ArgoCD shadow-array implementation here that preserves the order of app.spec.sources for
-		// the revisions.
-
-		misaligned_sources := make([]string, 0)
+		// We follow ArgoCD implementation here, where each item in the revisions array corresponds the
+		// app.spec.sources at the same index position.
+		misalignedSources := make([]string, 0)
 		for i, r := range multiSourceRevisions {
 
 			// An empty desired revision means we are not managing the corresponding source.
@@ -316,15 +324,15 @@ func stageHealthForAppSync(
 					app.GetNamespace(),
 					revisions[i],
 				)
-				misaligned_sources = append(misaligned_sources, msg)
+				misalignedSources = append(misalignedSources, msg)
 				logger.Debug(msg)
 			}
 		}
 
-		if len(misaligned_sources) > 0 {
+		if len(misalignedSources) > 0 {
 			msg := fmt.Sprintf("Not all sources of Application %q in namespace %q "+
 				"match the desired revisions, assuming unhealthy. Issues: %s",
-				app.GetName(), app.GetNamespace(), strings.Join(misaligned_sources[:], " "))
+				app.GetName(), app.GetNamespace(), strings.Join(misalignedSources[:], " "))
 			return kargoapi.HealthStateUnhealthy, errors.New(msg)
 		}
 

internal/argocd/health_test.go

@@ -991,8 +991,8 @@ func Test_stageHealthForAppSync(t *testing.T) {
 					Namespace: "fake-namespace",
 					Name:      "fake-name",
 				},
-				Operation: &argocd.Operation{
-					Sync: &argocd.SyncOperation{},
+				Status: argocd.ApplicationStatus{
+					Sync: argocd.SyncStatus{},
 				},
 			},
 			assertions: func(t *testing.T, state kargoapi.HealthState, err error) {

internal/argocd/revision.go

@@ -14,9 +14,10 @@ import (
 	"github.com/akuity/kargo/internal/logging"
 )
 
-// GetDesiredRevision returns the desired revision for the given
-// v1alpha1.Application. If that cannot be determined, an empty string is
-// returned.
+// GetDesiredRevision returns the desired revisions for the given
+// v1alpha1.Application. The array indices shadow the application's
+// sources. If desired revision for a source cannot be determined,
+// empty string is returned at the corresponding index.
 func GetDesiredRevisions(
 	ctx context.Context,
 	cl client.Client,
@@ -33,7 +34,7 @@ func GetDesiredRevisions(
 		return nil, nil
 	}
 
-	appLogger := logger.WithValues("application", app.Name, "namespace", app.Namespace)
+	appLogger := logger.WithValues("appName", app.Name, "namespace", app.Namespace)
 	appLogger.Debug("Getting desired revision for application", "app", app, "spec", app.Spec)
 
 	// Note that frght was provided as an argument instead of being plucked
@@ -42,76 +43,47 @@ func GetDesiredRevisions(
 	// a health check (current freight) or in the context of a promotion (new
 	// freight).
 
-	// An application may have one or more sources.
-	if !app.IsMultisource() {
-		s := app.Spec.Source
-		if s == nil {
-			err := errors.New("Single-source application source is nil")
-			appLogger.Error(err, "Cannot determine desired revision for application, bailing.")
-			return nil, nil
-		}
-
-		appLogger.Debug("Application source is not nil, checking.", "source", s)
-
-		// If there is a source update that targets app.Spec.Source, it might
-		// have its own ideas about the desired revision.
-		targetUpdate := getTargetUpdate(ctx, update, s)
-		desiredOrigin := freight.GetDesiredOrigin(ctx, stage, targetUpdate)
-		if desiredOrigin == nil {
-			appLogger.WithValues("source", s,
-				"revision", app.Status.Sync.Revision).Debug("Could not determine desired origin " +
-				"for application source, using existing revision.")
-			return []string{app.Status.Sync.Revision}, nil
-		}
-
-		desiredRevision, err := getRevisionFromSource(ctx, cl, stage, s, desiredOrigin, frght)
-
-		if err != nil {
-			return nil, err
-		}
-
-		if desiredRevision != "" {
-			appLogger.WithValues("source", s,
-				"revision", desiredRevision).Debug("Found desired revision for application source.")
-			return []string{desiredRevision}, nil
-		}
-
-		appLogger.WithValues("source", s).Debug("Could not determine desired revision for application from this source.")
+	hasSource := app.Spec.Source != nil
+	hasSources := app.Spec.Sources != nil
+	if !hasSource && !hasSources {
+		appLogger.Debug("Application has no sources, cannot determine desired revision.")
 		return nil, nil
 	}
 
-	// An application may have more than one source that points to the same Git repository,
-	// eg. a Helm and a vanilla manifest source.
-	// In that situation it doesn't matter which source's target revision is returned as ArgoCD does not support
-	// different target revisions for sources targeting the same repository.
-	var revisions = make([]string, len(app.Spec.Sources))
-	isSynced := app.Status.Sync.Revisions != nil
+	sources := app.Spec.Sources
+	if hasSource {
+		sources = []argocd.ApplicationSource{*app.Spec.Source}
+	}
+
+	var revisions = make([]string, len(sources))
 
-	// ArgoCD uses a shadow-array to store the corresponding revisions preserving the order of app.spec.sources.
+	// ArgoCD revisions array items correspond to the app.spec.sources at the same index location.
 	// We match that logic here and return the desired revision for each source,
 	// or empty string if one is not found.
-	for i, src := range app.Spec.Sources {
+	for i, src := range sources {
 		s := src
 
 		syncedRevisionOfSource := ""
-		if isSynced && i < len(app.Status.Sync.Revisions) {
+		if app.Status.Sync.Revisions != nil && i < len(app.Status.Sync.Revisions) {
 			syncedRevisionOfSource = app.Status.Sync.Revisions[i]
+		} else {
+			syncedRevisionOfSource = app.Status.Sync.Revision
 		}
 
-		// If there is a source update that targets app.Spec.Source, it might
+		// If there is a source update that targets the current source, it might
 		// have its own ideas about the desired revision.
 		targetUpdate := getTargetUpdate(ctx, update, &s)
 		desiredOrigin := freight.GetDesiredOrigin(ctx, stage, targetUpdate)
 		if desiredOrigin == nil {
 			appLogger.WithValues("source", s,
 				"revision", syncedRevisionOfSource).Debug("Could not determine desired origin" +
-				" for application source, using existing revision.")
-			revisions[i] = syncedRevisionOfSource
+				" for application source.")
+			revisions[i] = ""
 			continue
 		}
 
 		logger.Debug("Resolved origin for application source", "origin", desiredOrigin)
-		desiredRevision, err := getRevisionFromSource(ctx, cl, stage, &s, desiredOrigin, frght)
+		desiredRevision, err := getDesiredRevisionForSource(ctx, cl, stage, &s, desiredOrigin, frght)
 
 		if err != nil {
 			return nil, err
@@ -127,7 +99,7 @@ func GetDesiredRevisions(
 	return revisions, nil
 }
 
-func getRevisionFromSource(
+func getDesiredRevisionForSource(
 	ctx context.Context,
 	cl client.Client,
 	stage *kargoapi.Stage,

internal/argocd/revision_test.go

@@ -23,22 +23,27 @@ func TestGetDesiredRevisions(t *testing.T) {
 		Name: "another-warehouse",
 	}
 
-	var no_revisions []string
 	testCases := []struct {
 		name           string
 		app            *argocdapi.Application
 		stage          *kargoapi.Stage
 		freightHistory kargoapi.FreightHistory
-		want           []string
+		assertions     func(*testing.T, []string, error)
 	}{
 		{
 			name: "no application",
-			want: no_revisions,
+			assertions: func(t *testing.T, result []string, err error) {
+				require.NoError(t, err)
+				require.Empty(t, result)
+			},
 		},
 		{
 			name: "no application source",
 			app:  &argocdapi.Application{},
-			want: no_revisions,
+			assertions: func(t *testing.T, result []string, err error) {
+				require.NoError(t, err)
+				require.Empty(t, result)
+			},
 		},
 		{
 			name: "no source repo URL",
@@ -47,7 +52,10 @@ func TestGetDesiredRevisions(t *testing.T) {
 					Source: &argocdapi.ApplicationSource{},
 				},
 			},
-			want: no_revisions,
+			assertions: func(t *testing.T, result []string, err error) {
+				require.NoError(t, err)
+				require.Equal(t, result, []string{""})
+			},
 		},
 		{
 			name: "chart source",
@@ -80,7 +88,10 @@ func TestGetDesiredRevisions(t *testing.T) {
 					},
 				},
 			},
-			want: []string{"v2.0.0"},
+			assertions: func(t *testing.T, result []string, err error) {
+				require.NoError(t, err)
+				require.Equal(t, result, []string{"v2.0.0"})
+			},
 		},
 		{
 			name: "git source",
@@ -110,7 +121,10 @@ func TestGetDesiredRevisions(t *testing.T) {
 					},
 				},
 			},
-			want: []string{"fake-revision"},
+			assertions: func(t *testing.T, result []string, err error) {
+				require.NoError(t, err)
+				require.Equal(t, result, []string{"fake-revision"})
+			},
 		},
 		{
 			name: "git multisource with chart",
@@ -152,7 +166,10 @@ func TestGetDesiredRevisions(t *testing.T) {
 					},
 				},
 			},
-			want: []string{"", "fake-revision"},
+			assertions: func(t *testing.T, result []string, err error) {
+				require.NoError(t, err)
+				require.Equal(t, result, []string{"", "fake-revision"})
+			},
 		},
 		{
 			name: "git multisource with chart without synced revisions",
@@ -193,7 +210,10 @@ func TestGetDesiredRevisions(t *testing.T) {
 					},
 				},
 			},
-			want: []string{"", "fake-revision"},
+			assertions: func(t *testing.T, result []string, err error) {
+				require.NoError(t, err)
+				require.Equal(t, result, []string{"", "fake-revision"})
+			},
 		},
 		{
 			name: "git multisource with multiple freight references",
@@ -264,7 +284,10 @@ func TestGetDesiredRevisions(t *testing.T) {
 					},
 				},
 			},
-			want: []string{"", "fake-revision", "another-revision"},
+			assertions: func(t *testing.T, result []string, err error) {
+				require.NoError(t, err)
+				require.Equal(t, result, []string{"", "fake-revision", "another-revision"})
+			},
 		},
 		{
 			name: "git source with health check commit",
@@ -291,7 +314,10 @@ func TestGetDesiredRevisions(t *testing.T) {
 					},
 				},
 			},
-			want: []string{"fake-revision"},
+			assertions: func(t *testing.T, result []string, err error) {
+				require.NoError(t, err)
+				require.Equal(t, result, []string{"fake-revision"})
+			},
 		},
 	}
 
@@ -325,7 +351,7 @@ func TestGetDesiredRevisions(t *testing.T) {
 				stage.Status.FreightHistory.Current().References(),
 			)
 			require.NoError(t, err)
-			require.Equal(t, testCase.want, revisions)
+			testCase.assertions(t, revisions, err)
 		})
 	}
 }

internal/controller/argocd/api/v1alpha1/application_types.go

@@ -33,10 +33,6 @@ type ApplicationSource struct {
 	Path           string                      `json:"path,omitempty"`
 }
 
-func (a Application) IsMultisource() bool {
-	return a.Spec.Sources != nil && len(a.Spec.Sources) > 0
-}
-
 // Equals compares two instances of ApplicationSource and returns true if
 // they are equal.
 func (source *ApplicationSource) Equals(other *ApplicationSource) bool {