diff --git a/internal/controller/promotion/argocd.go b/internal/controller/promotion/argocd.go index 92edd573f..9f1e5fb7c 100644 --- a/internal/controller/promotion/argocd.go +++ b/internal/controller/promotion/argocd.go @@ -25,6 +25,7 @@ const ( authorizedStageAnnotationKey = "kargo.akuity.io/authorized-stage" applicationOperationInitiator = "kargo-controller" + freightCollectionInfoKey = "kargo.akuity.io/freight-collection" ) // argoCDMechanism is an implementation of the Mechanism interface that updates @@ -45,15 +46,16 @@ type argoCDMechanism struct { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) syncApplicationFn func( - context.Context, - *argocd.Application, - *argocd.ApplicationSource, - argocd.ApplicationSources, + ctx context.Context, + app *argocd.Application, + desiredSource *argocd.ApplicationSource, + desiredSources argocd.ApplicationSources, + freightColID string, ) error getAuthorizedApplicationFn func( ctx context.Context, @@ -106,52 +108,31 @@ func (a *argoCDMechanism) Promote( ctx context.Context, stage *kargoapi.Stage, promo *kargoapi.Promotion, - newFreight []kargoapi.FreightReference, -) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) { +) error { updates := stage.Spec.PromotionMechanisms.ArgoCDAppUpdates if len(updates) == 0 { - return promo.Status.WithPhase(kargoapi.PromotionPhaseSucceeded), newFreight, nil + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded + return nil } if a.argocdClient == nil { - return promo.Status.WithPhase(kargoapi.PromotionPhaseFailed), newFreight, - errors.New( - "Argo CD integration is disabled on this controller; cannot perform " + - "promotion", - ) + promo.Status.Phase = kargoapi.PromotionPhaseFailed + return errors.New( + "Argo CD integration is disabled on this controller; cannot perform promotion", + ) } logger := logging.LoggerFromContext(ctx) logger.Debug("executing Argo CD-based promotion mechanisms") var updateResults = make([]argocd.OperationPhase, 0, len(updates)) - var newStatus = promo.Status.DeepCopy() for i := range updates { update := &updates[i] // Retrieve the Argo CD Application. app, err := a.getAuthorizedApplicationFn(ctx, update.AppNamespace, update.AppName, stage.ObjectMeta) if err != nil { - return nil, newFreight, err - } - - // If we do not have specific source updates, request a sync of the - // Application with its current source(s). - if len(update.SourceUpdates) == 0 { - if err = a.syncApplicationFn( - ctx, - app, - app.Spec.Source.DeepCopy(), - app.Spec.Sources.DeepCopy(), - ); err != nil { - return nil, newFreight, err - } - - // As we have no knowledge of the specifically desired revision(s), - // we cannot wait for the update to complete as we would not know - // what to wait for. - updateResults = append(updateResults, argocd.OperationSucceeded) - continue + return err } // Build the desired source(s) for the Argo CD Application. @@ -160,10 +141,10 @@ func (a *argoCDMechanism) Promote( stage, update, app, - newFreight, + promo.Status.FreightCollection.References(), ) if err != nil { - return nil, newFreight, err + return err } // Check if the update needs to be performed and retrieve its phase. @@ -172,7 +153,7 @@ func (a *argoCDMechanism) Promote( stage, update, app, - newFreight, + promo.Status.FreightCollection, desiredSource, desiredSources, ) @@ -189,7 +170,7 @@ func (a *argoCDMechanism) Promote( if phase == "" { // If we do not have a phase, we cannot continue processing // this update by waiting. - return nil, newFreight, err + return err } // Log the error as a warning, but continue to the next update. logger.Info(err.Error()) @@ -197,7 +178,7 @@ func (a *argoCDMechanism) Promote( if phase.Failed() { // Record the reason for the failure if available. if app.Status.OperationState != nil { - newStatus.Message = fmt.Sprintf( + promo.Status.Message = fmt.Sprintf( "Argo CD Application %q in namespace %q failed with: %s", app.Name, app.Namespace, @@ -214,8 +195,14 @@ func (a *argoCDMechanism) Promote( } // Perform the update. - if err = a.syncApplicationFn(ctx, app, desiredSource, desiredSources); err != nil { - return nil, newFreight, err + if err = a.syncApplicationFn( + ctx, + app, + desiredSource, + desiredSources, + promo.Status.FreightCollection.ID, + ); err != nil { + return err } // As we have initiated an update, we should wait for it to complete. updateResults = append(updateResults, argocd.OperationRunning) @@ -223,15 +210,15 @@ func (a *argoCDMechanism) Promote( aggregatedPhase := operationPhaseToPromotionPhase(updateResults...) if aggregatedPhase == "" { - return nil, newFreight, fmt.Errorf( + return fmt.Errorf( "could not determine promotion phase from operation phases: %v", updateResults, ) } logger.Debug("done executing Argo CD-based promotion mechanisms") - newStatus.Phase = aggregatedPhase - return newStatus, newFreight, nil + promo.Status.Phase = aggregatedPhase + return nil } // buildDesiredSources returns the desired source(s) for an Argo CD Application, @@ -282,7 +269,7 @@ func (a *argoCDMechanism) mustPerformUpdate( stage *kargoapi.Stage, update *kargoapi.ArgoCDAppUpdate, app *argocd.Application, - newFreight []kargoapi.FreightReference, + freightCol *kargoapi.FreightCollection, desiredSource *argocd.ApplicationSource, desiredSources argocd.ApplicationSources, ) (phase argocd.OperationPhase, mustUpdate bool, err error) { @@ -292,6 +279,8 @@ func (a *argoCDMechanism) mustPerformUpdate( return "", true, nil } + // Deal with the possibility that the operation was not initiated by the + // expected user. if status.Operation.InitiatedBy.Username != applicationOperationInitiator { // The operation was not initiated by the expected user. if !status.Phase.Completed() { @@ -308,6 +297,31 @@ func (a *argoCDMechanism) mustPerformUpdate( return "", true, nil } + // 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 + for _, info := range status.Operation.Info { + if info.Name == freightCollectionInfoKey { + correctFreightColIDFound = info.Value == freightCol.ID + break + } + } + if !correctFreightColIDFound { + // The operation was not initiated for the current freight collection. + 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", + freightCol.ID, + ) + } + // Initiate our own operation. + return "", true, nil + } + if !status.Phase.Completed() { // The operation is still running. return status.Phase, false, nil @@ -326,7 +340,7 @@ func (a *argoCDMechanism) mustPerformUpdate( stage, update, app, - newFreight, + freightCol.References(), ); err != nil { return "", true, fmt.Errorf("error determining desired revision: %w", err) } else if desiredRevision != "" && status.SyncResult.Revision != desiredRevision { @@ -359,6 +373,7 @@ func (a *argoCDMechanism) syncApplication( app *argocd.Application, desiredSource *argocd.ApplicationSource, desiredSources argocd.ApplicationSources, + freightColID string, ) error { // Create a patch for the Application. patch := client.MergeFrom(app.DeepCopy()) @@ -384,6 +399,10 @@ func (a *argoCDMechanism) syncApplication( Name: "Reason", Value: "Promotion triggered a sync of this Application resource.", }, + { + Name: freightCollectionInfoKey, + Value: freightColID, + }, }, Sync: &argocd.SyncOperation{ Revisions: []string{}, diff --git a/internal/controller/promotion/argocd_test.go b/internal/controller/promotion/argocd_test.go index b60ce08ee..fde27f7f2 100644 --- a/internal/controller/promotion/argocd_test.go +++ b/internal/controller/promotion/argocd_test.go @@ -48,9 +48,8 @@ func TestArgoCDPromote(t *testing.T) { newFreight []kargoapi.FreightReference assertions func( t *testing.T, - newStatus *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) }{ @@ -64,13 +63,13 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -87,9 +86,8 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - _ []kargoapi.FreightReference, - _ []kargoapi.FreightReference, + _ *kargoapi.FreightCollection, + _ *kargoapi.Promotion, err error, ) { require.ErrorContains( @@ -121,96 +119,13 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.ErrorContains(t, err, "something went wrong") - require.Equal(t, newFreightIn, newFreightOut) - }, - }, - { - name: "request sync without updates", - promoMech: &argoCDMechanism{ - argocdClient: fake.NewFakeClient(), - getAuthorizedApplicationFn: func( - context.Context, - string, - string, - metav1.ObjectMeta, - ) (*argocd.Application, error) { - return &argocd.Application{}, nil - }, - syncApplicationFn: func( - context.Context, - *argocd.Application, - *argocd.ApplicationSource, - argocd.ApplicationSources, - ) error { - return nil - }, - }, - stage: &kargoapi.Stage{ - Spec: kargoapi.StageSpec{ - PromotionMechanisms: &kargoapi.PromotionMechanisms{ - ArgoCDAppUpdates: []kargoapi.ArgoCDAppUpdate{ - {}, - }, - }, - }, - }, - assertions: func( - t *testing.T, - status *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, - err error, - ) { - require.NoError(t, err) - require.Equal(t, kargoapi.PromotionPhaseSucceeded, status.Phase) - require.Equal(t, newFreightIn, newFreightOut) - }, - }, - { - name: "error requesting sync without updates", - promoMech: &argoCDMechanism{ - argocdClient: fake.NewFakeClient(), - getAuthorizedApplicationFn: func( - context.Context, - string, - string, - metav1.ObjectMeta, - ) (*argocd.Application, error) { - return &argocd.Application{}, nil - }, - syncApplicationFn: func( - context.Context, - *argocd.Application, - *argocd.ApplicationSource, - argocd.ApplicationSources, - ) error { - return errors.New("something went wrong") - }, - }, - stage: &kargoapi.Stage{ - Spec: kargoapi.StageSpec{ - PromotionMechanisms: &kargoapi.PromotionMechanisms{ - ArgoCDAppUpdates: []kargoapi.ArgoCDAppUpdate{ - {}, - }, - }, - }, - }, - assertions: func( - t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, - err error, - ) { - require.ErrorContains(t, err, "something went wrong") - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -250,13 +165,13 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.ErrorContains(t, err, "something went wrong") - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -285,7 +200,7 @@ func TestArgoCDPromote(t *testing.T) { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { @@ -307,13 +222,13 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.ErrorContains(t, err, "something went wrong") - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -342,7 +257,7 @@ func TestArgoCDPromote(t *testing.T) { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { @@ -353,6 +268,7 @@ func TestArgoCDPromote(t *testing.T) { *argocd.Application, *argocd.ApplicationSource, argocd.ApplicationSources, + string, ) error { return nil }, @@ -372,13 +288,12 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - status *kargoapi.PromotionStatus, - _ []kargoapi.FreightReference, - _ []kargoapi.FreightReference, + _ *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) - require.Equal(t, kargoapi.PromotionPhaseRunning, status.Phase) + require.Equal(t, kargoapi.PromotionPhaseRunning, promo.Status.Phase) }, }, { @@ -407,7 +322,7 @@ func TestArgoCDPromote(t *testing.T) { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { @@ -429,14 +344,14 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - status *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) - require.Equal(t, kargoapi.PromotionPhaseRunning, status.Phase) - require.Equal(t, newFreightIn, newFreightOut) + require.Equal(t, kargoapi.PromotionPhaseRunning, promo.Status.Phase) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -465,7 +380,7 @@ func TestArgoCDPromote(t *testing.T) { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { @@ -487,14 +402,14 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - status *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) - require.Equal(t, kargoapi.PromotionPhaseRunning, status.Phase) - require.Equal(t, newFreightIn, newFreightOut) + require.Equal(t, kargoapi.PromotionPhaseRunning, promo.Status.Phase) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -523,7 +438,7 @@ func TestArgoCDPromote(t *testing.T) { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { @@ -534,6 +449,7 @@ func TestArgoCDPromote(t *testing.T) { *argocd.Application, *argocd.ApplicationSource, argocd.ApplicationSources, + string, ) error { return errors.New("something went wrong") }, @@ -553,9 +469,8 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.Error(t, err) @@ -564,7 +479,8 @@ func TestArgoCDPromote(t *testing.T) { "something went wrong", err.Error(), ) - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -593,7 +509,7 @@ func TestArgoCDPromote(t *testing.T) { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { @@ -603,7 +519,7 @@ func TestArgoCDPromote(t *testing.T) { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { @@ -619,6 +535,7 @@ func TestArgoCDPromote(t *testing.T) { *argocd.Application, *argocd.ApplicationSource, argocd.ApplicationSources, + string, ) error { return nil }, @@ -643,14 +560,14 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - status *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) - require.Equal(t, kargoapi.PromotionPhaseFailed, status.Phase) - require.Equal(t, newFreightIn, newFreightOut) + require.Equal(t, kargoapi.PromotionPhaseFailed, promo.Status.Phase) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -679,7 +596,7 @@ func TestArgoCDPromote(t *testing.T) { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { @@ -701,13 +618,13 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.ErrorContains(t, err, "could not determine promotion phase from operation phases") - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -736,7 +653,7 @@ func TestArgoCDPromote(t *testing.T) { *kargoapi.Stage, *kargoapi.ArgoCDAppUpdate, *argocd.Application, - []kargoapi.FreightReference, + *kargoapi.FreightCollection, *argocd.ApplicationSource, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { @@ -758,29 +675,37 @@ func TestArgoCDPromote(t *testing.T) { }, assertions: func( t *testing.T, - status *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) - require.Equal(t, kargoapi.PromotionPhaseSucceeded, status.Phase) - require.Equal(t, newFreightIn, newFreightOut) + require.Equal(t, kargoapi.PromotionPhaseSucceeded, promo.Status.Phase) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - newStatus, newFreightOut, err := testCase.promoMech.Promote( + promo := &kargoapi.Promotion{ + Status: kargoapi.PromotionStatus{ + FreightCollection: &kargoapi.FreightCollection{}, + }, + } + for _, freight := range testCase.newFreight { + promo.Status.FreightCollection.UpdateOrPush(freight) + } + origFreight := promo.Status.FreightCollection.DeepCopy() + err := testCase.promoMech.Promote( logging.ContextWithLogger( context.Background(), logging.Wrap(logr.Discard()), ), testCase.stage, - &kargoapi.Promotion{}, - testCase.newFreight, + promo, ) - testCase.assertions(t, newStatus, testCase.newFreight, newFreightOut, err) + testCase.assertions(t, origFreight, promo, err) }) } } @@ -995,6 +920,7 @@ func TestArgoCDBuildDesiredSources(t *testing.T) { } func TestArgoCDMustPerformUpdate(t *testing.T) { + testFreightCollectionID := "fake-freight-collection" testOrigin := kargoapi.FreightOrigin{ Kind: kargoapi.FreightOriginKindWarehouse, Name: "fake-warehouse", @@ -1016,7 +942,7 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { }, }, { - name: "pending operation initiated by different user", + name: "running operation initiated by different user", modifyApplication: func(app *argocd.Application) { app.Status.OperationState = &argocd.OperationState{ Phase: argocd.OperationRunning, @@ -1053,7 +979,7 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { }, }, { - name: "pending operation initiated by us", + name: "running operation initiated for incorrect freight collection", modifyApplication: func(app *argocd.Application) { app.Status.OperationState = &argocd.OperationState{ Phase: argocd.OperationRunning, @@ -1061,6 +987,55 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { InitiatedBy: argocd.OperationInitiator{ Username: applicationOperationInitiator, }, + Info: []*argocd.Info{{ + Name: freightCollectionInfoKey, + Value: "wrong-freight-collection", + }}, + }, + } + }, + assertions: func(t *testing.T, phase argocd.OperationPhase, mustUpdate bool, err error) { + require.ErrorContains(t, err, "current operation was not initiated for") + require.ErrorContains(t, err, "waiting for operation to complete") + require.Equal(t, argocd.OperationRunning, phase) + require.False(t, mustUpdate) + }, + }, + { + name: "completed operation initiated for incorrect freight collection", + modifyApplication: func(app *argocd.Application) { + app.Status.OperationState = &argocd.OperationState{ + Phase: argocd.OperationSucceeded, + Operation: argocd.Operation{ + InitiatedBy: argocd.OperationInitiator{ + Username: applicationOperationInitiator, + }, + Info: []*argocd.Info{{ + Name: freightCollectionInfoKey, + Value: "wrong-freight-collection", + }}, + }, + } + }, + assertions: func(t *testing.T, phase argocd.OperationPhase, mustUpdate bool, err error) { + require.NoError(t, err) + require.True(t, mustUpdate) + require.Empty(t, phase) + }, + }, + { + name: "running operation", + modifyApplication: func(app *argocd.Application) { + app.Status.OperationState = &argocd.OperationState{ + Phase: argocd.OperationRunning, + Operation: argocd.Operation{ + InitiatedBy: argocd.OperationInitiator{ + Username: applicationOperationInitiator, + }, + Info: []*argocd.Info{{ + Name: freightCollectionInfoKey, + Value: testFreightCollectionID, + }}, }, } }, @@ -1079,6 +1054,10 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { InitiatedBy: argocd.OperationInitiator{ Username: applicationOperationInitiator, }, + Info: []*argocd.Info{{ + Name: freightCollectionInfoKey, + Value: testFreightCollectionID, + }}, }, SyncResult: &argocd.SyncOperationResult{}, } @@ -1101,6 +1080,10 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { InitiatedBy: argocd.OperationInitiator{ Username: applicationOperationInitiator, }, + Info: []*argocd.Info{{ + Name: freightCollectionInfoKey, + Value: testFreightCollectionID, + }}, }, } }, @@ -1130,6 +1113,10 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { InitiatedBy: argocd.OperationInitiator{ Username: applicationOperationInitiator, }, + Info: []*argocd.Info{{ + Name: freightCollectionInfoKey, + Value: testFreightCollectionID, + }}, }, SyncResult: &argocd.SyncOperationResult{ Revision: "other-fake-revision", @@ -1163,6 +1150,10 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { InitiatedBy: argocd.OperationInitiator{ Username: applicationOperationInitiator, }, + Info: []*argocd.Info{{ + Name: freightCollectionInfoKey, + Value: testFreightCollectionID, + }}, }, SyncResult: &argocd.SyncOperationResult{ Revision: "fake-revision", @@ -1195,6 +1186,10 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { InitiatedBy: argocd.OperationInitiator{ Username: applicationOperationInitiator, }, + Info: []*argocd.Info{{ + Name: freightCollectionInfoKey, + Value: testFreightCollectionID, + }}, }, SyncResult: &argocd.SyncOperationResult{ Revision: "fake-revision", @@ -1230,6 +1225,10 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { InitiatedBy: argocd.OperationInitiator{ Username: applicationOperationInitiator, }, + Info: []*argocd.Info{{ + Name: freightCollectionInfoKey, + Value: testFreightCollectionID, + }}, }, SyncResult: &argocd.SyncOperationResult{ Revision: "fake-revision", @@ -1287,12 +1286,19 @@ func TestArgoCDMustPerformUpdate(t *testing.T) { }, } + freight := &kargoapi.FreightCollection{} + for _, ref := range testCase.newFreight { + freight.UpdateOrPush(ref) + } + // Tamper with the freight collection ID for testing purposes + freight.ID = testFreightCollectionID + phase, mustUpdate, err := argocdMech.mustPerformUpdate( context.Background(), stage, &stage.Spec.PromotionMechanisms.ArgoCDAppUpdates[0], app, - testCase.newFreight, + freight, testCase.desiredSource, testCase.desiredSources, ) @@ -1372,6 +1378,7 @@ func TestArgoCDSyncApplication(t *testing.T) { testCase.app, testCase.desiredSource, testCase.desiredSources, + "fake-freight-collection-id", ), ) }) diff --git a/internal/controller/promotion/composite.go b/internal/controller/promotion/composite.go index e1862ab71..0307e15a0 100644 --- a/internal/controller/promotion/composite.go +++ b/internal/controller/promotion/composite.go @@ -43,31 +43,25 @@ func (c *compositeMechanism) Promote( ctx context.Context, stage *kargoapi.Stage, promo *kargoapi.Promotion, - newFreight []kargoapi.FreightReference, -) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) { +) error { if stage.Spec.PromotionMechanisms == nil { - return &kargoapi.PromotionStatus{Phase: kargoapi.PromotionPhaseSucceeded}, - newFreight, nil + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded + return nil } - var newStatus *kargoapi.PromotionStatus - logger := logging.LoggerFromContext(ctx).WithValues("name", c.name) logger.Debug("executing composite promotion mechanism") + // Start with success and degrade as child mechanisms report more severe + // phases. + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded for _, childMechanism := range c.childMechanisms { - var err error - var otherStatus *kargoapi.PromotionStatus - otherStatus, newFreight, err = childMechanism.Promote(ctx, stage, promo, newFreight) - if err != nil { - return nil, newFreight, fmt.Errorf( - "error executing %s: %w", - childMechanism.GetName(), - err, - ) + origStatus := promo.Status.DeepCopy() + if err := childMechanism.Promote(ctx, stage, promo); err != nil { + return fmt.Errorf("error executing %s: %w", childMechanism.GetName(), err) } - newStatus = aggregateGitPromoStatus(newStatus, *otherStatus) - if newStatus.Phase != kargoapi.PromotionPhaseSucceeded { + promo.Status = *mergePromoStatus(&promo.Status, origStatus) + if promo.Status.Phase != kargoapi.PromotionPhaseSucceeded { // We only continue to the next promotion mechanism if the current // mechanism succeeded. This is because a PR must be merged before // performing the ArgoCD sync. @@ -77,46 +71,56 @@ func (c *compositeMechanism) Promote( logger.Debug( "done executing composite promotion mechanism", - "aggregatedStatus", newStatus.Phase, + "aggregatedStatus", promo.Status.Phase, ) - return newStatus, newFreight, nil + return nil } -// aggregateGitPromoStatus returns the aggregated status of two promotion statuses when -// multiple promote mechanisms are used. Returns the most severe phase. In order of precedence: +// mergePromoStatus merges the PromotionStatus represented by newerStatus into a +// deep copy of the PromotionStatus represented by olderStatus and returns the +// result. The returned status will differ from the olderStatus in the following +// ways: // -// Error, Failed, Running, Succeeded -func aggregateGitPromoStatus(curr *kargoapi.PromotionStatus, other kargoapi.PromotionStatus) *kargoapi.PromotionStatus { - if curr == nil { - return other.DeepCopy() - } - newStatus := curr.DeepCopy() - if curr.Phase == kargoapi.PromotionPhaseErrored { - // do nothing. we are already at most severe phase - } else if other.Phase == kargoapi.PromotionPhaseErrored { - newStatus.Phase = kargoapi.PromotionPhaseErrored - newStatus.Message = other.Message - } else if curr.Phase == kargoapi.PromotionPhaseFailed || other.Phase == kargoapi.PromotionPhaseFailed { - newStatus.Phase = kargoapi.PromotionPhaseFailed - newStatus.Message = firstNonEmpty(curr.Message, other.Message) - } else if curr.Phase == kargoapi.PromotionPhaseRunning || other.Phase == kargoapi.PromotionPhaseRunning { - newStatus.Phase = kargoapi.PromotionPhaseRunning - newStatus.Message = firstNonEmpty(curr.Message, other.Message) - } else { - newStatus.Phase = kargoapi.PromotionPhaseSucceeded - newStatus.Message = firstNonEmpty(curr.Message, other.Message) +// 1. The Phase and corresponding Message are updated to reflect the more +// severe of the two. The order of severity is: +// Errored > Failed > Running > Succeeded. +// 2. The FreightCollection is unconditionally updated to that of src. +// 3. Metadata from src is merged into Metadata from olderStatus, with Metadata +// from src taking precedence in case of key conflicts. +// +// Both arguments must be non-nil. +func mergePromoStatus( + newerStatus *kargoapi.PromotionStatus, + olderStatus *kargoapi.PromotionStatus, +) *kargoapi.PromotionStatus { + mergedStatus := olderStatus.DeepCopy() + switch { + case mergedStatus.Phase == kargoapi.PromotionPhaseErrored: + // Do nothing. We are already at most severe phase. + case newerStatus.Phase == kargoapi.PromotionPhaseErrored: + mergedStatus.Phase = kargoapi.PromotionPhaseErrored + mergedStatus.Message = newerStatus.Message + case mergedStatus.Phase == kargoapi.PromotionPhaseFailed || newerStatus.Phase == kargoapi.PromotionPhaseFailed: + mergedStatus.Phase = kargoapi.PromotionPhaseFailed + mergedStatus.Message = firstNonEmpty(mergedStatus.Message, newerStatus.Message) + case mergedStatus.Phase == kargoapi.PromotionPhaseRunning || newerStatus.Phase == kargoapi.PromotionPhaseRunning: + mergedStatus.Phase = kargoapi.PromotionPhaseRunning + mergedStatus.Message = firstNonEmpty(mergedStatus.Message, newerStatus.Message) + case mergedStatus.Phase == kargoapi.PromotionPhaseSucceeded && newerStatus.Phase == kargoapi.PromotionPhaseSucceeded: + mergedStatus.Message = firstNonEmpty(mergedStatus.Message, newerStatus.Message) } + mergedStatus.FreightCollection = newerStatus.FreightCollection // Merge the two metadata maps - if len(other.Metadata) > 0 { - if newStatus.Metadata == nil { - newStatus.Metadata = make(map[string]string) + if len(newerStatus.Metadata) > 0 { + if mergedStatus.Metadata == nil { + mergedStatus.Metadata = make(map[string]string, len(newerStatus.Metadata)) } - for k, v := range other.Metadata { - newStatus.Metadata[k] = v + for k, v := range newerStatus.Metadata { + mergedStatus.Metadata[k] = v } } - return newStatus + return mergedStatus } func firstNonEmpty(a, b string) string { diff --git a/internal/controller/promotion/composite_test.go b/internal/controller/promotion/composite_test.go index 574dad000..644b715aa 100644 --- a/internal/controller/promotion/composite_test.go +++ b/internal/controller/promotion/composite_test.go @@ -3,6 +3,7 @@ package promotion import ( "context" "errors" + "fmt" "testing" "github.com/stretchr/testify/require" @@ -44,8 +45,8 @@ func TestCompositePromote(t *testing.T) { freight []kargoapi.FreightReference assertions func( t *testing.T, - promoStatus *kargoapi.PromotionStatus, - updatedFreight []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) }{ @@ -55,22 +56,16 @@ func TestCompositePromote(t *testing.T) { childMechanisms: []Mechanism{ &FakeMechanism{ Name: "fake promotion mechanism", - PromoteFn: func( - context.Context, - *kargoapi.Stage, - []kargoapi.FreightReference, - ) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) { - return &kargoapi.PromotionStatus{}, - []kargoapi.FreightReference{}, - errors.New("something went wrong") + PromoteFn: func(context.Context, *kargoapi.Stage, *kargoapi.Promotion) error { + return errors.New("something went wrong") }, }, }, }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - _ []kargoapi.FreightReference, + _ *kargoapi.FreightCollection, + _ *kargoapi.Promotion, err error, ) { require.ErrorContains(t, err, "error executing fake promotion mechanism") @@ -80,7 +75,8 @@ func TestCompositePromote(t *testing.T) { { name: "success", freight: []kargoapi.FreightReference{{ - Name: "fake-id", + Name: "fake-id", + Commits: []kargoapi.GitCommit{{}}, }}, promoMech: &compositeMechanism{ childMechanisms: []Mechanism{ @@ -89,50 +85,151 @@ func TestCompositePromote(t *testing.T) { PromoteFn: func( _ context.Context, _ *kargoapi.Stage, - newFreight []kargoapi.FreightReference, - ) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) { - require.True(t, len(newFreight) > 0) - // This is not a realistic change that a child promotion mechanism - // would make, but for testing purposes, this is good enough to - // help us assert that the function under test does return all - // modifications made by its child promotion mechanisms. - newFreight[0].Name = "fake-mutated-id" - return &kargoapi.PromotionStatus{Phase: kargoapi.PromotionPhaseSucceeded}, newFreight, nil + promo *kargoapi.Promotion, + ) error { + refs := promo.Status.FreightCollection.References() + require.NotEmpty(t, refs) + require.NotEmpty(t, refs[0].Commits) + refs[0].Commits[0].HealthCheckCommit = "fake-commit-id" + promo.Status.FreightCollection.UpdateOrPush(refs[0]) + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded + return nil }, }, }, }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - updatedFreight []kargoapi.FreightReference, + _ *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) // Verify that changes made by child promotion mechanism are returned - require.Equal( - t, - []kargoapi.FreightReference{{ - Name: "fake-mutated-id", - }}, - updatedFreight, - ) + refs := promo.Status.FreightCollection.References() + require.NotEmpty(t, refs) + require.NotEmpty(t, refs[0].Commits) + require.Equal(t, "fake-commit-id", refs[0].Commits[0].HealthCheckCommit) }, }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - promoStatus, updatedFreight, err := testCase.promoMech.Promote( + promo := &kargoapi.Promotion{ + Status: kargoapi.PromotionStatus{ + FreightCollection: &kargoapi.FreightCollection{}, + }, + } + for _, freight := range testCase.freight { + promo.Status.FreightCollection.UpdateOrPush(freight) + } + origFreight := promo.Status.FreightCollection.DeepCopy() + err := testCase.promoMech.Promote( context.Background(), &kargoapi.Stage{ Spec: kargoapi.StageSpec{ PromotionMechanisms: &kargoapi.PromotionMechanisms{}, }, }, - &kargoapi.Promotion{}, - testCase.freight, + promo, ) - testCase.assertions(t, promoStatus, updatedFreight, err) + testCase.assertions(t, origFreight, promo, err) }) } } + +func TestMergePromoStatus(t *testing.T) { + t.Run("phase merging", func(t *testing.T) { + testCases := []struct { + olderPhase kargoapi.PromotionPhase + newerPhase kargoapi.PromotionPhase + expectedPhase kargoapi.PromotionPhase + }{ + { + olderPhase: kargoapi.PromotionPhaseErrored, + newerPhase: kargoapi.PromotionPhaseFailed, + expectedPhase: kargoapi.PromotionPhaseErrored, + }, + { + olderPhase: kargoapi.PromotionPhaseFailed, + newerPhase: kargoapi.PromotionPhaseErrored, + expectedPhase: kargoapi.PromotionPhaseErrored, + }, + { + olderPhase: kargoapi.PromotionPhaseFailed, + newerPhase: kargoapi.PromotionPhaseRunning, + expectedPhase: kargoapi.PromotionPhaseFailed, + }, + { + olderPhase: kargoapi.PromotionPhaseRunning, + newerPhase: kargoapi.PromotionPhaseFailed, + expectedPhase: kargoapi.PromotionPhaseFailed, + }, + { + olderPhase: kargoapi.PromotionPhaseRunning, + newerPhase: kargoapi.PromotionPhaseSucceeded, + expectedPhase: kargoapi.PromotionPhaseRunning, + }, + { + olderPhase: kargoapi.PromotionPhaseSucceeded, + newerPhase: kargoapi.PromotionPhaseRunning, + expectedPhase: kargoapi.PromotionPhaseRunning, + }, + { + olderPhase: kargoapi.PromotionPhaseSucceeded, + newerPhase: kargoapi.PromotionPhaseSucceeded, + expectedPhase: kargoapi.PromotionPhaseSucceeded, + }, + } + for _, testCase := range testCases { + t.Run( + fmt.Sprintf("old is %s, new is %s", testCase.olderPhase, testCase.newerPhase), + func(t *testing.T) { + mergedStatus := mergePromoStatus( + &kargoapi.PromotionStatus{Phase: testCase.newerPhase}, + &kargoapi.PromotionStatus{Phase: testCase.olderPhase}, + ) + require.Equal(t, testCase.expectedPhase, mergedStatus.Phase) + }, + ) + } + }) + + t.Run("freight collection replacement", func(t *testing.T) { + olderStatus := &kargoapi.PromotionStatus{ + FreightCollection: &kargoapi.FreightCollection{}, + } + olderStatus.FreightCollection.UpdateOrPush(kargoapi.FreightReference{ + Commits: []kargoapi.GitCommit{{}}, + }) + newerStatus := &kargoapi.PromotionStatus{ + FreightCollection: olderStatus.FreightCollection.DeepCopy(), + } + newerStatus.FreightCollection.UpdateOrPush(kargoapi.FreightReference{ + Commits: []kargoapi.GitCommit{{ + HealthCheckCommit: "fake-commit", + }}, + }) + mergedStatus := mergePromoStatus(newerStatus, olderStatus) + require.Same(t, newerStatus.FreightCollection, mergedStatus.FreightCollection) + }) + + t.Run("metadata merging", func(t *testing.T) { + olderStatus := &kargoapi.PromotionStatus{ + Metadata: map[string]string{ + "a": "b", + "c": "d", // Should be overwritten + }, + } + newerStatus := &kargoapi.PromotionStatus{ + Metadata: map[string]string{ + "c": "D", // Should overwrite + "e": "f", + }, + } + mergedStatus := mergePromoStatus(newerStatus, olderStatus) + require.Equal(t, "b", mergedStatus.Metadata["a"]) + require.Equal(t, "D", mergedStatus.Metadata["c"]) + require.Equal(t, "f", mergedStatus.Metadata["e"]) + }) +} diff --git a/internal/controller/promotion/git.go b/internal/controller/promotion/git.go index 219df2e77..52fcc278d 100644 --- a/internal/controller/promotion/git.go +++ b/internal/controller/promotion/git.go @@ -46,8 +46,7 @@ type gitMechanism struct { *kargoapi.Stage, *kargoapi.Promotion, *kargoapi.GitRepoUpdate, - []kargoapi.FreightReference, - ) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) + ) error getReadRefFn func( context.Context, client.Client, @@ -128,37 +127,32 @@ func (g *gitMechanism) Promote( ctx context.Context, stage *kargoapi.Stage, promo *kargoapi.Promotion, - newFreight []kargoapi.FreightReference, -) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) { +) error { updates := g.selectUpdatesFn(stage.Spec.PromotionMechanisms.GitRepoUpdates) if len(updates) == 0 { - return &kargoapi.PromotionStatus{Phase: kargoapi.PromotionPhaseSucceeded}, newFreight, nil + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded + return nil } - var newStatus *kargoapi.PromotionStatus - logger := logging.LoggerFromContext(ctx).WithValues("name", g.name) logger.Debug("executing promotion mechanism") + // Start with success and degrade as individual updates report more severe + // phases. + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded for _, update := range updates { var err error - var otherStatus *kargoapi.PromotionStatus - if otherStatus, newFreight, err = g.doSingleUpdateFn( - ctx, - stage, - promo, - update, - newFreight, - ); err != nil { - return nil, newFreight, err + origStatus := promo.Status.DeepCopy() + if err = g.doSingleUpdateFn(ctx, stage, promo, update); err != nil { + return err } - newStatus = aggregateGitPromoStatus(newStatus, *otherStatus) + promo.Status = *mergePromoStatus(&promo.Status, origStatus) } logger.Debug("done executing promotion mechanism") - return newStatus, newFreight, nil + return nil } // doSingleUpdate updates configuration in a single Git repository by @@ -170,22 +164,21 @@ func (g *gitMechanism) doSingleUpdate( stage *kargoapi.Stage, promo *kargoapi.Promotion, update *kargoapi.GitRepoUpdate, - newFreight []kargoapi.FreightReference, -) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) { +) error { readRef, commit, err := g.getReadRefFn( ctx, g.client, stage, update, - newFreight, + promo.Status.FreightCollection.References(), ) if err != nil { - return nil, newFreight, err + return err } author, err := g.getAuthorFn() if err != nil { - return nil, newFreight, err + return err } if author == nil { author = &git.User{} @@ -196,7 +189,7 @@ func (g *gitMechanism) doSingleUpdate( update.RepoURL, ) if err != nil { - return nil, newFreight, err + return err } if creds == nil { creds = &git.RepoCredentials{} @@ -212,7 +205,7 @@ func (g *gitMechanism) doSingleUpdate( }, ) if err != nil { - return nil, newFreight, fmt.Errorf("error cloning git repo %q: %w", update.RepoURL, err) + return fmt.Errorf("error cloning git repo %q: %w", update.RepoURL, err) } defer repo.Close() @@ -225,7 +218,7 @@ func (g *gitMechanism) doSingleUpdate( if getPullRequestNumberFromMetadata(promo.Status.Metadata, update.RepoURL) == -1 { // PR was never created. Prepare the branch for the commit if err = preparePullRequestBranch(repo, commitBranch, update.WriteBranch); err != nil { - return nil, newFreight, fmt.Errorf("error preparing PR branch %q: %w", update.RepoURL, err) + return fmt.Errorf("error preparing PR branch %q: %w", update.RepoURL, err) } } } @@ -234,36 +227,35 @@ func (g *gitMechanism) doSingleUpdate( ctx, stage, update, - newFreight, + promo.Status.FreightCollection.References(), readRef, commitBranch, repo, *creds, ) if err != nil { - return nil, newFreight, err + return err } - newStatus := promo.Status.DeepCopy() if update.PullRequest != nil { gpClient, err := newGitProvider(update, creds) if err != nil { - return nil, newFreight, err + return err } - commitID, newStatus, err = reconcilePullRequest(ctx, promo.Status, repo, gpClient, commitBranch, update.WriteBranch) + commitID, err = reconcilePullRequest(ctx, promo, repo, gpClient, commitBranch, update.WriteBranch) if err != nil { - return nil, newFreight, err + return err } } else { // For git commit promotions, promotion is successful as soon as the commit is pushed. - newStatus.Phase = kargoapi.PromotionPhaseSucceeded + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded } - if commit != nil && newStatus.Phase == kargoapi.PromotionPhaseSucceeded { + if commit != nil && promo.Status.Phase == kargoapi.PromotionPhaseSucceeded { commit.HealthCheckCommit = commitID } - return newStatus, newFreight, nil + return nil } // getReadRef finds a commitID or branch name to read from in order to apply the diff --git a/internal/controller/promotion/git_test.go b/internal/controller/promotion/git_test.go index 5168415b6..caaab946a 100644 --- a/internal/controller/promotion/git_test.go +++ b/internal/controller/promotion/git_test.go @@ -64,9 +64,8 @@ func TestGitPromote(t *testing.T) { promoMech *gitMechanism assertions func( t *testing.T, - status *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) }{ @@ -79,13 +78,13 @@ func TestGitPromote(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -99,21 +98,20 @@ func TestGitPromote(t *testing.T) { _ *kargoapi.Stage, _ *kargoapi.Promotion, _ *kargoapi.GitRepoUpdate, - newFreight []kargoapi.FreightReference, - ) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) { - return nil, newFreight, errors.New("something went wrong") + ) error { + return errors.New("something went wrong") }, }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.Error(t, err) require.Equal(t, "something went wrong", err.Error()) - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -125,39 +123,43 @@ func TestGitPromote(t *testing.T) { doSingleUpdateFn: func( _ context.Context, _ *kargoapi.Stage, - _ *kargoapi.Promotion, + promo *kargoapi.Promotion, _ *kargoapi.GitRepoUpdate, - newFreight []kargoapi.FreightReference, - ) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) { - return &kargoapi.PromotionStatus{Phase: kargoapi.PromotionPhaseSucceeded}, newFreight, nil + ) error { + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded + return nil }, }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - newFreightIn := []kargoapi.FreightReference{} - status, newFreightOut, err := testCase.promoMech.Promote( + promo := &kargoapi.Promotion{ + Status: kargoapi.PromotionStatus{ + FreightCollection: &kargoapi.FreightCollection{}, + }, + } + origFreight := promo.Status.FreightCollection.DeepCopy() + err := testCase.promoMech.Promote( context.Background(), &kargoapi.Stage{ Spec: kargoapi.StageSpec{ PromotionMechanisms: &kargoapi.PromotionMechanisms{}, }, }, - &kargoapi.Promotion{}, - newFreightIn, + promo, ) - testCase.assertions(t, status, newFreightIn, newFreightOut, err) + testCase.assertions(t, origFreight, promo, err) }) } } @@ -169,9 +171,8 @@ func TestGitDoSingleUpdate(t *testing.T) { promoMech *gitMechanism assertions func( t *testing.T, - status *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) }{ @@ -190,14 +191,14 @@ func TestGitDoSingleUpdate(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.Error(t, err) require.Equal(t, "something went wrong", err.Error()) - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -225,14 +226,14 @@ func TestGitDoSingleUpdate(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.Error(t, err) require.Equal(t, "something went wrong", err.Error()) - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -260,14 +261,14 @@ func TestGitDoSingleUpdate(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.Error(t, err) require.Equal(t, "something went wrong", err.Error()) - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -307,14 +308,14 @@ func TestGitDoSingleUpdate(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.Error(t, err) require.Equal(t, "something went wrong", err.Error()) - require.Equal(t, newFreightIn, newFreightOut) + // The freight collection should be unaltered + require.Equal(t, origFreight, promo.Status.FreightCollection) }, }, { @@ -356,38 +357,47 @@ func TestGitDoSingleUpdate(t *testing.T) { }, assertions: func( t *testing.T, - _ *kargoapi.PromotionStatus, - newFreightIn []kargoapi.FreightReference, - newFreightOut []kargoapi.FreightReference, + origFreight *kargoapi.FreightCollection, + promo *kargoapi.Promotion, err error, ) { require.NoError(t, err) + origRefs := origFreight.References() + updatedRefs := promo.Status.FreightCollection.References() require.Equal( t, "fake-commit-id", - newFreightOut[0].Commits[0].HealthCheckCommit, + updatedRefs[0].Commits[0].HealthCheckCommit, ) // The newFreight is otherwise unaltered - newFreightIn[0].Commits[0].HealthCheckCommit = "" - require.Equal(t, newFreightIn, newFreightOut) + updatedRefs[0].Commits[0].HealthCheckCommit = "" + // The freight collection should be unaltered + require.Equal(t, origRefs, updatedRefs) }, }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - newFreightIn := []kargoapi.FreightReference{{ - Commits: []kargoapi.GitCommit{{}}, - }} - status, newFreightOut, err := testCase.promoMech.doSingleUpdate( + freight := &kargoapi.FreightCollection{} + freight.UpdateOrPush( + kargoapi.FreightReference{ + Commits: []kargoapi.GitCommit{{}}, + }, + ) + promo := &kargoapi.Promotion{ + ObjectMeta: metav1.ObjectMeta{Namespace: "fake-namespace"}, + Status: kargoapi.PromotionStatus{ + FreightCollection: freight, + }, + } + origFreight := freight.DeepCopy() + err := testCase.promoMech.doSingleUpdate( context.Background(), &kargoapi.Stage{}, - &kargoapi.Promotion{ - ObjectMeta: metav1.ObjectMeta{Namespace: "fake-namespace"}, - }, + promo, &kargoapi.GitRepoUpdate{RepoURL: "https://github.com/akuity/kargo"}, - newFreightIn, ) - testCase.assertions(t, status, newFreightIn, newFreightOut, err) + testCase.assertions(t, origFreight, promo, err) }) } } diff --git a/internal/controller/promotion/pullrequest.go b/internal/controller/promotion/pullrequest.go index 756abace0..8e6ddcca4 100644 --- a/internal/controller/promotion/pullrequest.go +++ b/internal/controller/promotion/pullrequest.go @@ -112,25 +112,24 @@ func newGitProvider( // it tracks (i.e. PR url). func reconcilePullRequest( ctx context.Context, - status kargoapi.PromotionStatus, + promo *kargoapi.Promotion, repo git.Repo, gpClient gitprovider.GitProviderService, prBranch string, writeBranch string, -) (string, *kargoapi.PromotionStatus, error) { - newStatus := status.DeepCopy() +) (string, error) { var mergeCommitSHA string - prNumber := getPullRequestNumberFromMetadata(status.Metadata, repo.URL()) + prNumber := getPullRequestNumberFromMetadata(promo.Status.Metadata, repo.URL()) if prNumber == -1 { needsPR, err := repo.RefsHaveDiffs(prBranch, writeBranch) if err != nil { - return "", nil, err + return "", err } if needsPR { title, err := repo.CommitMessage(prBranch) if err != nil { - return "", nil, err + return "", err } createOpts := gitprovider.CreatePullRequestOpts{ Head: prBranch, @@ -146,44 +145,44 @@ func reconcilePullRequest( Base: writeBranch, }) if listErr != nil || len(prs) != 1 { - return "", nil, err + return "", err } // If we get here, we found an existing open PR for the same branches pr = prs[0] } - newStatus.Phase = kargoapi.PromotionPhaseRunning - newStatus.Metadata = setPullRequestMetadata(newStatus.Metadata, repo.URL(), pr.Number, pr.URL) + promo.Status.Phase = kargoapi.PromotionPhaseRunning + promo.Status.Metadata = setPullRequestMetadata(promo.Status.Metadata, repo.URL(), pr.Number, pr.URL) } else { - newStatus.Phase = kargoapi.PromotionPhaseSucceeded - newStatus.Message = "No changes to promote" + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded + promo.Status.Message = "No changes to promote" } } else { // check if existing PR is closed/merged and update promo status to either // Succeeded or Failed depending if PR was merged pr, err := gpClient.GetPullRequest(ctx, prNumber) if err != nil { - return "", nil, err + return "", err } if !pr.IsOpen() { merged, err := gpClient.IsPullRequestMerged(ctx, prNumber) if err != nil { - return "", nil, err + return "", err } if merged { - newStatus.Phase = kargoapi.PromotionPhaseSucceeded - newStatus.Message = "Pull request was merged" + promo.Status.Phase = kargoapi.PromotionPhaseSucceeded + promo.Status.Message = "Pull request was merged" if pr.MergeCommitSHA == "" { - return "", nil, fmt.Errorf("merge commit SHA is empty") + return "", fmt.Errorf("merge commit SHA is empty") } mergeCommitSHA = pr.MergeCommitSHA } else { - newStatus.Phase = kargoapi.PromotionPhaseFailed - newStatus.Message = "Pull request was closed without being merged" + promo.Status.Phase = kargoapi.PromotionPhaseFailed + promo.Status.Message = "Pull request was closed without being merged" } } } - return mergeCommitSHA, newStatus, nil + return mergeCommitSHA, nil } // pullRequestMetadataKey returns the key used to store the pull request number in the metadata map. diff --git a/internal/controller/promotion/root.go b/internal/controller/promotion/root.go index 2cb62ea7f..b705b4644 100644 --- a/internal/controller/promotion/root.go +++ b/internal/controller/promotion/root.go @@ -15,14 +15,8 @@ type Mechanism interface { GetName() string // Promote consults rules in the provided Stage to perform some portion of the // transition to using artifacts from the provided FreightReferences. It - // returns updated PromotionStatus and FreightReferences, both of which may - // have been updated by the process. - Promote( - context.Context, - *kargoapi.Stage, - *kargoapi.Promotion, - []kargoapi.FreightReference, - ) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) + // may modify the provided Promotion's status. + Promote(context.Context, *kargoapi.Stage, *kargoapi.Promotion) error } // NewMechanisms returns the entrypoint to a hierarchical tree of promotion diff --git a/internal/controller/promotion/root_test.go b/internal/controller/promotion/root_test.go index 0ab111742..ef4f76d37 100644 --- a/internal/controller/promotion/root_test.go +++ b/internal/controller/promotion/root_test.go @@ -27,8 +27,8 @@ type FakeMechanism struct { PromoteFn func( context.Context, *kargoapi.Stage, - []kargoapi.FreightReference, - ) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) + *kargoapi.Promotion, + ) error } // GetName implements the Mechanism interface. @@ -40,8 +40,7 @@ func (f *FakeMechanism) GetName() string { func (f *FakeMechanism) Promote( ctx context.Context, stage *kargoapi.Stage, - _ *kargoapi.Promotion, - freight []kargoapi.FreightReference, -) (*kargoapi.PromotionStatus, []kargoapi.FreightReference, error) { - return f.PromoteFn(ctx, stage, freight) + promo *kargoapi.Promotion, +) error { + return f.PromoteFn(ctx, stage, promo) } diff --git a/internal/controller/promotions/promotions.go b/internal/controller/promotions/promotions.go index 3bbee5b1c..cbcf45947 100644 --- a/internal/controller/promotions/promotions.go +++ b/internal/controller/promotions/promotions.go @@ -497,29 +497,31 @@ func (r *reconciler) promote( Charts: targetFreight.Charts, Origin: targetFreight.Origin, } - targetFreightCol := r.buildTargetFreightCollection(ctx, targetFreightRef, stage) - newStatus, nextFreight, err := - r.promoMechanisms.Promote(ctx, stage, &promo, targetFreightCol.References()) - if err != nil { + // Make a deep copy of the Promotion to pass to the promotion mechanisms, + // which may modify its status. + workingPromo := promo.DeepCopy() + workingPromo.Status.Freight = &targetFreightRef + workingPromo.Status.FreightCollection = r.buildTargetFreightCollection( + ctx, + targetFreightRef, + stage, + ) + + if err := r.promoMechanisms.Promote(ctx, stage, workingPromo); err != nil { return nil, err } - newStatus.Freight = &targetFreightRef - newStatus.FreightCollection = &kargoapi.FreightCollection{} - for _, freightRef := range nextFreight { - newStatus.FreightCollection.UpdateOrPush(freightRef) - } - logger.Debug("promotion", "phase", newStatus.Phase) + logger.Debug("promotion", "phase", workingPromo.Status.Phase) - if newStatus.Phase == kargoapi.PromotionPhaseSucceeded { + if workingPromo.Status.Phase == kargoapi.PromotionPhaseSucceeded { // Trigger re-verification of the Stage if the promotion succeeded and // this is a re-promotion of the same Freight. current := stage.Status.FreightHistory.Current() if current != nil && current.VerificationHistory.Current() != nil { for _, f := range current.Freight { if f.Name == targetFreight.Name { - if err = kargoapi.ReverifyStageFreight( + if err := kargoapi.ReverifyStageFreight( ctx, r.kargoClient, types.NamespacedName{ @@ -537,7 +539,7 @@ func (r *reconciler) promote( } } - return newStatus, nil + return &workingPromo.Status, nil } // buildTargetFreightCollection constructs a FreightCollection that contains all