diff --git a/internal/directives/argocd_updater.go b/internal/directives/argocd_updater.go index b93d71283..f027b4a5a 100644 --- a/internal/directives/argocd_updater.go +++ b/internal/directives/argocd_updater.go @@ -25,7 +25,7 @@ import ( const ( applicationOperationInitiator = "kargo-controller" - freightCollectionInfoKey = "kargo.akuity.io/freight-collection" + promotionInfoKey = "kargo.akuity.io/promotion" ) func init() { @@ -69,7 +69,6 @@ type argocdUpdater struct { *PromotionStepContext, *ArgoCDAppUpdate, *argocd.Application, - argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) syncApplicationFn func( @@ -207,24 +206,8 @@ func (a *argocdUpdater) runPromotionStep( DesiredRevisions: desiredRevisions, } - // Build the desired source(s) for the Argo CD Application. - desiredSources, err := a.buildDesiredSourcesFn( - ctx, - stepCtx, - &stepCfg, - update, - desiredRevisions, - app, - ) - if err != nil { - return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf( - "error building desired sources for Argo CD Application %q in namespace %q: %w", - app.Name, app.Namespace, err, - ) - } - // Check if the update needs to be performed and retrieve its phase. - phase, mustUpdate, err := a.mustPerformUpdateFn(stepCtx, update, app, desiredSources) + phase, mustUpdate, err := a.mustPerformUpdateFn(stepCtx, update, app) // If we have a phase, append it to the results. if phase != "" { @@ -267,6 +250,22 @@ func (a *argocdUpdater) runPromotionStep( logger.Debug(err.Error()) } + // Build the desired source(s) for the Argo CD Application. + desiredSources, err := a.buildDesiredSourcesFn( + ctx, + stepCtx, + &stepCfg, + update, + desiredRevisions, + app, + ) + if err != nil { + return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf( + "error building desired sources for Argo CD Application %q in namespace %q: %w", + app.Name, app.Namespace, err, + ) + } + // Perform the update. if err = a.syncApplicationFn( ctx, @@ -372,7 +371,6 @@ func (a *argocdUpdater) mustPerformUpdate( stepCtx *PromotionStepContext, update *ArgoCDAppUpdate, app *argocd.Application, - desiredSources argocd.ApplicationSources, ) (phase argocd.OperationPhase, mustUpdate bool, err error) { status := app.Status.OperationState if status == nil { @@ -400,23 +398,23 @@ func (a *argocdUpdater) mustPerformUpdate( // Deal with the possibility that the operation was not initiated for the // current freight collection. i.e. Not related to the current promotion. - var correctFreightColIDFound bool + var correctPromotionIDFound bool for _, info := range status.Operation.Info { - if info.Name == freightCollectionInfoKey { - correctFreightColIDFound = info.Value == stepCtx.Freight.ID + if info.Name == promotionInfoKey { + correctPromotionIDFound = info.Value == stepCtx.Promotion break } } - if !correctFreightColIDFound { - // The operation was not initiated for the current freight collection. + if !correctPromotionIDFound { + // The operation was not initiated for the current Promotion. if !status.Phase.Completed() { // We should wait for the operation to complete before attempting to // apply an update ourselves. // NB: We return the current phase here because we want the caller // to know that an operation is still running. return status.Phase, false, fmt.Errorf( - "current operation was not initiated for freight collection %q: waiting for operation to complete", - stepCtx.Freight.ID, + "current operation was not initiated for Promotion %s: waiting for operation to complete", + stepCtx.Promotion, ) } // Initiate our own operation. @@ -434,18 +432,6 @@ func (a *argocdUpdater) mustPerformUpdate( return "", true, errors.New("operation completed without a sync result") } - // Check if the desired sources were applied. - if len(update.Sources) > 0 { - if (status.SyncResult.Source.RepoURL != "" && !status.SyncResult.Source.Equals(&desiredSources[0])) || - (status.SyncResult.Source.RepoURL == "" && !status.SyncResult.Sources.Equals(desiredSources)) { - // The operation did not result in the desired sources being applied. We - // should attempt to retry the operation. - return "", true, fmt.Errorf( - "operation result source does not match desired source", - ) - } - } - // Check if the desired revisions were applied. desiredRevisions, err := a.getDesiredRevisions(stepCtx, update, app) if err != nil { @@ -514,8 +500,8 @@ func (a *argocdUpdater) syncApplication( Value: "Promotion triggered a sync of this Application resource.", }, { - Name: freightCollectionInfoKey, - Value: stepCtx.Freight.ID, + Name: promotionInfoKey, + Value: stepCtx.Promotion, }, }, Sync: &argocd.SyncOperation{ diff --git a/internal/directives/argocd_updater_test.go b/internal/directives/argocd_updater_test.go index f008ef8fe..37a5d30fb 100644 --- a/internal/directives/argocd_updater_test.go +++ b/internal/directives/argocd_updater_test.go @@ -477,7 +477,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { }, }, { - name: "error building desired sources", + name: "error determining if update is necessary", runner: &argocdUpdater{ getAuthorizedApplicationFn: func( context.Context, @@ -486,15 +486,12 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { ) (*v1alpha1.Application, error) { return &argocd.Application{}, nil }, - buildDesiredSourcesFn: func( - context.Context, + mustPerformUpdateFn: func( *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, - []string, *argocd.Application, - ) (argocd.ApplicationSources, error) { - return nil, errors.New("something went wrong") + ) (argocd.OperationPhase, bool, error) { + return "", false, errors.New("something went wrong") }, }, stepCtx: &PromotionStepContext{ @@ -505,12 +502,11 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { }, assertions: func(t *testing.T, res PromotionStepResult, err error) { require.Equal(t, kargoapi.PromotionPhaseErrored, res.Status) - require.ErrorContains(t, err, "error building desired sources for Argo CD Application") require.ErrorContains(t, err, "something went wrong") }, }, { - name: "error determining if update is necessary", + name: "determination error can be solved by applying update", runner: &argocdUpdater{ getAuthorizedApplicationFn: func( context.Context, @@ -533,9 +529,16 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { *PromotionStepContext, *ArgoCDAppUpdate, *argocd.Application, - argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { - return "", false, errors.New("something went wrong") + return "", true, errors.New("something went wrong") + }, + syncApplicationFn: func( + context.Context, + *PromotionStepContext, + *argocd.Application, + argocd.ApplicationSources, + ) error { + return nil }, }, stepCtx: &PromotionStepContext{ @@ -545,12 +548,12 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { Apps: []ArgoCDAppUpdate{{}}, }, assertions: func(t *testing.T, res PromotionStepResult, err error) { - require.Equal(t, kargoapi.PromotionPhaseErrored, res.Status) - require.ErrorContains(t, err, "something went wrong") + require.Equal(t, kargoapi.PromotionPhaseRunning, res.Status) + require.NoError(t, err) }, }, { - name: "determination error can be solved by applying update", + name: "must wait for update to complete", runner: &argocdUpdater{ getAuthorizedApplicationFn: func( context.Context, @@ -559,31 +562,12 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { ) (*v1alpha1.Application, error) { return &argocd.Application{}, nil }, - buildDesiredSourcesFn: func( - context.Context, - *PromotionStepContext, - *ArgoCDUpdateConfig, - *ArgoCDAppUpdate, - []string, - *argocd.Application, - ) (argocd.ApplicationSources, error) { - return []argocd.ApplicationSource{{}}, nil - }, mustPerformUpdateFn: func( *PromotionStepContext, *ArgoCDAppUpdate, *argocd.Application, - argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { - return "", true, errors.New("something went wrong") - }, - syncApplicationFn: func( - context.Context, - *PromotionStepContext, - *argocd.Application, - argocd.ApplicationSources, - ) error { - return nil + return argocd.OperationRunning, false, nil }, }, stepCtx: &PromotionStepContext{ @@ -598,7 +582,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { }, }, { - name: "must wait for update to complete", + name: "must wait for operation from different user to complete", runner: &argocdUpdater{ getAuthorizedApplicationFn: func( context.Context, @@ -607,23 +591,12 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { ) (*v1alpha1.Application, error) { return &argocd.Application{}, nil }, - buildDesiredSourcesFn: func( - context.Context, - *PromotionStepContext, - *ArgoCDUpdateConfig, - *ArgoCDAppUpdate, - []string, - *argocd.Application, - ) (argocd.ApplicationSources, error) { - return []argocd.ApplicationSource{{}}, nil - }, mustPerformUpdateFn: func( *PromotionStepContext, *ArgoCDAppUpdate, *argocd.Application, - argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { - return argocd.OperationRunning, false, nil + return argocd.OperationRunning, false, fmt.Errorf("waiting for operation to complete") }, }, stepCtx: &PromotionStepContext{ @@ -638,7 +611,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { }, }, { - name: "must wait for operation from different user to complete", + name: "error building desired sources", runner: &argocdUpdater{ getAuthorizedApplicationFn: func( context.Context, @@ -647,6 +620,13 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { ) (*v1alpha1.Application, error) { return &argocd.Application{}, nil }, + mustPerformUpdateFn: func( + *PromotionStepContext, + *ArgoCDAppUpdate, + *argocd.Application, + ) (argocd.OperationPhase, bool, error) { + return "", true, nil + }, buildDesiredSourcesFn: func( context.Context, *PromotionStepContext, @@ -655,15 +635,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { []string, *argocd.Application, ) (argocd.ApplicationSources, error) { - return []argocd.ApplicationSource{{}}, nil - }, - mustPerformUpdateFn: func( - *PromotionStepContext, - *ArgoCDAppUpdate, - *argocd.Application, - argocd.ApplicationSources, - ) (argocd.OperationPhase, bool, error) { - return argocd.OperationRunning, false, fmt.Errorf("waiting for operation to complete") + return nil, errors.New("something went wrong") }, }, stepCtx: &PromotionStepContext{ @@ -673,8 +645,9 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { Apps: []ArgoCDAppUpdate{{}}, }, assertions: func(t *testing.T, res PromotionStepResult, err error) { - require.Equal(t, kargoapi.PromotionPhaseRunning, res.Status) - require.NoError(t, err) + require.Equal(t, kargoapi.PromotionPhaseErrored, res.Status) + require.ErrorContains(t, err, "error building desired sources for Argo CD Application") + require.ErrorContains(t, err, "something went wrong") }, }, { @@ -687,6 +660,13 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { ) (*v1alpha1.Application, error) { return &argocd.Application{}, nil }, + mustPerformUpdateFn: func( + *PromotionStepContext, + *ArgoCDAppUpdate, + *argocd.Application, + ) (argocd.OperationPhase, bool, error) { + return "", true, nil + }, buildDesiredSourcesFn: func( context.Context, *PromotionStepContext, @@ -697,14 +677,6 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { ) (argocd.ApplicationSources, error) { return []argocd.ApplicationSource{{}}, nil }, - mustPerformUpdateFn: func( - *PromotionStepContext, - *ArgoCDAppUpdate, - *argocd.Application, - argocd.ApplicationSources, - ) (argocd.OperationPhase, bool, error) { - return "", true, nil - }, syncApplicationFn: func( context.Context, *PromotionStepContext, @@ -736,28 +708,16 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { ) (*v1alpha1.Application, error) { return &argocd.Application{}, nil }, - buildDesiredSourcesFn: func( - context.Context, - *PromotionStepContext, - *ArgoCDUpdateConfig, - *ArgoCDAppUpdate, - []string, - *argocd.Application, - ) (argocd.ApplicationSources, error) { - return []argocd.ApplicationSource{{}}, nil - }, mustPerformUpdateFn: func() func( *PromotionStepContext, *ArgoCDAppUpdate, *argocd.Application, - argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { var count uint return func( *PromotionStepContext, *ArgoCDAppUpdate, *argocd.Application, - argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { count++ if count > 1 { @@ -766,6 +726,16 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { return "", true, nil } }(), + buildDesiredSourcesFn: func( + context.Context, + *PromotionStepContext, + *ArgoCDUpdateConfig, + *ArgoCDAppUpdate, + []string, + *argocd.Application, + ) (argocd.ApplicationSources, error) { + return []argocd.ApplicationSource{{}}, nil + }, syncApplicationFn: func( context.Context, *PromotionStepContext, @@ -799,21 +769,10 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { ) (*v1alpha1.Application, error) { return &argocd.Application{}, nil }, - buildDesiredSourcesFn: func( - context.Context, - *PromotionStepContext, - *ArgoCDUpdateConfig, - *ArgoCDAppUpdate, - []string, - *argocd.Application, - ) (argocd.ApplicationSources, error) { - return []argocd.ApplicationSource{{}}, nil - }, mustPerformUpdateFn: func( *PromotionStepContext, *ArgoCDAppUpdate, *argocd.Application, - argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { return "Unknown", false, nil }, @@ -839,21 +798,10 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { ) (*v1alpha1.Application, error) { return &argocd.Application{}, nil }, - buildDesiredSourcesFn: func( - context.Context, - *PromotionStepContext, - *ArgoCDUpdateConfig, - *ArgoCDAppUpdate, - []string, - *argocd.Application, - ) (argocd.ApplicationSources, error) { - return []argocd.ApplicationSource{{}}, nil - }, mustPerformUpdateFn: func( *PromotionStepContext, *ArgoCDAppUpdate, *argocd.Application, - argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { return argocd.OperationSucceeded, false, nil }, @@ -1003,11 +951,10 @@ func Test_argoCDUpdater_buildDesiredSources(t *testing.T) { } func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { - testFreightCollectionID := "fake-freight-collection" + testPromotionID := "fake-promotion" testCases := []struct { name string modifyApplication func(*argocd.Application) - desiredSources argocd.ApplicationSources assertions func(t *testing.T, phase argocd.OperationPhase, mustUpdate bool, err error) }{ { @@ -1056,7 +1003,7 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { }, }, { - name: "running operation initiated for incorrect freight collection", + name: "running operation initiated for incorrect Promotion", modifyApplication: func(app *argocd.Application) { app.Status.OperationState = &argocd.OperationState{ Phase: argocd.OperationRunning, @@ -1065,7 +1012,7 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { Username: applicationOperationInitiator, }, Info: []*argocd.Info{{ - Name: freightCollectionInfoKey, + Name: promotionInfoKey, Value: "wrong-freight-collection", }}, }, @@ -1079,7 +1026,7 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { }, }, { - name: "completed operation initiated for incorrect freight collection", + name: "completed operation initiated for incorrect Promotion", modifyApplication: func(app *argocd.Application) { app.Status.OperationState = &argocd.OperationState{ Phase: argocd.OperationSucceeded, @@ -1088,7 +1035,7 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { Username: applicationOperationInitiator, }, Info: []*argocd.Info{{ - Name: freightCollectionInfoKey, + Name: promotionInfoKey, Value: "wrong-freight-collection", }}, }, @@ -1110,8 +1057,8 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { Username: applicationOperationInitiator, }, Info: []*argocd.Info{{ - Name: freightCollectionInfoKey, - Value: testFreightCollectionID, + Name: promotionInfoKey, + Value: testPromotionID, }}, }, } @@ -1132,8 +1079,8 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { Username: applicationOperationInitiator, }, Info: []*argocd.Info{{ - Name: freightCollectionInfoKey, - Value: testFreightCollectionID, + Name: promotionInfoKey, + Value: testPromotionID, }}, }, SyncResult: &argocd.SyncOperationResult{}, @@ -1158,8 +1105,8 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { Username: applicationOperationInitiator, }, Info: []*argocd.Info{{ - Name: freightCollectionInfoKey, - Value: testFreightCollectionID, + Name: promotionInfoKey, + Value: testPromotionID, }}, }, } @@ -1183,8 +1130,8 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { Username: applicationOperationInitiator, }, Info: []*argocd.Info{{ - Name: freightCollectionInfoKey, - Value: testFreightCollectionID, + Name: promotionInfoKey, + Value: testPromotionID, }}, }, SyncResult: &argocd.SyncOperationResult{ @@ -1199,46 +1146,6 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { require.True(t, mustUpdate) }, }, - { - name: "desired sources do not match operation state", - modifyApplication: func(app *argocd.Application) { - app.Spec.Sources = argocd.ApplicationSources{ - { - RepoURL: "https://github.com/universe/42", - }, - } - app.Status.OperationState = &argocd.OperationState{ - Phase: argocd.OperationSucceeded, - Operation: argocd.Operation{ - InitiatedBy: argocd.OperationInitiator{ - Username: applicationOperationInitiator, - }, - Info: []*argocd.Info{{ - Name: freightCollectionInfoKey, - Value: testFreightCollectionID, - }}, - }, - SyncResult: &argocd.SyncOperationResult{ - Revision: "fake-revision", - Sources: argocd.ApplicationSources{ - { - RepoURL: "https://github.com/different/universe", - }, - }, - }, - } - }, - desiredSources: argocd.ApplicationSources{ - { - RepoURL: "https://github.com/universe/42", - }, - }, - assertions: func(t *testing.T, phase argocd.OperationPhase, mustUpdate bool, err error) { - require.ErrorContains(t, err, "does not match desired source") - require.Empty(t, phase) - require.True(t, mustUpdate) - }, - }, { name: "operation completed", modifyApplication: func(app *argocd.Application) { @@ -1252,8 +1159,8 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { Username: applicationOperationInitiator, }, Info: []*argocd.Info{{ - Name: freightCollectionInfoKey, - Value: testFreightCollectionID, + Name: promotionInfoKey, + Value: testPromotionID, }}, }, SyncResult: &argocd.SyncOperationResult{ @@ -1293,15 +1200,9 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { } phase, mustUpdate, err := runner.mustPerformUpdate( - &PromotionStepContext{ - Freight: kargoapi.FreightCollection{ - // Hard-coded for testing purposes - ID: testFreightCollectionID, - }, - }, + &PromotionStepContext{Promotion: testPromotionID}, &stepCfg.Apps[0], app, - testCase.desiredSources, ) testCase.assertions(t, phase, mustUpdate, err) })