diff --git a/api/v1alpha1/helpers.go b/api/v1alpha1/helpers.go index 19df3dd6b4..e4c03c81f0 100644 --- a/api/v1alpha1/helpers.go +++ b/api/v1alpha1/helpers.go @@ -8,6 +8,26 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +func ClearAnnotations(ctx context.Context, c client.Client, obj client.Object, keys ...string) error { + if len(keys) == 0 { + return nil + } + + patchBytes := []byte(`{"metadata":{"annotations":{`) + for i, key := range keys { + if i > 0 { + patchBytes = append(patchBytes, ',') + } + patchBytes = append(patchBytes, fmt.Sprintf(`"%s":null`, key)...) + } + patchBytes = append(patchBytes, "}}}"...) + patch := client.RawPatch(types.MergePatchType, patchBytes) + if err := c.Patch(ctx, obj, patch); err != nil { + return fmt.Errorf("patch annotation: %w", err) + } + return nil +} + func patchAnnotation(ctx context.Context, c client.Client, obj client.Object, key, value string) error { patchBytes := []byte( fmt.Sprintf( @@ -22,17 +42,3 @@ func patchAnnotation(ctx context.Context, c client.Client, obj client.Object, ke } return nil } - -func clearObjectAnnotation( - ctx context.Context, - c client.Client, - obj client.Object, - annotationKey string, -) error { - patchBytes := []byte(fmt.Sprintf(`{"metadata":{"annotations":{"%s":null}}}`, annotationKey)) - patch := client.RawPatch(types.MergePatchType, patchBytes) - if err := c.Patch(ctx, obj, patch); err != nil { - return fmt.Errorf("patch annotation: %w", err) - } - return nil -} diff --git a/api/v1alpha1/helpers_test.go b/api/v1alpha1/helpers_test.go index 9f24d8a2df..d6b1c84a79 100644 --- a/api/v1alpha1/helpers_test.go +++ b/api/v1alpha1/helpers_test.go @@ -11,6 +11,128 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +func TestClearAnnotations(t *testing.T) { + scheme := k8sruntime.NewScheme() + require.NoError(t, SchemeBuilder.AddToScheme(scheme)) + + t.Parallel() + newFakeClient := func(obj ...client.Object) client.Client { + return fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(obj...). + Build() + } + + testCases := []struct{ + name string + client client.Client + obj client.Object + keys []string + assertions func(*testing.T, client.Object, error) + }{ + { + name: "no keys", + client: newFakeClient(), + obj: nil, + keys: nil, + assertions: func(t *testing.T, _ client.Object, err error) { + require.NoError(t, err) + }, + }, + { + name: "no annotations", + client: newFakeClient(&Stage{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "stage", + }, + }), + obj: &Stage{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "stage", + }, + }, + keys: []string{"key"}, + assertions: func(t *testing.T, _ client.Object, err error) { + require.NoError(t, err) + }, + }, + { + name: "not found", + client: newFakeClient(), + obj: &Stage{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "stage", + }, + }, + keys: []string{"key"}, + assertions: func(t *testing.T, _ client.Object, err error) { + require.ErrorContains(t, err, "patch annotation") + }, + }, + { + name: "clear one", + client: newFakeClient(&Stage{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "stage", + Annotations: map[string]string{ + "key1": "value1", + "key2": "value2", + }, + }, + }), + obj: &Stage{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "stage", + }, + }, + keys: []string{"key1"}, + assertions: func(t *testing.T, obj client.Object, err error) { + require.NoError(t, err) + require.Contains(t, obj.GetAnnotations(), "key2") + require.NotContains(t, obj.GetAnnotations(), "key1") + }, + }, + { + name: "clear two", + client: newFakeClient(&Stage{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "stage", + Annotations: map[string]string{ + "key1": "value1", + "key2": "value2", + }, + }, + }), + obj: &Stage{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "stage", + }, + }, + keys: []string{"key1", "key2"}, + assertions: func(t *testing.T, obj client.Object, err error) { + require.NoError(t, err) + require.NotContains(t, obj.GetAnnotations(), "key2") + require.NotContains(t, obj.GetAnnotations(), "key1") + }, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + err := ClearAnnotations(context.TODO(), tc.client, tc.obj, tc.keys...) + tc.assertions(t, tc.obj, err) + }) + } +} + func Test_patchAnnotation(t *testing.T) { scheme := k8sruntime.NewScheme() require.NoError(t, SchemeBuilder.AddToScheme(scheme)) diff --git a/api/v1alpha1/promotion_helpers.go b/api/v1alpha1/promotion_helpers.go index cfeb5b0072..e0604bb419 100644 --- a/api/v1alpha1/promotion_helpers.go +++ b/api/v1alpha1/promotion_helpers.go @@ -53,25 +53,3 @@ func RefreshPromotion( } return promo, nil } - -// ClearPromotionRefresh is called by the Promotion controller to clear the refresh -// annotation on the Promotion (if present). -func ClearPromotionRefresh( - ctx context.Context, - c client.Client, - promo *Promotion, -) error { - if promo.Annotations == nil { - return nil - } - if _, ok := promo.Annotations[AnnotationKeyRefresh]; !ok { - return nil - } - newPromo := Promotion{ - ObjectMeta: metav1.ObjectMeta{ - Name: promo.Name, - Namespace: promo.Namespace, - }, - } - return clearObjectAnnotation(ctx, c, &newPromo, AnnotationKeyRefresh) -} diff --git a/api/v1alpha1/stage_helpers.go b/api/v1alpha1/stage_helpers.go index a324d9012b..41fe067475 100644 --- a/api/v1alpha1/stage_helpers.go +++ b/api/v1alpha1/stage_helpers.go @@ -55,30 +55,6 @@ func RefreshStage( return stage, nil } -// ClearStageRefresh is called by the Stage controller to clear the refresh -// annotation on the Stage (if present). A client (e.g. UI) who requested a -// Stage refresh, can wait until the annotation is cleared, to understand that -// the controller successfully reconciled the Stage after the refresh request. -func ClearStageRefresh( - ctx context.Context, - c client.Client, - stage *Stage, -) error { - if stage.Annotations == nil { - return nil - } - if _, ok := stage.Annotations[AnnotationKeyRefresh]; !ok { - return nil - } - newStage := Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: stage.Name, - Namespace: stage.Namespace, - }, - } - return clearObjectAnnotation(ctx, c, &newStage, AnnotationKeyRefresh) -} - // ReverifyStageFreight forces reconfirmation of the verification of the // Freight associated with a Stage by setting an AnnotationKeyReverify // annotation on the Stage, causing the controller to rerun the verification. @@ -111,33 +87,6 @@ func ReverifyStageFreight( return patchAnnotation(ctx, c, stage, AnnotationKeyReverify, curFreight.VerificationInfo.ID) } -// ClearStageReverify is called by the Stage controller to clear the -// AnnotationKeyReverify annotation on the Stage (if present). A client (e.g. -// UI) who requested a reconfirmation of the Stage verification, can wait -// until the annotation is cleared, to understand that the controller -// acknowledged the reverification request. -func ClearStageReverify( - ctx context.Context, - c client.Client, - stage *Stage, -) error { - if stage.Annotations == nil { - return nil - } - - if _, ok := stage.Annotations[AnnotationKeyReverify]; !ok { - return nil - } - - newStage := Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: stage.Name, - Namespace: stage.Namespace, - }, - } - return clearObjectAnnotation(ctx, c, &newStage, AnnotationKeyReverify) -} - // AbortStageFreightVerification forces aborting the verification of the // Freight associated with a Stage by setting an AnnotationKeyAbort // annotation on the Stage, causing the controller to abort the verification. @@ -174,30 +123,3 @@ func AbortStageFreightVerification( return patchAnnotation(ctx, c, stage, AnnotationKeyAbort, curFreight.VerificationInfo.ID) } - -// ClearStageAbort is called by the Stage controller to clear the -// AnnotationKeyAbort annotation on the Stage (if present). A client (e.g. -// UI) who requested an abort of the Stage verification, can wait -// until the annotation is cleared, to understand that the controller -// acknowledged the abort request. -func ClearStageAbort( - ctx context.Context, - c client.Client, - stage *Stage, -) error { - if stage.Annotations == nil { - return nil - } - - if _, ok := stage.Annotations[AnnotationKeyAbort]; !ok { - return nil - } - - newStage := Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: stage.Name, - Namespace: stage.Namespace, - }, - } - return clearObjectAnnotation(ctx, c, &newStage, AnnotationKeyAbort) -} diff --git a/api/v1alpha1/stage_helpers_test.go b/api/v1alpha1/stage_helpers_test.go index e023d87bd0..15833468c3 100644 --- a/api/v1alpha1/stage_helpers_test.go +++ b/api/v1alpha1/stage_helpers_test.go @@ -168,84 +168,6 @@ func TestReverifyStageFreight(t *testing.T) { }) } -func TestClearStageReverify(t *testing.T) { - scheme := k8sruntime.NewScheme() - require.NoError(t, SchemeBuilder.AddToScheme(scheme)) - - t.Run("no annotations", func(t *testing.T) { - c := fake.NewClientBuilder().WithScheme(scheme).WithObjects( - &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - }, - }, - ).Build() - - err := ClearStageReverify(context.TODO(), c, &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - }, - }) - require.NoError(t, err) - }) - - t.Run("no reconfirm annotation", func(t *testing.T) { - c := fake.NewClientBuilder().WithScheme(scheme).WithObjects( - &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - Annotations: map[string]string{}, - }, - }, - ).Build() - - err := ClearStageReverify(context.TODO(), c, &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - Annotations: map[string]string{}, - }, - }) - require.NoError(t, err) - }) - - t.Run("success", func(t *testing.T) { - c := fake.NewClientBuilder().WithScheme(scheme).WithObjects( - &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - Annotations: map[string]string{ - AnnotationKeyReverify: "fake-id", - }, - }, - }, - ).Build() - - err := ClearStageReverify(context.TODO(), c, &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - Annotations: map[string]string{ - AnnotationKeyReverify: "fake-id", - }, - }, - }) - require.NoError(t, err) - - stage, err := GetStage(context.TODO(), c, types.NamespacedName{ - Namespace: "fake-namespace", - Name: "fake-stage", - }) - require.NoError(t, err) - _, ok := stage.Annotations[AnnotationKeyReverify] - require.False(t, ok) - }) -} - func TestAbortStageFreightVerification(t *testing.T) { scheme := k8sruntime.NewScheme() require.NoError(t, SchemeBuilder.AddToScheme(scheme)) @@ -383,81 +305,3 @@ func TestAbortStageFreightVerification(t *testing.T) { require.Equal(t, "fake-id", stage.Annotations[AnnotationKeyAbort]) }) } - -func TestClearStageAbort(t *testing.T) { - scheme := k8sruntime.NewScheme() - require.NoError(t, SchemeBuilder.AddToScheme(scheme)) - - t.Run("no annotations", func(t *testing.T) { - c := fake.NewClientBuilder().WithScheme(scheme).WithObjects( - &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - }, - }, - ).Build() - - err := ClearStageAbort(context.TODO(), c, &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - }, - }) - require.NoError(t, err) - }) - - t.Run("no abort annotation", func(t *testing.T) { - c := fake.NewClientBuilder().WithScheme(scheme).WithObjects( - &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - Annotations: map[string]string{}, - }, - }, - ).Build() - - err := ClearStageAbort(context.TODO(), c, &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - Annotations: map[string]string{}, - }, - }) - require.NoError(t, err) - }) - - t.Run("success", func(t *testing.T) { - c := fake.NewClientBuilder().WithScheme(scheme).WithObjects( - &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - Annotations: map[string]string{ - AnnotationKeyAbort: "fake-id", - }, - }, - }, - ).Build() - - err := ClearStageAbort(context.TODO(), c, &Stage{ - ObjectMeta: metav1.ObjectMeta{ - Name: "fake-stage", - Namespace: "fake-namespace", - Annotations: map[string]string{ - AnnotationKeyAbort: "fake-id", - }, - }, - }) - require.NoError(t, err) - - stage, err := GetStage(context.TODO(), c, types.NamespacedName{ - Namespace: "fake-namespace", - Name: "fake-stage", - }) - require.NoError(t, err) - _, ok := stage.Annotations[AnnotationKeyAbort] - require.False(t, ok) - }) -} diff --git a/api/v1alpha1/warehouse_helpers.go b/api/v1alpha1/warehouse_helpers.go index 558bd92342..71e1d5494c 100644 --- a/api/v1alpha1/warehouse_helpers.go +++ b/api/v1alpha1/warehouse_helpers.go @@ -59,27 +59,3 @@ func RefreshWarehouse( } return warehouse, nil } - -// ClearWarehouseRefresh is called by the Warehouse controller to clear the refresh -// annotation on the Warehouse (if present). A client (e.g. UI) who requested a -// Warehouse refresh, can wait until the annotation is cleared, to understand that -// the controller successfully reconciled the Warehouse after the refresh request. -func ClearWarehouseRefresh( - ctx context.Context, - c client.Client, - wh *Warehouse, -) error { - if wh.Annotations == nil { - return nil - } - if _, ok := wh.Annotations[AnnotationKeyRefresh]; !ok { - return nil - } - newWh := Warehouse{ - ObjectMeta: metav1.ObjectMeta{ - Name: wh.Name, - Namespace: wh.Namespace, - }, - } - return clearObjectAnnotation(ctx, c, &newWh, AnnotationKeyRefresh) -} diff --git a/internal/controller/promotions/promotions.go b/internal/controller/promotions/promotions.go index 394cda8693..a59d4a22a7 100644 --- a/internal/controller/promotions/promotions.go +++ b/internal/controller/promotions/promotions.go @@ -237,9 +237,15 @@ func (r *reconciler) Reconcile( if err != nil { logger.Errorf("error updating Promotion status: %s", err) } - if clearRefreshErr := kargoapi.ClearPromotionRefresh(ctx, r.kargoClient, promo); clearRefreshErr != nil { + if clearRefreshErr := kargoapi.ClearAnnotations( + ctx, + r.kargoClient, + promo, + kargoapi.AnnotationKeyRefresh, + ); clearRefreshErr != nil { logger.Errorf("error clearing Promotion refresh annotation: %s", clearRefreshErr) } + if err != nil { // Controller runtime automatically gives us a progressive backoff if err is // not nil diff --git a/internal/controller/stages/stages.go b/internal/controller/stages/stages.go index 5e6676c203..653e8ac9f8 100644 --- a/internal/controller/stages/stages.go +++ b/internal/controller/stages/stages.go @@ -2,6 +2,7 @@ package stages import ( "context" + "errors" "fmt" "sort" "time" @@ -554,33 +555,23 @@ func (r *reconciler) Reconcile( if updateErr != nil { logger.Errorf("error updating Stage status: %s", updateErr) } - clearRefreshErr := kargoapi.ClearStageRefresh(ctx, r.kargoClient, stage) - if clearRefreshErr != nil { - logger.Errorf("error clearing Stage refresh annotation: %s", clearRefreshErr) - } - clearReconfirmErr := kargoapi.ClearStageReverify(ctx, r.kargoClient, stage) - if clearReconfirmErr != nil { - logger.Errorf("error clearing Stage reconfirm annotation: %s", clearReconfirmErr) - } - clearAbortErr := kargoapi.ClearStageAbort(ctx, r.kargoClient, stage) - if clearAbortErr != nil { - logger.Errorf("error clearing Stage abort annotation: %s", clearAbortErr) + clearErr := kargoapi.ClearAnnotations( + ctx, + r.kargoClient, + stage, + kargoapi.AnnotationKeyRefresh, + kargoapi.AnnotationKeyReverify, + kargoapi.AnnotationKeyAbort, + ) + if clearErr != nil { + logger.Errorf("error clearing Stage annotations: %s", clearErr) } // If we had no error, but couldn't update, then we DO have an error. But we // do it this way so that a failure to update is never counted as THE failure // when something else more serious occurred first. if err == nil { - err = updateErr - } - if err == nil { - err = clearRefreshErr - } - if err == nil { - err = clearReconfirmErr - } - if err == nil { - err = clearAbortErr + err = errors.Join(updateErr, clearErr) } logger.Debug("done reconciling Stage") diff --git a/internal/controller/warehouses/warehouses.go b/internal/controller/warehouses/warehouses.go index 3040b9e889..ccbd53bcb0 100644 --- a/internal/controller/warehouses/warehouses.go +++ b/internal/controller/warehouses/warehouses.go @@ -197,7 +197,12 @@ func (r *reconciler) Reconcile( if updateErr != nil { logger.Errorf("error updating Warehouse status: %s", updateErr) } - if clearRefreshErr := kargoapi.ClearWarehouseRefresh(ctx, r.client, warehouse); clearRefreshErr != nil { + if clearRefreshErr := kargoapi.ClearAnnotations( + ctx, + r.client, + warehouse, + kargoapi.AnnotationKeyRefresh, + ); clearRefreshErr != nil { logger.Errorf("error clearing Warehouse refresh annotation: %s", clearRefreshErr) }