From 4666209ac245e497cad149f8ae8e6fa5b6cbccd0 Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 15 Oct 2025 15:27:48 +0200 Subject: [PATCH 01/15] feat: Introduce Deletion Service --- .../deletion/deletion_service.go | 46 ++++ .../deletion/deletion_service_test.go | 237 ++++++++++++++++++ .../mandatorymodule/deletion/errors.go | 8 + .../deletion/usecases/delete_manifests.go | 38 +++ .../usecases/delete_manifests_test.go | 119 +++++++++ .../deletion/usecases/ensure_finalizer.go | 30 +++ .../usecases/ensure_finalizer_test.go | 86 +++++++ .../deletion/usecases/remove_finalizer.go | 30 +++ .../usecases/remove_finalizer_test.go | 87 +++++++ .../deletion/usecases/skip_non_deleting.go | 23 ++ .../usecases/skip_non_deleting_test.go | 48 ++++ .../deletion/usecases/skip_non_mandatory.go | 24 ++ .../usecases/skip_non_mandatory_test.go | 47 ++++ 13 files changed, 823 insertions(+) create mode 100644 internal/service/mandatorymodule/deletion/deletion_service.go create mode 100644 internal/service/mandatorymodule/deletion/deletion_service_test.go create mode 100644 internal/service/mandatorymodule/deletion/errors.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/delete_manifests.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go create mode 100644 internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go diff --git a/internal/service/mandatorymodule/deletion/deletion_service.go b/internal/service/mandatorymodule/deletion/deletion_service.go new file mode 100644 index 0000000000..74eb24c186 --- /dev/null +++ b/internal/service/mandatorymodule/deletion/deletion_service.go @@ -0,0 +1,46 @@ +package deletion + +import ( + "context" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" +) + +type UseCase interface { + ShouldExecute(context.Context, *v1beta2.ModuleReleaseMeta) (bool, error) + Execute(context.Context, *v1beta2.ModuleReleaseMeta) error +} + +type Service struct { + orderedUseCases []UseCase +} + +func NewService(skipNonMandatory UseCase, + ensureFinalizer UseCase, + skipNonDeleting UseCase, + deleteManifests UseCase, + removeFinalizer UseCase, +) *Service { + return &Service{ + orderedUseCases: []UseCase{ + skipNonMandatory, // if returns deletion.ErrMrmNotMandatory, controller should not requeue + ensureFinalizer, + skipNonDeleting, // if returns deletion.ErrMrmNotInDeletingState, controller should not requeue + deleteManifests, + removeFinalizer, + }, + } +} + +func (s *Service) HandleDeletion(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { + for _, useCase := range s.orderedUseCases { + shouldExecute, err := useCase.ShouldExecute(ctx, mrm) + if err != nil { + return err + } + if shouldExecute { + return useCase.Execute(ctx, mrm) + } + } + return nil +} diff --git a/internal/service/mandatorymodule/deletion/deletion_service_test.go b/internal/service/mandatorymodule/deletion/deletion_service_test.go new file mode 100644 index 0000000000..51d48f588d --- /dev/null +++ b/internal/service/mandatorymodule/deletion/deletion_service_test.go @@ -0,0 +1,237 @@ +package deletion_test + +import ( + "context" + "errors" + "testing" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" + "github.com/stretchr/testify/require" +) + +func TestDeletionService_Execute(t *testing.T) { + t.Parallel() + + var executionOrder []string + + skipNonMandatoryStub := &SkipNonMandatoryStub{StubName: "skipNonMandatory", ExecutionOrder: &executionOrder} + ensureFinalizerStub := &EnsureFinalizerStub{StubName: "ensureFinalizer", ExecutionOrder: &executionOrder} + skipNonDeletingStub := &SkipNonDeletingStub{StubName: "skipNonDeleting", ExecutionOrder: &executionOrder} + deleteManifestsStub := &DeleteManifestsStub{StubName: "deleteManifests", ExecutionOrder: &executionOrder} + removeFinalizerStub := &RemoveFinalizerStub{StubName: "removeFinalizer", ExecutionOrder: &executionOrder} + + service := deletion.NewService( + skipNonMandatoryStub, + ensureFinalizerStub, + skipNonDeletingStub, + deleteManifestsStub, + removeFinalizerStub, + ) + mrm := &v1beta2.ModuleReleaseMeta{} + + for i := 0; i < 5; i++ { + err := service.HandleDeletion(context.Background(), mrm) + require.NoError(t, err) + } + + expectedOrder := []string{ + "skipNonMandatory", + "ensureFinalizer", + "skipNonDeleting", + "deleteManifests", + "removeFinalizer", + } + require.Equal(t, expectedOrder, executionOrder) + + initialLen := len(executionOrder) + err := service.HandleDeletion(context.Background(), mrm) + require.NoError(t, err) + require.Equal(t, initialLen, len(executionOrder)) + + require.True(t, skipNonMandatoryStub.ShouldExecuteCalled) + require.True(t, skipNonMandatoryStub.ExecuteCalled) + require.True(t, ensureFinalizerStub.ShouldExecuteCalled) + require.True(t, ensureFinalizerStub.ExecuteCalled) + require.True(t, skipNonDeletingStub.ShouldExecuteCalled) + require.True(t, skipNonDeletingStub.ExecuteCalled) + require.True(t, deleteManifestsStub.ShouldExecuteCalled) + require.True(t, deleteManifestsStub.ExecuteCalled) + require.True(t, removeFinalizerStub.ShouldExecuteCalled) + require.True(t, removeFinalizerStub.ExecuteCalled) +} + +func TestDeletionService_Execute_ErrorPropagation(t *testing.T) { + t.Parallel() + + var executionOrder []string + + skipNonMandatoryErrorStub := &SkipNonMandatoryErrorStub{ + StubName: "skipNonMandatory", + ExecutionOrder: &executionOrder, + } + ensureFinalizerStub := &EnsureFinalizerStub{StubName: "ensureFinalizer", ExecutionOrder: &executionOrder} + skipNonDeletingStub := &SkipNonDeletingStub{StubName: "skipNonDeleting", ExecutionOrder: &executionOrder} + deleteManifestsStub := &DeleteManifestsStub{StubName: "deleteManifests", ExecutionOrder: &executionOrder} + removeFinalizerStub := &RemoveFinalizerStub{StubName: "removeFinalizer", ExecutionOrder: &executionOrder} + + service := deletion.NewService( + skipNonMandatoryErrorStub, + ensureFinalizerStub, + skipNonDeletingStub, + deleteManifestsStub, + removeFinalizerStub, + ) + mrm := &v1beta2.ModuleReleaseMeta{} + + for i := 0; i < 5; i++ { + err := service.HandleDeletion(context.Background(), mrm) + require.Error(t, err) + require.Contains(t, err.Error(), "skipNonMandatory failed") + } + + expectedOrder := []string{ + "skipNonMandatory", + "skipNonMandatory", + "skipNonMandatory", + "skipNonMandatory", + "skipNonMandatory", + } + require.Equal(t, expectedOrder, executionOrder) +} + +// Stubs for the use cases to track execution order and calls + +type SkipNonMandatoryStub struct { + ShouldExecuteCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string +} + +func (stub *SkipNonMandatoryStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.ShouldExecuteCalled { + return false, nil + } + stub.ShouldExecuteCalled = true + return true, nil +} +func (stub *SkipNonMandatoryStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { + stub.ExecuteCalled = true + if stub.ExecutionOrder != nil { + *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) + } + return nil +} + +type EnsureFinalizerStub struct { + ShouldExecuteCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string +} + +func (stub *EnsureFinalizerStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.ShouldExecuteCalled { + return false, nil + } + stub.ShouldExecuteCalled = true + return true, nil +} + +func (stub *EnsureFinalizerStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { + stub.ExecuteCalled = true + if stub.ExecutionOrder != nil { + *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) + } + return nil +} + +type SkipNonDeletingStub struct { + ShouldExecuteCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string +} + +func (stub *SkipNonDeletingStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.ShouldExecuteCalled { + return false, nil + } + stub.ShouldExecuteCalled = true + return true, nil +} + +func (stub *SkipNonDeletingStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { + stub.ExecuteCalled = true + if stub.ExecutionOrder != nil { + *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) + } + return nil +} + +type DeleteManifestsStub struct { + ShouldExecuteCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string +} + +func (stub *DeleteManifestsStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.ShouldExecuteCalled { + return false, nil + } + stub.ShouldExecuteCalled = true + return true, nil +} + +func (stub *DeleteManifestsStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { + stub.ExecuteCalled = true + if stub.ExecutionOrder != nil { + *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) + } + return nil +} + +type RemoveFinalizerStub struct { + ShouldExecuteCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string +} + +func (stub *RemoveFinalizerStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.ShouldExecuteCalled { + return false, nil + } + stub.ShouldExecuteCalled = true + return true, nil +} + +func (stub *RemoveFinalizerStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { + stub.ExecuteCalled = true + if stub.ExecutionOrder != nil { + *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) + } + return nil +} + +type SkipNonMandatoryErrorStub struct { + ShouldExecuteCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string +} + +func (stub *SkipNonMandatoryErrorStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + stub.ShouldExecuteCalled = true + return true, nil +} + +func (stub *SkipNonMandatoryErrorStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { + stub.ExecuteCalled = true + if stub.ExecutionOrder != nil { + *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) + } + return errors.New("skipNonMandatory failed") +} diff --git a/internal/service/mandatorymodule/deletion/errors.go b/internal/service/mandatorymodule/deletion/errors.go new file mode 100644 index 0000000000..0a7fac72b0 --- /dev/null +++ b/internal/service/mandatorymodule/deletion/errors.go @@ -0,0 +1,8 @@ +package deletion + +import "errors" + +var ( + ErrMrmNotMandatory = errors.New("ModuleReleaseMeta is not a mandatory module") + ErrMrmNotInDeletingState = errors.New("ModuleReleaseMeta not in deleting state") +) diff --git a/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go b/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go new file mode 100644 index 0000000000..92395e07d6 --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go @@ -0,0 +1,38 @@ +package usecases + +import ( + "context" + "fmt" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +type ManifestRepo interface { + ListAllForModule(ctx context.Context, moduleName string) ([]apimetav1.PartialObjectMetadata, error) + DeleteAllForModule(ctx context.Context, moduleName string) error +} + +// DeleteManifests is responsible for deleting all manifests associated with a ModuleReleaseMeta. +type DeleteManifests struct { + repo ManifestRepo +} + +func NewDeleteManifests(repo ManifestRepo) *DeleteManifests { + return &DeleteManifests{repo: repo} +} + +func (d *DeleteManifests) ShouldExecute(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { + manifests, err := d.repo.ListAllForModule(ctx, mrm.Name) + if err != nil { + return false, fmt.Errorf("failed to list manifests for module %s: %w", mrm.Name, err) + } + return len(manifests) > 0, nil +} + +func (d *DeleteManifests) Execute(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { + if err := d.repo.DeleteAllForModule(ctx, mrm.Name); err != nil { + return fmt.Errorf("failed to delete manifests for module %s: %w", mrm.Name, err) + } + return nil +} diff --git a/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go b/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go new file mode 100644 index 0000000000..18a9601d10 --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go @@ -0,0 +1,119 @@ +package usecases_test + +import ( + "context" + "errors" + "testing" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" + "github.com/kyma-project/lifecycle-manager/pkg/testutils/random" + "github.com/stretchr/testify/require" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +type MockManifestRepo struct { + ListAllForModuleCalled bool + DeleteAllForModuleCalled bool + ListAllForModuleError error + DeleteAllForModuleError error + CalledWithModuleName string + ManifestsToReturn []apimetav1.PartialObjectMetadata +} + +func (m *MockManifestRepo) ListAllForModule(_ context.Context, moduleName string) ( + []apimetav1.PartialObjectMetadata, error) { + m.ListAllForModuleCalled = true + m.CalledWithModuleName = moduleName + return m.ManifestsToReturn, m.ListAllForModuleError +} + +func (m *MockManifestRepo) DeleteAllForModule(_ context.Context, moduleName string) error { + m.DeleteAllForModuleCalled = true + m.CalledWithModuleName = moduleName + return m.DeleteAllForModuleError +} + +func TestDeleteManifests_WithManifests(t *testing.T) { + t.Parallel() + + mockRepo := &MockManifestRepo{ + ManifestsToReturn: []apimetav1.PartialObjectMetadata{ + {ObjectMeta: metav1.ObjectMeta{Name: random.Name()}}, + }, + } + deleteManifests := usecases.NewDeleteManifests(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + }, + } + + shouldExecute, err := deleteManifests.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.True(t, shouldExecute) + require.True(t, mockRepo.ListAllForModuleCalled) + require.Equal(t, mrm.Name, mockRepo.CalledWithModuleName) + + executeErr := deleteManifests.Execute(context.Background(), mrm) + require.NoError(t, executeErr) + require.True(t, mockRepo.DeleteAllForModuleCalled) +} + +func TestDeleteManifests_NoManifests(t *testing.T) { + t.Parallel() + + mockRepo := &MockManifestRepo{ + ManifestsToReturn: []apimetav1.PartialObjectMetadata{}, + } + deleteManifests := usecases.NewDeleteManifests(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + }, + } + + shouldExecute, err := deleteManifests.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.False(t, shouldExecute) +} + +func TestDeleteManifests_ListError(t *testing.T) { + t.Parallel() + + expectedErr := errors.New("list error") + mockRepo := &MockManifestRepo{ + ListAllForModuleError: expectedErr, + } + deleteManifests := usecases.NewDeleteManifests(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + }, + } + + shouldExecute, err := deleteManifests.ShouldExecute(context.Background(), mrm) + require.Error(t, err) + require.False(t, shouldExecute) + require.Contains(t, err.Error(), "failed to list manifests for module") +} + +func TestDeleteManifests_DeleteError(t *testing.T) { + t.Parallel() + + expectedErr := errors.New("delete error") + mockRepo := &MockManifestRepo{ + DeleteAllForModuleError: expectedErr, + } + deleteManifests := usecases.NewDeleteManifests(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + }, + } + + executeErr := deleteManifests.Execute(context.Background(), mrm) + require.Error(t, executeErr) + require.Contains(t, executeErr.Error(), "failed to delete manifests for module") +} diff --git a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go new file mode 100644 index 0000000000..317546b262 --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go @@ -0,0 +1,30 @@ +package usecases + +import ( + "context" + + "github.com/kyma-project/lifecycle-manager/api/shared" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +type MrmEnsureFinalizerRepo interface { + EnsureFinalizer(ctx context.Context, moduleName string, finalizer string) error +} + +// EnsureFinalizer is responsible for ensuring that the mandatory finalizer is present on the ModuleReleaseMeta. +type EnsureFinalizer struct { + repo MrmEnsureFinalizerRepo +} + +func NewEnsureFinalizer(repo MrmEnsureFinalizerRepo) *EnsureFinalizer { + return &EnsureFinalizer{repo: repo} +} + +func (e *EnsureFinalizer) ShouldExecute(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { + return !controllerutil.ContainsFinalizer(mrm, shared.MandatoryModuleFinalizer), nil +} + +func (e *EnsureFinalizer) Execute(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { + return e.repo.EnsureFinalizer(ctx, mrm.Name, shared.MandatoryModuleFinalizer) +} diff --git a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go new file mode 100644 index 0000000000..496a9956fc --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go @@ -0,0 +1,86 @@ +package usecases_test + +import ( + "context" + "errors" + "testing" + + "github.com/kyma-project/lifecycle-manager/api/shared" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" + "github.com/kyma-project/lifecycle-manager/pkg/testutils/random" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +type MockMrmEnsureFinalizerRepo struct { + EnsureFinalizerCalled bool + EnsureFinalizerError error + CalledWithModule string + CalledWithFinalizer string +} + +func (m *MockMrmEnsureFinalizerRepo) EnsureFinalizer(_ context.Context, moduleName string, finalizer string) error { + m.EnsureFinalizerCalled = true + m.CalledWithModule = moduleName + m.CalledWithFinalizer = finalizer + return m.EnsureFinalizerError +} + +func TestEnsureFinalizer_WithoutFinalizer(t *testing.T) { + t.Parallel() + + mockRepo := &MockMrmEnsureFinalizerRepo{} + ensureFinalizer := usecases.NewEnsureFinalizer(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + }, + } + + shouldExecute, err := ensureFinalizer.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.True(t, shouldExecute) + + executeErr := ensureFinalizer.Execute(context.Background(), mrm) + require.NoError(t, executeErr) + require.True(t, mockRepo.EnsureFinalizerCalled) + require.Equal(t, mrm.Name, mockRepo.CalledWithModule) + require.Equal(t, shared.MandatoryModuleFinalizer, mockRepo.CalledWithFinalizer) +} + +func TestEnsureFinalizer_WithFinalizer(t *testing.T) { + t.Parallel() + + mockRepo := &MockMrmEnsureFinalizerRepo{} + ensureFinalizer := usecases.NewEnsureFinalizer(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + Finalizers: []string{shared.MandatoryModuleFinalizer}, + }, + } + + shouldExecute, err := ensureFinalizer.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.False(t, shouldExecute) +} + +func TestEnsureFinalizer_RepositoryError(t *testing.T) { + t.Parallel() + + expectedErr := errors.New("repository error") + mockRepo := &MockMrmEnsureFinalizerRepo{ + EnsureFinalizerError: expectedErr, + } + ensureFinalizer := usecases.NewEnsureFinalizer(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + }, + } + + executeErr := ensureFinalizer.Execute(context.Background(), mrm) + require.ErrorIs(t, executeErr, expectedErr) + require.True(t, mockRepo.EnsureFinalizerCalled) +} diff --git a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go new file mode 100644 index 0000000000..0bb05d6711 --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go @@ -0,0 +1,30 @@ +package usecases + +import ( + "context" + + "github.com/kyma-project/lifecycle-manager/api/shared" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +type MrmRemoFinalizerRepo interface { + RemoveFinalizer(ctx context.Context, moduleName string, finalizer string) error +} + +// RemoveFinalizer is responsible for removing the mandatory finalizer from the ModuleReleaseMeta. +type RemoveFinalizer struct { + repo MrmRemoFinalizerRepo +} + +func NewRemoveFinalizer(repo MrmRemoFinalizerRepo) *RemoveFinalizer { + return &RemoveFinalizer{repo: repo} +} + +func (e *RemoveFinalizer) ShouldExecute(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { + return controllerutil.ContainsFinalizer(mrm, shared.MandatoryModuleFinalizer), nil +} + +func (e *RemoveFinalizer) Execute(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { + return e.repo.RemoveFinalizer(ctx, mrm.Name, shared.MandatoryModuleFinalizer) +} diff --git a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go new file mode 100644 index 0000000000..2d307218be --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go @@ -0,0 +1,87 @@ +package usecases_test + +import ( + "context" + "errors" + "testing" + + "github.com/kyma-project/lifecycle-manager/api/shared" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" + "github.com/kyma-project/lifecycle-manager/pkg/testutils/random" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +type MockMrmRemoFinalizerRepo struct { + RemoveFinalizerCalled bool + RemoveFinalizerError error + CalledWithModule string + CalledWithFinalizer string +} + +func (m *MockMrmRemoFinalizerRepo) RemoveFinalizer(_ context.Context, moduleName string, finalizer string) error { + m.RemoveFinalizerCalled = true + m.CalledWithModule = moduleName + m.CalledWithFinalizer = finalizer + return m.RemoveFinalizerError +} + +func TestRemoveFinalizer_WithFinalizer(t *testing.T) { + t.Parallel() + + mockRepo := &MockMrmRemoFinalizerRepo{} + removeFinalizer := usecases.NewRemoveFinalizer(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + Finalizers: []string{shared.MandatoryModuleFinalizer}, + }, + } + + shouldExecute, err := removeFinalizer.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.True(t, shouldExecute) + + executeErr := removeFinalizer.Execute(context.Background(), mrm) + require.NoError(t, executeErr) + require.True(t, mockRepo.RemoveFinalizerCalled) + require.Equal(t, mrm.Name, mockRepo.CalledWithModule) + require.Equal(t, shared.MandatoryModuleFinalizer, mockRepo.CalledWithFinalizer) +} + +func TestRemoveFinalizer_WithoutFinalizer(t *testing.T) { + t.Parallel() + + mockRepo := &MockMrmRemoFinalizerRepo{} + removeFinalizer := usecases.NewRemoveFinalizer(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + }, + } + + shouldExecute, err := removeFinalizer.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.False(t, shouldExecute) +} + +func TestRemoveFinalizer_RepositoryError(t *testing.T) { + t.Parallel() + + expectedErr := errors.New("repository error") + mockRepo := &MockMrmRemoFinalizerRepo{ + RemoveFinalizerError: expectedErr, + } + removeFinalizer := usecases.NewRemoveFinalizer(mockRepo) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: random.Name(), + Finalizers: []string{shared.MandatoryModuleFinalizer}, + }, + } + + executeErr := removeFinalizer.Execute(context.Background(), mrm) + require.ErrorIs(t, executeErr, expectedErr) + require.True(t, mockRepo.RemoveFinalizerCalled) +} diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go new file mode 100644 index 0000000000..51f643371d --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go @@ -0,0 +1,23 @@ +package usecases + +import ( + "context" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" +) + +// SkipNonDeleting is a use case that skips ModuleReleaseMetas that are not in deleting state. +type SkipNonDeleting struct{} + +func NewSkipNonDeleting() *SkipNonDeleting { + return &SkipNonDeleting{} +} + +func (s *SkipNonDeleting) ShouldExecute(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { + return mrm.DeletionTimestamp.IsZero(), nil +} + +func (s *SkipNonDeleting) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { + return deletion.ErrMrmNotInDeletingState +} diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go new file mode 100644 index 0000000000..bbef9fbc2e --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go @@ -0,0 +1,48 @@ +package usecases_test + +import ( + "context" + "testing" + "time" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestSkipNonDeleting_NotDeleting(t *testing.T) { + t.Parallel() + + skipNonDeleting := usecases.NewSkipNonDeleting() + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-module", + }, + } + + shouldExecute, err := skipNonDeleting.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.True(t, shouldExecute) + + executeErr := skipNonDeleting.Execute(context.Background(), mrm) + require.ErrorIs(t, executeErr, deletion.ErrMrmNotInDeletingState) +} + +func TestSkipNonDeleting_IsDeleting(t *testing.T) { + t.Parallel() + + skipNonDeleting := usecases.NewSkipNonDeleting() + now := metav1.NewTime(time.Now()) + mrm := &v1beta2.ModuleReleaseMeta{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-module", + DeletionTimestamp: &now, + }, + } + + shouldExecute, err := skipNonDeleting.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.False(t, shouldExecute) +} diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go new file mode 100644 index 0000000000..ee56c77981 --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go @@ -0,0 +1,24 @@ +package usecases + +import ( + "context" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" +) + +// SkipNonMandatory is a use case that skips ModuleReleaseMetas that are not mandatory. +type SkipNonMandatory struct { +} + +func NewSkipNonMandatory() *SkipNonMandatory { + return &SkipNonMandatory{} +} + +func (s *SkipNonMandatory) ShouldExecute(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { + return mrm.Spec.Mandatory == nil || mrm.Spec.Mandatory.Version == "", nil +} + +func (s *SkipNonMandatory) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { + return deletion.ErrMrmNotMandatory +} diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go new file mode 100644 index 0000000000..82cd866b0d --- /dev/null +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go @@ -0,0 +1,47 @@ +package usecases_test + +import ( + "context" + "testing" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" + "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" + "github.com/stretchr/testify/require" +) + +func TestSkipNonMandatory_WithNonMandatoryModule(t *testing.T) { + t.Parallel() + + skipNonMandatory := usecases.NewSkipNonMandatory() + mrm := &v1beta2.ModuleReleaseMeta{ + Spec: v1beta2.ModuleReleaseMetaSpec{ + Mandatory: nil, + }, + } + + shouldExecute, err := skipNonMandatory.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.True(t, shouldExecute) + + executeErr := skipNonMandatory.Execute(context.Background(), mrm) + require.Error(t, executeErr) + require.Equal(t, deletion.ErrMrmNotMandatory, executeErr) +} + +func TestSkipNonMandatory_WithMandatoryModule(t *testing.T) { + t.Parallel() + + skipNonMandatory := usecases.NewSkipNonMandatory() + mrm := &v1beta2.ModuleReleaseMeta{ + Spec: v1beta2.ModuleReleaseMetaSpec{ + Mandatory: &v1beta2.Mandatory{ + Version: "1.0.0", + }, + }, + } + + shouldExecute, err := skipNonMandatory.ShouldExecute(context.Background(), mrm) + require.NoError(t, err) + require.False(t, shouldExecute) +} From f7185d521f423df537b16ccccf3fd0f750d8fcb8 Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 15 Oct 2025 15:30:10 +0200 Subject: [PATCH 02/15] chore: Update Unit Test Coverage --- unit-test-coverage-lifecycle-manager.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/unit-test-coverage-lifecycle-manager.yaml b/unit-test-coverage-lifecycle-manager.yaml index 3fa5e36a79..995f1952ca 100644 --- a/unit-test-coverage-lifecycle-manager.yaml +++ b/unit-test-coverage-lifecycle-manager.yaml @@ -43,6 +43,8 @@ packages: internal/service/watcher/gateway: 94 internal/service/watcher/resources: 82 internal/service/skrclient: 35 + internal/service/mandatorymodule/deletion: 100 + internal/service/mandatorymodule/deletion/usecases: 100 internal/util/collections: 87 pkg/module/sync: 10 From 1cf441bde7ebba095de540bd4dea2d60303ea73b Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 15 Oct 2025 15:34:50 +0200 Subject: [PATCH 03/15] test: Add Final Test for Deletion Service --- .../deletion/deletion_service_test.go | 67 ++++++++++++++++++- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/internal/service/mandatorymodule/deletion/deletion_service_test.go b/internal/service/mandatorymodule/deletion/deletion_service_test.go index 51d48f588d..425675382d 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service_test.go +++ b/internal/service/mandatorymodule/deletion/deletion_service_test.go @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestDeletionService_Execute(t *testing.T) { +func TestDeletionService_HandleDeletion_ExecutionOrder(t *testing.T) { t.Parallel() var executionOrder []string @@ -61,7 +61,7 @@ func TestDeletionService_Execute(t *testing.T) { require.True(t, removeFinalizerStub.ExecuteCalled) } -func TestDeletionService_Execute_ErrorPropagation(t *testing.T) { +func TestDeletionService_HandleDeletion_ErrorPropagation(t *testing.T) { t.Parallel() var executionOrder []string @@ -100,6 +100,47 @@ func TestDeletionService_Execute_ErrorPropagation(t *testing.T) { require.Equal(t, expectedOrder, executionOrder) } +func TestDeletionService_HandleDeletion_ShouldExecuteError(t *testing.T) { + t.Parallel() + + var executionOrder []string + + skipNonMandatoryShouldExecuteErrorStub := &SkipNonMandatoryShouldExecuteErrorStub{ + StubName: "skipNonMandatory", + ExecutionOrder: &executionOrder, + } + ensureFinalizerStub := &EnsureFinalizerStub{StubName: "ensureFinalizer", ExecutionOrder: &executionOrder} + skipNonDeletingStub := &SkipNonDeletingStub{StubName: "skipNonDeleting", ExecutionOrder: &executionOrder} + deleteManifestsStub := &DeleteManifestsStub{StubName: "deleteManifests", ExecutionOrder: &executionOrder} + removeFinalizerStub := &RemoveFinalizerStub{StubName: "removeFinalizer", ExecutionOrder: &executionOrder} + + service := deletion.NewService( + skipNonMandatoryShouldExecuteErrorStub, + ensureFinalizerStub, + skipNonDeletingStub, + deleteManifestsStub, + removeFinalizerStub, + ) + mrm := &v1beta2.ModuleReleaseMeta{} + + err := service.HandleDeletion(context.Background(), mrm) + require.Error(t, err) + require.Contains(t, err.Error(), "shouldExecute failed") + + require.Empty(t, executionOrder) + + require.True(t, skipNonMandatoryShouldExecuteErrorStub.ShouldExecuteCalled) + require.False(t, skipNonMandatoryShouldExecuteErrorStub.ExecuteCalled) + require.False(t, ensureFinalizerStub.ShouldExecuteCalled) + require.False(t, ensureFinalizerStub.ExecuteCalled) + require.False(t, skipNonDeletingStub.ShouldExecuteCalled) + require.False(t, skipNonDeletingStub.ExecuteCalled) + require.False(t, deleteManifestsStub.ShouldExecuteCalled) + require.False(t, deleteManifestsStub.ExecuteCalled) + require.False(t, removeFinalizerStub.ShouldExecuteCalled) + require.False(t, removeFinalizerStub.ExecuteCalled) +} + // Stubs for the use cases to track execution order and calls type SkipNonMandatoryStub struct { @@ -235,3 +276,25 @@ func (stub *SkipNonMandatoryErrorStub) Execute(_ context.Context, _ *v1beta2.Mod } return errors.New("skipNonMandatory failed") } + +type SkipNonMandatoryShouldExecuteErrorStub struct { + ShouldExecuteCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string +} + +func (stub *SkipNonMandatoryShouldExecuteErrorStub) ShouldExecute(_ context.Context, + _ *v1beta2.ModuleReleaseMeta, +) (bool, error) { + stub.ShouldExecuteCalled = true + return false, errors.New("shouldExecute failed") +} + +func (stub *SkipNonMandatoryShouldExecuteErrorStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { + stub.ExecuteCalled = true + if stub.ExecutionOrder != nil { + *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) + } + return nil +} From 5c0320579ff7b4b38d6d73d6362b823b5f11ddb0 Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 15 Oct 2025 15:56:31 +0200 Subject: [PATCH 04/15] chore: Linting --- .../deletion/deletion_service.go | 4 ++-- .../deletion/deletion_service_test.go | 13 +++++-------- .../deletion/usecases/delete_manifests.go | 3 ++- .../usecases/delete_manifests_test.go | 19 ++++++++++--------- .../deletion/usecases/ensure_finalizer.go | 3 ++- .../usecases/ensure_finalizer_test.go | 11 ++++++----- .../deletion/usecases/remove_finalizer.go | 3 ++- .../usecases/remove_finalizer_test.go | 11 ++++++----- .../usecases/skip_non_deleting_test.go | 11 ++++++----- .../deletion/usecases/skip_non_mandatory.go | 3 +-- .../usecases/skip_non_mandatory_test.go | 3 ++- 11 files changed, 44 insertions(+), 40 deletions(-) diff --git a/internal/service/mandatorymodule/deletion/deletion_service.go b/internal/service/mandatorymodule/deletion/deletion_service.go index 74eb24c186..9819054403 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service.go +++ b/internal/service/mandatorymodule/deletion/deletion_service.go @@ -7,8 +7,8 @@ import ( ) type UseCase interface { - ShouldExecute(context.Context, *v1beta2.ModuleReleaseMeta) (bool, error) - Execute(context.Context, *v1beta2.ModuleReleaseMeta) error + ShouldExecute(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) + Execute(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error } type Service struct { diff --git a/internal/service/mandatorymodule/deletion/deletion_service_test.go b/internal/service/mandatorymodule/deletion/deletion_service_test.go index 425675382d..60d5b6341d 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service_test.go +++ b/internal/service/mandatorymodule/deletion/deletion_service_test.go @@ -5,9 +5,10 @@ import ( "errors" "testing" + "github.com/stretchr/testify/require" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" - "github.com/stretchr/testify/require" ) func TestDeletionService_HandleDeletion_ExecutionOrder(t *testing.T) { @@ -30,7 +31,7 @@ func TestDeletionService_HandleDeletion_ExecutionOrder(t *testing.T) { ) mrm := &v1beta2.ModuleReleaseMeta{} - for i := 0; i < 5; i++ { + for range 5 { err := service.HandleDeletion(context.Background(), mrm) require.NoError(t, err) } @@ -44,11 +45,6 @@ func TestDeletionService_HandleDeletion_ExecutionOrder(t *testing.T) { } require.Equal(t, expectedOrder, executionOrder) - initialLen := len(executionOrder) - err := service.HandleDeletion(context.Background(), mrm) - require.NoError(t, err) - require.Equal(t, initialLen, len(executionOrder)) - require.True(t, skipNonMandatoryStub.ShouldExecuteCalled) require.True(t, skipNonMandatoryStub.ExecuteCalled) require.True(t, ensureFinalizerStub.ShouldExecuteCalled) @@ -84,7 +80,7 @@ func TestDeletionService_HandleDeletion_ErrorPropagation(t *testing.T) { ) mrm := &v1beta2.ModuleReleaseMeta{} - for i := 0; i < 5; i++ { + for range 5 { err := service.HandleDeletion(context.Background(), mrm) require.Error(t, err) require.Contains(t, err.Error(), "skipNonMandatory failed") @@ -157,6 +153,7 @@ func (stub *SkipNonMandatoryStub) ShouldExecute(_ context.Context, _ *v1beta2.Mo stub.ShouldExecuteCalled = true return true, nil } + func (stub *SkipNonMandatoryStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { stub.ExecuteCalled = true if stub.ExecutionOrder != nil { diff --git a/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go b/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go index 92395e07d6..40569879a7 100644 --- a/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go +++ b/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go @@ -4,8 +4,9 @@ import ( "context" "fmt" - "github.com/kyma-project/lifecycle-manager/api/v1beta2" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" ) type ManifestRepo interface { diff --git a/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go b/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go index 18a9601d10..3e1f0be6fd 100644 --- a/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go @@ -5,12 +5,12 @@ import ( "errors" "testing" + "github.com/stretchr/testify/require" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" "github.com/kyma-project/lifecycle-manager/pkg/testutils/random" - "github.com/stretchr/testify/require" - apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) type MockManifestRepo struct { @@ -23,7 +23,8 @@ type MockManifestRepo struct { } func (m *MockManifestRepo) ListAllForModule(_ context.Context, moduleName string) ( - []apimetav1.PartialObjectMetadata, error) { + []apimetav1.PartialObjectMetadata, error, +) { m.ListAllForModuleCalled = true m.CalledWithModuleName = moduleName return m.ManifestsToReturn, m.ListAllForModuleError @@ -40,12 +41,12 @@ func TestDeleteManifests_WithManifests(t *testing.T) { mockRepo := &MockManifestRepo{ ManifestsToReturn: []apimetav1.PartialObjectMetadata{ - {ObjectMeta: metav1.ObjectMeta{Name: random.Name()}}, + {ObjectMeta: apimetav1.ObjectMeta{Name: random.Name()}}, }, } deleteManifests := usecases.NewDeleteManifests(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), }, } @@ -69,7 +70,7 @@ func TestDeleteManifests_NoManifests(t *testing.T) { } deleteManifests := usecases.NewDeleteManifests(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), }, } @@ -88,7 +89,7 @@ func TestDeleteManifests_ListError(t *testing.T) { } deleteManifests := usecases.NewDeleteManifests(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), }, } @@ -108,7 +109,7 @@ func TestDeleteManifests_DeleteError(t *testing.T) { } deleteManifests := usecases.NewDeleteManifests(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), }, } diff --git a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go index 317546b262..0e8c12ce4e 100644 --- a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go +++ b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go @@ -3,9 +3,10 @@ package usecases import ( "context" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) type MrmEnsureFinalizerRepo interface { diff --git a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go index 496a9956fc..8efb9150cc 100644 --- a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go @@ -5,12 +5,13 @@ import ( "errors" "testing" + "github.com/stretchr/testify/require" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" "github.com/kyma-project/lifecycle-manager/pkg/testutils/random" - "github.com/stretchr/testify/require" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) type MockMrmEnsureFinalizerRepo struct { @@ -33,7 +34,7 @@ func TestEnsureFinalizer_WithoutFinalizer(t *testing.T) { mockRepo := &MockMrmEnsureFinalizerRepo{} ensureFinalizer := usecases.NewEnsureFinalizer(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), }, } @@ -55,7 +56,7 @@ func TestEnsureFinalizer_WithFinalizer(t *testing.T) { mockRepo := &MockMrmEnsureFinalizerRepo{} ensureFinalizer := usecases.NewEnsureFinalizer(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), Finalizers: []string{shared.MandatoryModuleFinalizer}, }, @@ -75,7 +76,7 @@ func TestEnsureFinalizer_RepositoryError(t *testing.T) { } ensureFinalizer := usecases.NewEnsureFinalizer(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), }, } diff --git a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go index 0bb05d6711..d6cd58a421 100644 --- a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go +++ b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go @@ -3,9 +3,10 @@ package usecases import ( "context" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) type MrmRemoFinalizerRepo interface { diff --git a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go index 2d307218be..4ed79e225b 100644 --- a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go @@ -5,12 +5,13 @@ import ( "errors" "testing" + "github.com/stretchr/testify/require" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" "github.com/kyma-project/lifecycle-manager/pkg/testutils/random" - "github.com/stretchr/testify/require" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) type MockMrmRemoFinalizerRepo struct { @@ -33,7 +34,7 @@ func TestRemoveFinalizer_WithFinalizer(t *testing.T) { mockRepo := &MockMrmRemoFinalizerRepo{} removeFinalizer := usecases.NewRemoveFinalizer(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), Finalizers: []string{shared.MandatoryModuleFinalizer}, }, @@ -56,7 +57,7 @@ func TestRemoveFinalizer_WithoutFinalizer(t *testing.T) { mockRepo := &MockMrmRemoFinalizerRepo{} removeFinalizer := usecases.NewRemoveFinalizer(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), }, } @@ -75,7 +76,7 @@ func TestRemoveFinalizer_RepositoryError(t *testing.T) { } removeFinalizer := usecases.NewRemoveFinalizer(mockRepo) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: random.Name(), Finalizers: []string{shared.MandatoryModuleFinalizer}, }, diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go index bbef9fbc2e..d50aea5ae9 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go @@ -5,11 +5,12 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" - "github.com/stretchr/testify/require" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestSkipNonDeleting_NotDeleting(t *testing.T) { @@ -17,7 +18,7 @@ func TestSkipNonDeleting_NotDeleting(t *testing.T) { skipNonDeleting := usecases.NewSkipNonDeleting() mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: "test-module", }, } @@ -34,9 +35,9 @@ func TestSkipNonDeleting_IsDeleting(t *testing.T) { t.Parallel() skipNonDeleting := usecases.NewSkipNonDeleting() - now := metav1.NewTime(time.Now()) + now := apimetav1.NewTime(time.Now()) mrm := &v1beta2.ModuleReleaseMeta{ - ObjectMeta: metav1.ObjectMeta{ + ObjectMeta: apimetav1.ObjectMeta{ Name: "test-module", DeletionTimestamp: &now, }, diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go index ee56c77981..03139e8872 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go @@ -8,8 +8,7 @@ import ( ) // SkipNonMandatory is a use case that skips ModuleReleaseMetas that are not mandatory. -type SkipNonMandatory struct { -} +type SkipNonMandatory struct{} func NewSkipNonMandatory() *SkipNonMandatory { return &SkipNonMandatory{} diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go index 82cd866b0d..25b796c21e 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go @@ -4,10 +4,11 @@ import ( "context" "testing" + "github.com/stretchr/testify/require" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" - "github.com/stretchr/testify/require" ) func TestSkipNonMandatory_WithNonMandatoryModule(t *testing.T) { From 50ff50610e369e8b9aab918bb961127a87c941f3 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 16 Oct 2025 10:24:34 +0200 Subject: [PATCH 05/15] chore: Move Errors to Dedicated Package --- internal/{service => errors}/mandatorymodule/deletion/errors.go | 0 .../mandatorymodule/deletion/usecases/skip_non_deleting.go | 2 +- .../mandatorymodule/deletion/usecases/skip_non_deleting_test.go | 2 +- .../mandatorymodule/deletion/usecases/skip_non_mandatory.go | 2 +- .../deletion/usecases/skip_non_mandatory_test.go | 2 +- 5 files changed, 4 insertions(+), 4 deletions(-) rename internal/{service => errors}/mandatorymodule/deletion/errors.go (100%) diff --git a/internal/service/mandatorymodule/deletion/errors.go b/internal/errors/mandatorymodule/deletion/errors.go similarity index 100% rename from internal/service/mandatorymodule/deletion/errors.go rename to internal/errors/mandatorymodule/deletion/errors.go diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go index 51f643371d..39aeab2fb5 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go @@ -4,7 +4,7 @@ import ( "context" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" + "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" ) // SkipNonDeleting is a use case that skips ModuleReleaseMetas that are not in deleting state. diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go index d50aea5ae9..c87cce0dc9 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go @@ -5,11 +5,11 @@ import ( "testing" "time" + "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" "github.com/stretchr/testify/require" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" ) diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go index 03139e8872..7817f37ae4 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go @@ -4,7 +4,7 @@ import ( "context" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" + "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" ) // SkipNonMandatory is a use case that skips ModuleReleaseMetas that are not mandatory. diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go index 25b796c21e..7e85c63889 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go @@ -4,10 +4,10 @@ import ( "context" "testing" + "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" "github.com/stretchr/testify/require" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" ) From 71aae403465b3096c0feb3ed634a20f772200b53 Mon Sep 17 00:00:00 2001 From: Raj <54686422+LeelaChacha@users.noreply.github.com> Date: Thu, 16 Oct 2025 13:20:00 +0200 Subject: [PATCH 06/15] chore: Apply suggestions from code review Co-authored-by: Benjamin Lindner <50365642+lindnerby@users.noreply.github.com> --- .../service/mandatorymodule/deletion/deletion_service.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/service/mandatorymodule/deletion/deletion_service.go b/internal/service/mandatorymodule/deletion/deletion_service.go index 9819054403..f7f7261f28 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service.go +++ b/internal/service/mandatorymodule/deletion/deletion_service.go @@ -22,7 +22,7 @@ func NewService(skipNonMandatory UseCase, removeFinalizer UseCase, ) *Service { return &Service{ - orderedUseCases: []UseCase{ + orderedSteps: []UseCase{ skipNonMandatory, // if returns deletion.ErrMrmNotMandatory, controller should not requeue ensureFinalizer, skipNonDeleting, // if returns deletion.ErrMrmNotInDeletingState, controller should not requeue @@ -33,7 +33,8 @@ func NewService(skipNonMandatory UseCase, } func (s *Service) HandleDeletion(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { - for _, useCase := range s.orderedUseCases { + // Find the first applicable step and execute it + for _, step := range s. orderedSteps { shouldExecute, err := useCase.ShouldExecute(ctx, mrm) if err != nil { return err From ab1f36ae320d4907545fd0fa9593262f3ca2dfc6 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 16 Oct 2025 13:24:48 +0200 Subject: [PATCH 07/15] chore: PR Feedback --- .../deletion/deletion_service.go | 10 +- .../deletion/deletion_service_test.go | 130 +++++++++--------- .../deletion/usecases/delete_manifests.go | 2 +- .../usecases/delete_manifests_test.go | 12 +- .../deletion/usecases/ensure_finalizer.go | 2 +- .../usecases/ensure_finalizer_test.go | 8 +- .../deletion/usecases/remove_finalizer.go | 2 +- .../usecases/remove_finalizer_test.go | 8 +- .../deletion/usecases/skip_non_deleting.go | 2 +- .../usecases/skip_non_deleting_test.go | 8 +- .../deletion/usecases/skip_non_mandatory.go | 2 +- .../usecases/skip_non_mandatory_test.go | 8 +- 12 files changed, 97 insertions(+), 97 deletions(-) diff --git a/internal/service/mandatorymodule/deletion/deletion_service.go b/internal/service/mandatorymodule/deletion/deletion_service.go index f7f7261f28..aa34ecc495 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service.go +++ b/internal/service/mandatorymodule/deletion/deletion_service.go @@ -7,12 +7,12 @@ import ( ) type UseCase interface { - ShouldExecute(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) + IsApplicable(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) Execute(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error } type Service struct { - orderedUseCases []UseCase + orderedSteps []UseCase } func NewService(skipNonMandatory UseCase, @@ -34,13 +34,13 @@ func NewService(skipNonMandatory UseCase, func (s *Service) HandleDeletion(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { // Find the first applicable step and execute it - for _, step := range s. orderedSteps { - shouldExecute, err := useCase.ShouldExecute(ctx, mrm) + for _, step := range s.orderedSteps { + shouldExecute, err := step.IsApplicable(ctx, mrm) if err != nil { return err } if shouldExecute { - return useCase.Execute(ctx, mrm) + return step.Execute(ctx, mrm) } } return nil diff --git a/internal/service/mandatorymodule/deletion/deletion_service_test.go b/internal/service/mandatorymodule/deletion/deletion_service_test.go index 60d5b6341d..ae8ad97c57 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service_test.go +++ b/internal/service/mandatorymodule/deletion/deletion_service_test.go @@ -45,15 +45,15 @@ func TestDeletionService_HandleDeletion_ExecutionOrder(t *testing.T) { } require.Equal(t, expectedOrder, executionOrder) - require.True(t, skipNonMandatoryStub.ShouldExecuteCalled) + require.True(t, skipNonMandatoryStub.IsApplicableCalled) require.True(t, skipNonMandatoryStub.ExecuteCalled) - require.True(t, ensureFinalizerStub.ShouldExecuteCalled) + require.True(t, ensureFinalizerStub.IsApplicableCalled) require.True(t, ensureFinalizerStub.ExecuteCalled) - require.True(t, skipNonDeletingStub.ShouldExecuteCalled) + require.True(t, skipNonDeletingStub.IsApplicableCalled) require.True(t, skipNonDeletingStub.ExecuteCalled) - require.True(t, deleteManifestsStub.ShouldExecuteCalled) + require.True(t, deleteManifestsStub.IsApplicableCalled) require.True(t, deleteManifestsStub.ExecuteCalled) - require.True(t, removeFinalizerStub.ShouldExecuteCalled) + require.True(t, removeFinalizerStub.IsApplicableCalled) require.True(t, removeFinalizerStub.ExecuteCalled) } @@ -96,12 +96,12 @@ func TestDeletionService_HandleDeletion_ErrorPropagation(t *testing.T) { require.Equal(t, expectedOrder, executionOrder) } -func TestDeletionService_HandleDeletion_ShouldExecuteError(t *testing.T) { +func TestDeletionService_HandleDeletion_IsApplicableError(t *testing.T) { t.Parallel() var executionOrder []string - skipNonMandatoryShouldExecuteErrorStub := &SkipNonMandatoryShouldExecuteErrorStub{ + skipNonMandatoryIsApplicableErrorStub := &SkipNonMandatoryIsApplicableErrorStub{ StubName: "skipNonMandatory", ExecutionOrder: &executionOrder, } @@ -111,7 +111,7 @@ func TestDeletionService_HandleDeletion_ShouldExecuteError(t *testing.T) { removeFinalizerStub := &RemoveFinalizerStub{StubName: "removeFinalizer", ExecutionOrder: &executionOrder} service := deletion.NewService( - skipNonMandatoryShouldExecuteErrorStub, + skipNonMandatoryIsApplicableErrorStub, ensureFinalizerStub, skipNonDeletingStub, deleteManifestsStub, @@ -121,36 +121,36 @@ func TestDeletionService_HandleDeletion_ShouldExecuteError(t *testing.T) { err := service.HandleDeletion(context.Background(), mrm) require.Error(t, err) - require.Contains(t, err.Error(), "shouldExecute failed") + require.Contains(t, err.Error(), "IsApplicable failed") require.Empty(t, executionOrder) - require.True(t, skipNonMandatoryShouldExecuteErrorStub.ShouldExecuteCalled) - require.False(t, skipNonMandatoryShouldExecuteErrorStub.ExecuteCalled) - require.False(t, ensureFinalizerStub.ShouldExecuteCalled) + require.True(t, skipNonMandatoryIsApplicableErrorStub.IsApplicableCalled) + require.False(t, skipNonMandatoryIsApplicableErrorStub.ExecuteCalled) + require.False(t, ensureFinalizerStub.IsApplicableCalled) require.False(t, ensureFinalizerStub.ExecuteCalled) - require.False(t, skipNonDeletingStub.ShouldExecuteCalled) + require.False(t, skipNonDeletingStub.IsApplicableCalled) require.False(t, skipNonDeletingStub.ExecuteCalled) - require.False(t, deleteManifestsStub.ShouldExecuteCalled) + require.False(t, deleteManifestsStub.IsApplicableCalled) require.False(t, deleteManifestsStub.ExecuteCalled) - require.False(t, removeFinalizerStub.ShouldExecuteCalled) + require.False(t, removeFinalizerStub.IsApplicableCalled) require.False(t, removeFinalizerStub.ExecuteCalled) } // Stubs for the use cases to track execution order and calls type SkipNonMandatoryStub struct { - ShouldExecuteCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string + IsApplicableCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string } -func (stub *SkipNonMandatoryStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - if stub.ShouldExecuteCalled { +func (stub *SkipNonMandatoryStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.IsApplicableCalled { return false, nil } - stub.ShouldExecuteCalled = true + stub.IsApplicableCalled = true return true, nil } @@ -163,17 +163,17 @@ func (stub *SkipNonMandatoryStub) Execute(_ context.Context, _ *v1beta2.ModuleRe } type EnsureFinalizerStub struct { - ShouldExecuteCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string + IsApplicableCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string } -func (stub *EnsureFinalizerStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - if stub.ShouldExecuteCalled { +func (stub *EnsureFinalizerStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.IsApplicableCalled { return false, nil } - stub.ShouldExecuteCalled = true + stub.IsApplicableCalled = true return true, nil } @@ -186,17 +186,17 @@ func (stub *EnsureFinalizerStub) Execute(_ context.Context, _ *v1beta2.ModuleRel } type SkipNonDeletingStub struct { - ShouldExecuteCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string + IsApplicableCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string } -func (stub *SkipNonDeletingStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - if stub.ShouldExecuteCalled { +func (stub *SkipNonDeletingStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.IsApplicableCalled { return false, nil } - stub.ShouldExecuteCalled = true + stub.IsApplicableCalled = true return true, nil } @@ -209,17 +209,17 @@ func (stub *SkipNonDeletingStub) Execute(_ context.Context, _ *v1beta2.ModuleRel } type DeleteManifestsStub struct { - ShouldExecuteCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string + IsApplicableCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string } -func (stub *DeleteManifestsStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - if stub.ShouldExecuteCalled { +func (stub *DeleteManifestsStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.IsApplicableCalled { return false, nil } - stub.ShouldExecuteCalled = true + stub.IsApplicableCalled = true return true, nil } @@ -232,17 +232,17 @@ func (stub *DeleteManifestsStub) Execute(_ context.Context, _ *v1beta2.ModuleRel } type RemoveFinalizerStub struct { - ShouldExecuteCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string + IsApplicableCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string } -func (stub *RemoveFinalizerStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - if stub.ShouldExecuteCalled { +func (stub *RemoveFinalizerStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + if stub.IsApplicableCalled { return false, nil } - stub.ShouldExecuteCalled = true + stub.IsApplicableCalled = true return true, nil } @@ -255,14 +255,14 @@ func (stub *RemoveFinalizerStub) Execute(_ context.Context, _ *v1beta2.ModuleRel } type SkipNonMandatoryErrorStub struct { - ShouldExecuteCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string + IsApplicableCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string } -func (stub *SkipNonMandatoryErrorStub) ShouldExecute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - stub.ShouldExecuteCalled = true +func (stub *SkipNonMandatoryErrorStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { + stub.IsApplicableCalled = true return true, nil } @@ -274,21 +274,21 @@ func (stub *SkipNonMandatoryErrorStub) Execute(_ context.Context, _ *v1beta2.Mod return errors.New("skipNonMandatory failed") } -type SkipNonMandatoryShouldExecuteErrorStub struct { - ShouldExecuteCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string +type SkipNonMandatoryIsApplicableErrorStub struct { + IsApplicableCalled bool + ExecuteCalled bool + ExecutionOrder *[]string + StubName string } -func (stub *SkipNonMandatoryShouldExecuteErrorStub) ShouldExecute(_ context.Context, +func (stub *SkipNonMandatoryIsApplicableErrorStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta, ) (bool, error) { - stub.ShouldExecuteCalled = true - return false, errors.New("shouldExecute failed") + stub.IsApplicableCalled = true + return false, errors.New("IsApplicable failed") } -func (stub *SkipNonMandatoryShouldExecuteErrorStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { +func (stub *SkipNonMandatoryIsApplicableErrorStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { stub.ExecuteCalled = true if stub.ExecutionOrder != nil { *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) diff --git a/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go b/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go index 40569879a7..09cfc278dd 100644 --- a/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go +++ b/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go @@ -23,7 +23,7 @@ func NewDeleteManifests(repo ManifestRepo) *DeleteManifests { return &DeleteManifests{repo: repo} } -func (d *DeleteManifests) ShouldExecute(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { +func (d *DeleteManifests) IsApplicable(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { manifests, err := d.repo.ListAllForModule(ctx, mrm.Name) if err != nil { return false, fmt.Errorf("failed to list manifests for module %s: %w", mrm.Name, err) diff --git a/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go b/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go index 3e1f0be6fd..ac6b13b87e 100644 --- a/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/delete_manifests_test.go @@ -51,9 +51,9 @@ func TestDeleteManifests_WithManifests(t *testing.T) { }, } - shouldExecute, err := deleteManifests.ShouldExecute(context.Background(), mrm) + isApplicable, err := deleteManifests.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.True(t, shouldExecute) + require.True(t, isApplicable) require.True(t, mockRepo.ListAllForModuleCalled) require.Equal(t, mrm.Name, mockRepo.CalledWithModuleName) @@ -75,9 +75,9 @@ func TestDeleteManifests_NoManifests(t *testing.T) { }, } - shouldExecute, err := deleteManifests.ShouldExecute(context.Background(), mrm) + isApplicable, err := deleteManifests.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.False(t, shouldExecute) + require.False(t, isApplicable) } func TestDeleteManifests_ListError(t *testing.T) { @@ -94,9 +94,9 @@ func TestDeleteManifests_ListError(t *testing.T) { }, } - shouldExecute, err := deleteManifests.ShouldExecute(context.Background(), mrm) + isApplicable, err := deleteManifests.IsApplicable(context.Background(), mrm) require.Error(t, err) - require.False(t, shouldExecute) + require.False(t, isApplicable) require.Contains(t, err.Error(), "failed to list manifests for module") } diff --git a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go index 0e8c12ce4e..1fc2d759e3 100644 --- a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go +++ b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go @@ -22,7 +22,7 @@ func NewEnsureFinalizer(repo MrmEnsureFinalizerRepo) *EnsureFinalizer { return &EnsureFinalizer{repo: repo} } -func (e *EnsureFinalizer) ShouldExecute(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { +func (e *EnsureFinalizer) IsApplicable(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { return !controllerutil.ContainsFinalizer(mrm, shared.MandatoryModuleFinalizer), nil } diff --git a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go index 8efb9150cc..32fa5721ba 100644 --- a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer_test.go @@ -39,9 +39,9 @@ func TestEnsureFinalizer_WithoutFinalizer(t *testing.T) { }, } - shouldExecute, err := ensureFinalizer.ShouldExecute(context.Background(), mrm) + isApplicable, err := ensureFinalizer.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.True(t, shouldExecute) + require.True(t, isApplicable) executeErr := ensureFinalizer.Execute(context.Background(), mrm) require.NoError(t, executeErr) @@ -62,9 +62,9 @@ func TestEnsureFinalizer_WithFinalizer(t *testing.T) { }, } - shouldExecute, err := ensureFinalizer.ShouldExecute(context.Background(), mrm) + isApplicable, err := ensureFinalizer.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.False(t, shouldExecute) + require.False(t, isApplicable) } func TestEnsureFinalizer_RepositoryError(t *testing.T) { diff --git a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go index d6cd58a421..56a104ebb5 100644 --- a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go +++ b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go @@ -22,7 +22,7 @@ func NewRemoveFinalizer(repo MrmRemoFinalizerRepo) *RemoveFinalizer { return &RemoveFinalizer{repo: repo} } -func (e *RemoveFinalizer) ShouldExecute(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { +func (e *RemoveFinalizer) IsApplicable(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { return controllerutil.ContainsFinalizer(mrm, shared.MandatoryModuleFinalizer), nil } diff --git a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go index 4ed79e225b..e262ba72fc 100644 --- a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer_test.go @@ -40,9 +40,9 @@ func TestRemoveFinalizer_WithFinalizer(t *testing.T) { }, } - shouldExecute, err := removeFinalizer.ShouldExecute(context.Background(), mrm) + isApplicable, err := removeFinalizer.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.True(t, shouldExecute) + require.True(t, isApplicable) executeErr := removeFinalizer.Execute(context.Background(), mrm) require.NoError(t, executeErr) @@ -62,9 +62,9 @@ func TestRemoveFinalizer_WithoutFinalizer(t *testing.T) { }, } - shouldExecute, err := removeFinalizer.ShouldExecute(context.Background(), mrm) + isApplicable, err := removeFinalizer.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.False(t, shouldExecute) + require.False(t, isApplicable) } func TestRemoveFinalizer_RepositoryError(t *testing.T) { diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go index 39aeab2fb5..b26e8425e9 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go @@ -14,7 +14,7 @@ func NewSkipNonDeleting() *SkipNonDeleting { return &SkipNonDeleting{} } -func (s *SkipNonDeleting) ShouldExecute(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { +func (s *SkipNonDeleting) IsApplicable(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { return mrm.DeletionTimestamp.IsZero(), nil } diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go index c87cce0dc9..fcdae94e4d 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go @@ -23,9 +23,9 @@ func TestSkipNonDeleting_NotDeleting(t *testing.T) { }, } - shouldExecute, err := skipNonDeleting.ShouldExecute(context.Background(), mrm) + isApplicable, err := skipNonDeleting.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.True(t, shouldExecute) + require.True(t, isApplicable) executeErr := skipNonDeleting.Execute(context.Background(), mrm) require.ErrorIs(t, executeErr, deletion.ErrMrmNotInDeletingState) @@ -43,7 +43,7 @@ func TestSkipNonDeleting_IsDeleting(t *testing.T) { }, } - shouldExecute, err := skipNonDeleting.ShouldExecute(context.Background(), mrm) + isApplicable, err := skipNonDeleting.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.False(t, shouldExecute) + require.False(t, isApplicable) } diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go index 7817f37ae4..96749440d6 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go @@ -14,7 +14,7 @@ func NewSkipNonMandatory() *SkipNonMandatory { return &SkipNonMandatory{} } -func (s *SkipNonMandatory) ShouldExecute(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { +func (s *SkipNonMandatory) IsApplicable(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { return mrm.Spec.Mandatory == nil || mrm.Spec.Mandatory.Version == "", nil } diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go index 7e85c63889..1486f7a480 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go @@ -21,9 +21,9 @@ func TestSkipNonMandatory_WithNonMandatoryModule(t *testing.T) { }, } - shouldExecute, err := skipNonMandatory.ShouldExecute(context.Background(), mrm) + isApplicable, err := skipNonMandatory.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.True(t, shouldExecute) + require.True(t, isApplicable) executeErr := skipNonMandatory.Execute(context.Background(), mrm) require.Error(t, executeErr) @@ -42,7 +42,7 @@ func TestSkipNonMandatory_WithMandatoryModule(t *testing.T) { }, } - shouldExecute, err := skipNonMandatory.ShouldExecute(context.Background(), mrm) + isApplicable, err := skipNonMandatory.IsApplicable(context.Background(), mrm) require.NoError(t, err) - require.False(t, shouldExecute) + require.False(t, isApplicable) } From b71e66c400838c97816e317b6dc711d6f5dea04d Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 16 Oct 2025 15:25:37 +0200 Subject: [PATCH 08/15] chore: PR Feedback --- internal/service/mandatorymodule/deletion/deletion_service.go | 4 ++-- .../deletion/usecases/skip_non_deleting_test.go | 2 +- .../deletion/usecases/skip_non_mandatory_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/service/mandatorymodule/deletion/deletion_service.go b/internal/service/mandatorymodule/deletion/deletion_service.go index aa34ecc495..ab8e6709f5 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service.go +++ b/internal/service/mandatorymodule/deletion/deletion_service.go @@ -35,11 +35,11 @@ func NewService(skipNonMandatory UseCase, func (s *Service) HandleDeletion(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { // Find the first applicable step and execute it for _, step := range s.orderedSteps { - shouldExecute, err := step.IsApplicable(ctx, mrm) + isApplicable, err := step.IsApplicable(ctx, mrm) if err != nil { return err } - if shouldExecute { + if isApplicable { return step.Execute(ctx, mrm) } } diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go index fcdae94e4d..02d0ca0b1b 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting_test.go @@ -5,11 +5,11 @@ import ( "testing" "time" - "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" "github.com/stretchr/testify/require" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" ) diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go index 1486f7a480..f11feb5d3c 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go @@ -4,10 +4,10 @@ import ( "context" "testing" - "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" "github.com/stretchr/testify/require" "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" ) From 964a16290d42774193b751bec87c95de48e8b391 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 16 Oct 2025 15:31:53 +0200 Subject: [PATCH 09/15] test: No Applicable Step for Deletion Service --- .../service/mandatorymodule/deletion/deletion_service_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/mandatorymodule/deletion/deletion_service_test.go b/internal/service/mandatorymodule/deletion/deletion_service_test.go index ae8ad97c57..65116f52df 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service_test.go +++ b/internal/service/mandatorymodule/deletion/deletion_service_test.go @@ -31,7 +31,7 @@ func TestDeletionService_HandleDeletion_ExecutionOrder(t *testing.T) { ) mrm := &v1beta2.ModuleReleaseMeta{} - for range 5 { + for range 6 { err := service.HandleDeletion(context.Background(), mrm) require.NoError(t, err) } From 6d5bd5e61c28aa2d5093ef4e1392f22a9ed19d50 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 16 Oct 2025 16:34:47 +0200 Subject: [PATCH 10/15] chore: PR Feedback --- .../errors/mandatorymodule/deletion/errors.go | 1 - .../deletion/deletion_service.go | 8 +- .../deletion/deletion_service_test.go | 185 ++++-------------- 3 files changed, 46 insertions(+), 148 deletions(-) diff --git a/internal/errors/mandatorymodule/deletion/errors.go b/internal/errors/mandatorymodule/deletion/errors.go index 0a7fac72b0..d6634c3de6 100644 --- a/internal/errors/mandatorymodule/deletion/errors.go +++ b/internal/errors/mandatorymodule/deletion/errors.go @@ -3,6 +3,5 @@ package deletion import "errors" var ( - ErrMrmNotMandatory = errors.New("ModuleReleaseMeta is not a mandatory module") ErrMrmNotInDeletingState = errors.New("ModuleReleaseMeta not in deleting state") ) diff --git a/internal/service/mandatorymodule/deletion/deletion_service.go b/internal/service/mandatorymodule/deletion/deletion_service.go index ab8e6709f5..1f54b23635 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service.go +++ b/internal/service/mandatorymodule/deletion/deletion_service.go @@ -15,23 +15,23 @@ type Service struct { orderedSteps []UseCase } -func NewService(skipNonMandatory UseCase, - ensureFinalizer UseCase, +func NewService(ensureFinalizer UseCase, skipNonDeleting UseCase, deleteManifests UseCase, removeFinalizer UseCase, ) *Service { return &Service{ orderedSteps: []UseCase{ - skipNonMandatory, // if returns deletion.ErrMrmNotMandatory, controller should not requeue ensureFinalizer, - skipNonDeleting, // if returns deletion.ErrMrmNotInDeletingState, controller should not requeue + skipNonDeleting, deleteManifests, removeFinalizer, }, } } +// HandleDeletion processes the deletion of a ModuleReleaseMeta through a series of ordered use cases. +// Returns deletion.ErrMrmNotInDeletingState error if the MRM is not in deleting state, which indicates that the controller should not requeue. func (s *Service) HandleDeletion(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { // Find the first applicable step and execute it for _, step := range s.orderedSteps { diff --git a/internal/service/mandatorymodule/deletion/deletion_service_test.go b/internal/service/mandatorymodule/deletion/deletion_service_test.go index 65116f52df..1e2f7aaa9f 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service_test.go +++ b/internal/service/mandatorymodule/deletion/deletion_service_test.go @@ -16,14 +16,12 @@ func TestDeletionService_HandleDeletion_ExecutionOrder(t *testing.T) { var executionOrder []string - skipNonMandatoryStub := &SkipNonMandatoryStub{StubName: "skipNonMandatory", ExecutionOrder: &executionOrder} - ensureFinalizerStub := &EnsureFinalizerStub{StubName: "ensureFinalizer", ExecutionOrder: &executionOrder} - skipNonDeletingStub := &SkipNonDeletingStub{StubName: "skipNonDeleting", ExecutionOrder: &executionOrder} - deleteManifestsStub := &DeleteManifestsStub{StubName: "deleteManifests", ExecutionOrder: &executionOrder} - removeFinalizerStub := &RemoveFinalizerStub{StubName: "removeFinalizer", ExecutionOrder: &executionOrder} + ensureFinalizerStub := &UseCaseStub{UseCaseName: "ensureFinalizer", ExecutionOrder: &executionOrder} + skipNonDeletingStub := &UseCaseStub{UseCaseName: "skipNonDeleting", ExecutionOrder: &executionOrder} + deleteManifestsStub := &UseCaseStub{UseCaseName: "deleteManifests", ExecutionOrder: &executionOrder} + removeFinalizerStub := &UseCaseStub{UseCaseName: "removeFinalizer", ExecutionOrder: &executionOrder} service := deletion.NewService( - skipNonMandatoryStub, ensureFinalizerStub, skipNonDeletingStub, deleteManifestsStub, @@ -31,13 +29,12 @@ func TestDeletionService_HandleDeletion_ExecutionOrder(t *testing.T) { ) mrm := &v1beta2.ModuleReleaseMeta{} - for range 6 { + for range 5 { err := service.HandleDeletion(context.Background(), mrm) require.NoError(t, err) } expectedOrder := []string{ - "skipNonMandatory", "ensureFinalizer", "skipNonDeleting", "deleteManifests", @@ -45,8 +42,6 @@ func TestDeletionService_HandleDeletion_ExecutionOrder(t *testing.T) { } require.Equal(t, expectedOrder, executionOrder) - require.True(t, skipNonMandatoryStub.IsApplicableCalled) - require.True(t, skipNonMandatoryStub.ExecuteCalled) require.True(t, ensureFinalizerStub.IsApplicableCalled) require.True(t, ensureFinalizerStub.ExecuteCalled) require.True(t, skipNonDeletingStub.IsApplicableCalled) @@ -62,18 +57,17 @@ func TestDeletionService_HandleDeletion_ErrorPropagation(t *testing.T) { var executionOrder []string - skipNonMandatoryErrorStub := &SkipNonMandatoryErrorStub{ - StubName: "skipNonMandatory", + ensureFinalizerErrorStub := &UseCaseErrorStub{ + StubName: "ensureFinalizer", ExecutionOrder: &executionOrder, + ErrorMessage: "ensureFinalizer failed", } - ensureFinalizerStub := &EnsureFinalizerStub{StubName: "ensureFinalizer", ExecutionOrder: &executionOrder} - skipNonDeletingStub := &SkipNonDeletingStub{StubName: "skipNonDeleting", ExecutionOrder: &executionOrder} - deleteManifestsStub := &DeleteManifestsStub{StubName: "deleteManifests", ExecutionOrder: &executionOrder} - removeFinalizerStub := &RemoveFinalizerStub{StubName: "removeFinalizer", ExecutionOrder: &executionOrder} + skipNonDeletingStub := &UseCaseStub{UseCaseName: "skipNonDeleting", ExecutionOrder: &executionOrder} + deleteManifestsStub := &UseCaseStub{UseCaseName: "deleteManifests", ExecutionOrder: &executionOrder} + removeFinalizerStub := &UseCaseStub{UseCaseName: "removeFinalizer", ExecutionOrder: &executionOrder} service := deletion.NewService( - skipNonMandatoryErrorStub, - ensureFinalizerStub, + ensureFinalizerErrorStub, skipNonDeletingStub, deleteManifestsStub, removeFinalizerStub, @@ -83,15 +77,15 @@ func TestDeletionService_HandleDeletion_ErrorPropagation(t *testing.T) { for range 5 { err := service.HandleDeletion(context.Background(), mrm) require.Error(t, err) - require.Contains(t, err.Error(), "skipNonMandatory failed") + require.Contains(t, err.Error(), "ensureFinalizer failed") } expectedOrder := []string{ - "skipNonMandatory", - "skipNonMandatory", - "skipNonMandatory", - "skipNonMandatory", - "skipNonMandatory", + "ensureFinalizer", + "ensureFinalizer", + "ensureFinalizer", + "ensureFinalizer", + "ensureFinalizer", } require.Equal(t, expectedOrder, executionOrder) } @@ -101,18 +95,17 @@ func TestDeletionService_HandleDeletion_IsApplicableError(t *testing.T) { var executionOrder []string - skipNonMandatoryIsApplicableErrorStub := &SkipNonMandatoryIsApplicableErrorStub{ - StubName: "skipNonMandatory", + ensureFinalizerIsApplicableErrorStub := &UseCaseIsApplicableErrorStub{ + StubName: "ensureFinalizer", ExecutionOrder: &executionOrder, + ErrorMessage: "IsApplicable failed", } - ensureFinalizerStub := &EnsureFinalizerStub{StubName: "ensureFinalizer", ExecutionOrder: &executionOrder} - skipNonDeletingStub := &SkipNonDeletingStub{StubName: "skipNonDeleting", ExecutionOrder: &executionOrder} - deleteManifestsStub := &DeleteManifestsStub{StubName: "deleteManifests", ExecutionOrder: &executionOrder} - removeFinalizerStub := &RemoveFinalizerStub{StubName: "removeFinalizer", ExecutionOrder: &executionOrder} + skipNonDeletingStub := &UseCaseStub{UseCaseName: "skipNonDeleting", ExecutionOrder: &executionOrder} + deleteManifestsStub := &UseCaseStub{UseCaseName: "deleteManifests", ExecutionOrder: &executionOrder} + removeFinalizerStub := &UseCaseStub{UseCaseName: "removeFinalizer", ExecutionOrder: &executionOrder} service := deletion.NewService( - skipNonMandatoryIsApplicableErrorStub, - ensureFinalizerStub, + ensureFinalizerIsApplicableErrorStub, skipNonDeletingStub, deleteManifestsStub, removeFinalizerStub, @@ -125,10 +118,8 @@ func TestDeletionService_HandleDeletion_IsApplicableError(t *testing.T) { require.Empty(t, executionOrder) - require.True(t, skipNonMandatoryIsApplicableErrorStub.IsApplicableCalled) - require.False(t, skipNonMandatoryIsApplicableErrorStub.ExecuteCalled) - require.False(t, ensureFinalizerStub.IsApplicableCalled) - require.False(t, ensureFinalizerStub.ExecuteCalled) + require.True(t, ensureFinalizerIsApplicableErrorStub.IsApplicableCalled) + require.False(t, ensureFinalizerIsApplicableErrorStub.ExecuteCalled) require.False(t, skipNonDeletingStub.IsApplicableCalled) require.False(t, skipNonDeletingStub.ExecuteCalled) require.False(t, deleteManifestsStub.IsApplicableCalled) @@ -139,60 +130,14 @@ func TestDeletionService_HandleDeletion_IsApplicableError(t *testing.T) { // Stubs for the use cases to track execution order and calls -type SkipNonMandatoryStub struct { - IsApplicableCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string -} - -func (stub *SkipNonMandatoryStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - if stub.IsApplicableCalled { - return false, nil - } - stub.IsApplicableCalled = true - return true, nil -} - -func (stub *SkipNonMandatoryStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { - stub.ExecuteCalled = true - if stub.ExecutionOrder != nil { - *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) - } - return nil -} - -type EnsureFinalizerStub struct { - IsApplicableCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string -} - -func (stub *EnsureFinalizerStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - if stub.IsApplicableCalled { - return false, nil - } - stub.IsApplicableCalled = true - return true, nil -} - -func (stub *EnsureFinalizerStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { - stub.ExecuteCalled = true - if stub.ExecutionOrder != nil { - *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) - } - return nil -} - -type SkipNonDeletingStub struct { +type UseCaseStub struct { IsApplicableCalled bool ExecuteCalled bool ExecutionOrder *[]string - StubName string + UseCaseName string } -func (stub *SkipNonDeletingStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { +func (stub *UseCaseStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { if stub.IsApplicableCalled { return false, nil } @@ -200,95 +145,49 @@ func (stub *SkipNonDeletingStub) IsApplicable(_ context.Context, _ *v1beta2.Modu return true, nil } -func (stub *SkipNonDeletingStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { +func (stub *UseCaseStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { stub.ExecuteCalled = true if stub.ExecutionOrder != nil { - *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) - } - return nil -} - -type DeleteManifestsStub struct { - IsApplicableCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string -} - -func (stub *DeleteManifestsStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - if stub.IsApplicableCalled { - return false, nil - } - stub.IsApplicableCalled = true - return true, nil -} - -func (stub *DeleteManifestsStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { - stub.ExecuteCalled = true - if stub.ExecutionOrder != nil { - *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) - } - return nil -} - -type RemoveFinalizerStub struct { - IsApplicableCalled bool - ExecuteCalled bool - ExecutionOrder *[]string - StubName string -} - -func (stub *RemoveFinalizerStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { - if stub.IsApplicableCalled { - return false, nil - } - stub.IsApplicableCalled = true - return true, nil -} - -func (stub *RemoveFinalizerStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { - stub.ExecuteCalled = true - if stub.ExecutionOrder != nil { - *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) + *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.UseCaseName) } return nil } -type SkipNonMandatoryErrorStub struct { +type UseCaseErrorStub struct { IsApplicableCalled bool ExecuteCalled bool ExecutionOrder *[]string StubName string + ErrorMessage string } -func (stub *SkipNonMandatoryErrorStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { +func (stub *UseCaseErrorStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { stub.IsApplicableCalled = true return true, nil } -func (stub *SkipNonMandatoryErrorStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { +func (stub *UseCaseErrorStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { stub.ExecuteCalled = true if stub.ExecutionOrder != nil { *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) } - return errors.New("skipNonMandatory failed") + return errors.New(stub.ErrorMessage) } -type SkipNonMandatoryIsApplicableErrorStub struct { +type UseCaseIsApplicableErrorStub struct { IsApplicableCalled bool ExecuteCalled bool ExecutionOrder *[]string StubName string + ErrorMessage string } -func (stub *SkipNonMandatoryIsApplicableErrorStub) IsApplicable(_ context.Context, - _ *v1beta2.ModuleReleaseMeta, -) (bool, error) { +func (stub *UseCaseIsApplicableErrorStub) IsApplicable(_ context.Context, _ *v1beta2.ModuleReleaseMeta) (bool, error) { stub.IsApplicableCalled = true - return false, errors.New("IsApplicable failed") + return false, errors.New(stub.ErrorMessage) } -func (stub *SkipNonMandatoryIsApplicableErrorStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { +func (stub *UseCaseIsApplicableErrorStub) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { stub.ExecuteCalled = true if stub.ExecutionOrder != nil { *stub.ExecutionOrder = append(*stub.ExecutionOrder, stub.StubName) From d94c89a68c732b3a5f7e896c78492274c690edaa Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 16 Oct 2025 16:55:22 +0200 Subject: [PATCH 11/15] chore: Add docs for IsApplicable --- .../mandatorymodule/deletion/usecases/delete_manifests.go | 1 + .../mandatorymodule/deletion/usecases/ensure_finalizer.go | 1 + .../mandatorymodule/deletion/usecases/remove_finalizer.go | 2 ++ .../mandatorymodule/deletion/usecases/skip_non_deleting.go | 1 + 4 files changed, 5 insertions(+) diff --git a/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go b/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go index 09cfc278dd..e687742c96 100644 --- a/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go +++ b/internal/service/mandatorymodule/deletion/usecases/delete_manifests.go @@ -23,6 +23,7 @@ func NewDeleteManifests(repo ManifestRepo) *DeleteManifests { return &DeleteManifests{repo: repo} } +// IsApplicable returns true if the ModuleReleaseMeta has associated manifests, so they should be deleted. func (d *DeleteManifests) IsApplicable(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { manifests, err := d.repo.ListAllForModule(ctx, mrm.Name) if err != nil { diff --git a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go index 1fc2d759e3..5b7df59f46 100644 --- a/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go +++ b/internal/service/mandatorymodule/deletion/usecases/ensure_finalizer.go @@ -22,6 +22,7 @@ func NewEnsureFinalizer(repo MrmEnsureFinalizerRepo) *EnsureFinalizer { return &EnsureFinalizer{repo: repo} } +// IsApplicable returns true if the ModuleReleaseMeta does not contain the mandatory finalizer, so it should be added. func (e *EnsureFinalizer) IsApplicable(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { return !controllerutil.ContainsFinalizer(mrm, shared.MandatoryModuleFinalizer), nil } diff --git a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go index 56a104ebb5..a82542dbf7 100644 --- a/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go +++ b/internal/service/mandatorymodule/deletion/usecases/remove_finalizer.go @@ -22,6 +22,8 @@ func NewRemoveFinalizer(repo MrmRemoFinalizerRepo) *RemoveFinalizer { return &RemoveFinalizer{repo: repo} } +// IsApplicable returns true if the ModuleReleaseMeta contains the mandatory finalizer, so it should be removed. +// This should be the last step in the deletion process. func (e *RemoveFinalizer) IsApplicable(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { return controllerutil.ContainsFinalizer(mrm, shared.MandatoryModuleFinalizer), nil } diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go index b26e8425e9..0982a3759a 100644 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go +++ b/internal/service/mandatorymodule/deletion/usecases/skip_non_deleting.go @@ -14,6 +14,7 @@ func NewSkipNonDeleting() *SkipNonDeleting { return &SkipNonDeleting{} } +// IsApplicable returns true if the ModuleReleaseMeta is not in deleting state, it should be skipped. func (s *SkipNonDeleting) IsApplicable(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { return mrm.DeletionTimestamp.IsZero(), nil } From 2176a4ad41a6653a1bdca4bb54af9c36ed47febe Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 16 Oct 2025 16:56:23 +0200 Subject: [PATCH 12/15] chore: Remove Useless Use Case. This should be handled in the predicates of the controller --- .../deletion/usecases/skip_non_mandatory.go | 23 --------- .../usecases/skip_non_mandatory_test.go | 48 ------------------- 2 files changed, 71 deletions(-) delete mode 100644 internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go delete mode 100644 internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go deleted file mode 100644 index 96749440d6..0000000000 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory.go +++ /dev/null @@ -1,23 +0,0 @@ -package usecases - -import ( - "context" - - "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" -) - -// SkipNonMandatory is a use case that skips ModuleReleaseMetas that are not mandatory. -type SkipNonMandatory struct{} - -func NewSkipNonMandatory() *SkipNonMandatory { - return &SkipNonMandatory{} -} - -func (s *SkipNonMandatory) IsApplicable(_ context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error) { - return mrm.Spec.Mandatory == nil || mrm.Spec.Mandatory.Version == "", nil -} - -func (s *SkipNonMandatory) Execute(_ context.Context, _ *v1beta2.ModuleReleaseMeta) error { - return deletion.ErrMrmNotMandatory -} diff --git a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go b/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go deleted file mode 100644 index f11feb5d3c..0000000000 --- a/internal/service/mandatorymodule/deletion/usecases/skip_non_mandatory_test.go +++ /dev/null @@ -1,48 +0,0 @@ -package usecases_test - -import ( - "context" - "testing" - - "github.com/stretchr/testify/require" - - "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/errors/mandatorymodule/deletion" - "github.com/kyma-project/lifecycle-manager/internal/service/mandatorymodule/deletion/usecases" -) - -func TestSkipNonMandatory_WithNonMandatoryModule(t *testing.T) { - t.Parallel() - - skipNonMandatory := usecases.NewSkipNonMandatory() - mrm := &v1beta2.ModuleReleaseMeta{ - Spec: v1beta2.ModuleReleaseMetaSpec{ - Mandatory: nil, - }, - } - - isApplicable, err := skipNonMandatory.IsApplicable(context.Background(), mrm) - require.NoError(t, err) - require.True(t, isApplicable) - - executeErr := skipNonMandatory.Execute(context.Background(), mrm) - require.Error(t, executeErr) - require.Equal(t, deletion.ErrMrmNotMandatory, executeErr) -} - -func TestSkipNonMandatory_WithMandatoryModule(t *testing.T) { - t.Parallel() - - skipNonMandatory := usecases.NewSkipNonMandatory() - mrm := &v1beta2.ModuleReleaseMeta{ - Spec: v1beta2.ModuleReleaseMetaSpec{ - Mandatory: &v1beta2.Mandatory{ - Version: "1.0.0", - }, - }, - } - - isApplicable, err := skipNonMandatory.IsApplicable(context.Background(), mrm) - require.NoError(t, err) - require.False(t, isApplicable) -} From 45e553121f324b52ba8673d236ddb8b35d8cafca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20Schw=C3=A4gerl?= Date: Fri, 17 Oct 2025 08:17:46 +0200 Subject: [PATCH 13/15] fix linting error --- internal/errors/mandatorymodule/deletion/errors.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/errors/mandatorymodule/deletion/errors.go b/internal/errors/mandatorymodule/deletion/errors.go index d6634c3de6..75c5a3f1a4 100644 --- a/internal/errors/mandatorymodule/deletion/errors.go +++ b/internal/errors/mandatorymodule/deletion/errors.go @@ -2,6 +2,4 @@ package deletion import "errors" -var ( - ErrMrmNotInDeletingState = errors.New("ModuleReleaseMeta not in deleting state") -) +var ErrMrmNotInDeletingState = errors.New("ModuleReleaseMeta not in deleting state") From cf877ddd9af729b46551480f183b82e6ef2e5bdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20Schw=C3=A4gerl?= Date: Fri, 17 Oct 2025 08:25:42 +0200 Subject: [PATCH 14/15] fix line length linting error --- internal/service/mandatorymodule/deletion/deletion_service.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/service/mandatorymodule/deletion/deletion_service.go b/internal/service/mandatorymodule/deletion/deletion_service.go index 1f54b23635..2546749ac4 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service.go +++ b/internal/service/mandatorymodule/deletion/deletion_service.go @@ -31,7 +31,8 @@ func NewService(ensureFinalizer UseCase, } // HandleDeletion processes the deletion of a ModuleReleaseMeta through a series of ordered use cases. -// Returns deletion.ErrMrmNotInDeletingState error if the MRM is not in deleting state, which indicates that the controller should not requeue. +// Returns deletion.ErrMrmNotInDeletingState error if the MRM is not in deleting state, +// which indicates that the controller should not requeue. func (s *Service) HandleDeletion(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { // Find the first applicable step and execute it for _, step := range s.orderedSteps { From 822b1e36dbe1edfc96762cb98c94fb38202bf48a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20Schw=C3=A4gerl?= Date: Fri, 17 Oct 2025 08:58:47 +0200 Subject: [PATCH 15/15] fix lint --- internal/service/mandatorymodule/deletion/deletion_service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/mandatorymodule/deletion/deletion_service.go b/internal/service/mandatorymodule/deletion/deletion_service.go index 2546749ac4..bae305571d 100644 --- a/internal/service/mandatorymodule/deletion/deletion_service.go +++ b/internal/service/mandatorymodule/deletion/deletion_service.go @@ -31,7 +31,7 @@ func NewService(ensureFinalizer UseCase, } // HandleDeletion processes the deletion of a ModuleReleaseMeta through a series of ordered use cases. -// Returns deletion.ErrMrmNotInDeletingState error if the MRM is not in deleting state, +// Returns deletion.ErrMrmNotInDeletingState error if the MRM is not in deleting state, // which indicates that the controller should not requeue. func (s *Service) HandleDeletion(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) error { // Find the first applicable step and execute it