diff --git a/internal/argocd/health.go b/internal/argocd/health.go index a03085255..150a25914 100644 --- a/internal/argocd/health.go +++ b/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) } diff --git a/internal/argocd/health_test.go b/internal/argocd/health_test.go index 1c3ae3ea4..85c0abee9 100644 --- a/internal/argocd/health_test.go +++ b/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) { diff --git a/internal/argocd/revision.go b/internal/argocd/revision.go index 49e4c7f40..9e3ab894f 100644 --- a/internal/argocd/revision.go +++ b/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, diff --git a/internal/argocd/revision_test.go b/internal/argocd/revision_test.go index 35cdcf7ce..a4eb91ccb 100644 --- a/internal/argocd/revision_test.go +++ b/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) }) } } diff --git a/internal/controller/argocd/api/v1alpha1/application_types.go b/internal/controller/argocd/api/v1alpha1/application_types.go index eb8499076..151e5857e 100644 --- a/internal/controller/argocd/api/v1alpha1/application_types.go +++ b/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 { diff --git a/internal/controller/freight/origins_test.go b/internal/controller/freight/origins_test.go index 95f696a7b..b10f28709 100644 --- a/internal/controller/freight/origins_test.go +++ b/internal/controller/freight/origins_test.go @@ -14,10 +14,6 @@ func TestGetDesiredOrigin(t *testing.T) { Kind: "Foo", Name: "origin1", } - testOrigin2 := &kargoapi.FreightOrigin{ - Kind: "Foo", - Name: "origin2", - } testCases := []struct { name string setup func() (any, any) @@ -220,54 +216,6 @@ func TestGetDesiredOrigin(t *testing.T) { return m, &m.ArgoCDAppUpdates[0] }, }, - { - name: "ArgoCDAppUpdate for multi-source app with different origins for various sources are correctly identified", - setup: func() (any, any) { - stage := &kargoapi.Stage{ - Spec: kargoapi.StageSpec{ - PromotionMechanisms: &kargoapi.PromotionMechanisms{ - ArgoCDAppUpdates: []kargoapi.ArgoCDAppUpdate{ - { - SourceUpdates: []kargoapi.ArgoCDSourceUpdate{ - {Origin: testOrigin, RepoURL: "https://github.com/universe/42"}, - {Origin: testOrigin2, RepoURL: "https://github.com/another-universe/42"}, - }, - }, - }, - }, - }, - Status: kargoapi.StageStatus{ - FreightHistory: kargoapi.FreightHistory{ - &kargoapi.FreightCollection{ - Freight: map[string]kargoapi.FreightReference{ - testOrigin.String(): { - Origin: *testOrigin, - Commits: []kargoapi.GitCommit{ - { - RepoURL: "https://github.com/universe/42", - ID: "fake-revision", - }, - }, - }, - testOrigin2.String(): { - Origin: *testOrigin2, - Commits: []kargoapi.GitCommit{ - { - RepoURL: "https://github.com/another-universe/42", - ID: "another-revision", - }, - }, - }, - }, - }, - }, - }, - } - - return stage, &stage.Spec.PromotionMechanisms.ArgoCDAppUpdates[0].SourceUpdates[1] - }, - expectedOrigin: testOrigin2, - }, { name: "ArgoCDSourceUpdate can inherit from ArgoCDAppUpdate", setup: func() (any, any) { diff --git a/internal/controller/promotion/argocd.go b/internal/controller/promotion/argocd.go index 2d51f7447..9b5bb1e26 100644 --- a/internal/controller/promotion/argocd.go +++ b/internal/controller/promotion/argocd.go @@ -356,7 +356,7 @@ func (a *argoCDMechanism) mustPerformUpdate( } } - // Check if the desired revision was applied. + // Check if the desired revisions were applied. desiredRevisions, err := libargocd.GetDesiredRevisions( ctx, a.kargoClient, @@ -372,14 +372,14 @@ func (a *argoCDMechanism) mustPerformUpdate( return "", true, fmt.Errorf("error determining desired revision: %w", err) } - if desiredRevisions == nil { + if len(desiredRevisions) == 0 { // We do not have a desired revision, so we cannot determine if the // operation was successful. return status.Phase, false, nil } // Check multi-source app for matching revisions. - if app.IsMultisource() { + if len(app.Spec.Sources) > 0 { syncedRevisions := status.SyncResult.Revisions for i, r := range syncedRevisions { if r != desiredRevisions[i] { @@ -653,8 +653,7 @@ func (a *argoCDMechanism) applyArgoCDSourceUpdate( updateLogger.Debug("About to apply ArgoCD source update") if source.Chart != "" || update.Chart != "" { - // Kargo uses the "oci://" prefix, but Argo CD does not. - if source.RepoURL != strings.TrimPrefix(update.RepoURL, "oci://") || source.Chart != update.Chart { + if source.RepoURL != update.RepoURL || source.Chart != update.Chart { updateLogger.Trace("Source update not applicable to Application source, skipping.") return source, nil }