diff --git a/internal/management/controller/database_controller_test.go b/internal/management/controller/database_controller_test.go index f5b27bfd10..be9c8487cf 100644 --- a/internal/management/controller/database_controller_test.go +++ b/internal/management/controller/database_controller_test.go @@ -17,12 +17,14 @@ limitations under the License. package controller import ( + "context" "database/sql" "fmt" "github.com/DATA-DOG/go-sqlmock" "github.com/jackc/pgx/v5" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" @@ -43,26 +45,6 @@ const databaseDetectionQuery = `SELECT count(*) FROM pg_database WHERE datname = $1` -type databaseTesterAdapter struct { - *apiv1.Database -} - -func (w *databaseTesterAdapter) GetStatusApplied() *bool { - return w.Status.Applied -} - -func (w *databaseTesterAdapter) SetObservedGeneration(gen int64) { - w.Status.ObservedGeneration = gen -} - -func (w *databaseTesterAdapter) GetClientObject() client.Object { - return w.Database -} - -func newDatabaseTesterAdapter(db *apiv1.Database) postgresObjectManager { - return &databaseTesterAdapter{db} -} - var _ = Describe("Managed Database status", func() { var ( dbMock sqlmock.Sqlmock @@ -72,7 +54,6 @@ var _ = Describe("Managed Database status", func() { r *DatabaseReconciler fakeClient client.Client err error - tester postgresReconciliationTester[*apiv1.Database] ) BeforeEach(func() { @@ -128,11 +109,6 @@ var _ = Describe("Managed Database status", func() { utils.DatabaseFinalizerName, r.evaluateDropDatabase, ) - - tester = postgresReconciliationTester[*apiv1.Database]{ - cli: fakeClient, - reconcileFunc: r.Reconcile, - } }) AfterEach(func() { @@ -140,11 +116,53 @@ var _ = Describe("Managed Database status", func() { }) It("adds finalizer and sets status ready on success", func(ctx SpecContext) { - tester.setPostgresExpectations(func() { + expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") + dbMock.ExpectQuery(databaseDetectionQuery).WithArgs(database.Spec.Name). + WillReturnRows(expectedValue) + + expectedCreate := sqlmock.NewResult(0, 1) + expectedQuery := fmt.Sprintf( + "CREATE DATABASE %s OWNER %s", + pgx.Identifier{database.Spec.Name}.Sanitize(), + pgx.Identifier{database.Spec.Owner}.Sanitize(), + ) + dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) + + err := reconcileDatabase(ctx, fakeClient, r, database) + Expect(err).ToNot(HaveOccurred()) + + Expect(database.Status.Applied).Should(HaveValue(BeTrue())) + Expect(database.GetStatusMessage()).Should(BeEmpty()) + Expect(database.GetFinalizers()).NotTo(BeEmpty()) + }) + + It("database object inherits error after patching", func(ctx SpecContext) { + expectedError := fmt.Errorf("no permission") + expectedValue := sqlmock.NewRows([]string{""}).AddRow("1") + dbMock.ExpectQuery(databaseDetectionQuery).WithArgs(database.Spec.Name). + WillReturnRows(expectedValue) + + expectedQuery := fmt.Sprintf("ALTER DATABASE %s OWNER TO %s", + pgx.Identifier{database.Spec.Name}.Sanitize(), + pgx.Identifier{database.Spec.Owner}.Sanitize(), + ) + dbMock.ExpectExec(expectedQuery).WillReturnError(expectedError) + + err := reconcileDatabase(ctx, fakeClient, r, database) + Expect(err).ToNot(HaveOccurred()) + + Expect(database.Status.Applied).Should(HaveValue(BeFalse())) + Expect(database.GetStatusMessage()).Should(ContainSubstring(expectedError.Error())) + }) + + When("reclaim policy is delete", func() { + It("on deletion it removes finalizers and drops DB", func(ctx SpecContext) { + // Mocking DetectDB expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") dbMock.ExpectQuery(databaseDetectionQuery).WithArgs(database.Spec.Name). WillReturnRows(expectedValue) + // Mocking CreateDB expectedCreate := sqlmock.NewResult(0, 1) expectedQuery := fmt.Sprintf( "CREATE DATABASE %s OWNER %s", @@ -152,127 +170,76 @@ var _ = Describe("Managed Database status", func() { pgx.Identifier{database.Spec.Owner}.Sanitize(), ) dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - }) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Database) { - Expect(obj.Status.Applied).Should(HaveValue(BeTrue())) - Expect(obj.GetStatusMessage()).Should(BeEmpty()) - Expect(obj.GetFinalizers()).NotTo(BeEmpty()) - }) + // Mocking Drop Database + expectedDrop := fmt.Sprintf("DROP DATABASE IF EXISTS %s", + pgx.Identifier{database.Spec.Name}.Sanitize(), + ) + dbMock.ExpectExec(expectedDrop).WillReturnResult(sqlmock.NewResult(0, 1)) + + err := reconcileDatabase(ctx, fakeClient, r, database) + Expect(err).ToNot(HaveOccurred()) + + // Plain successful reconciliation, finalizers have been created + Expect(database.GetFinalizers()).NotTo(BeEmpty()) + Expect(database.Status.Applied).Should(HaveValue(BeTrue())) + Expect(database.Status.Message).Should(BeEmpty()) + + // The next 2 lines are a hacky bit to make sure the next reconciler + // call doesn't skip on account of Generation == ObservedGeneration. + // See fake.Client known issues with `Generation` + // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder + database.SetGeneration(database.GetGeneration() + 1) + Expect(fakeClient.Update(ctx, database)).To(Succeed()) + + // We now look at the behavior when we delete the Database object + Expect(fakeClient.Delete(ctx, database)).To(Succeed()) - tester.assert(ctx, newDatabaseTesterAdapter(database)) + err = reconcileDatabase(ctx, fakeClient, r, database) + Expect(err).To(HaveOccurred()) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) }) - It("database object inherits error after patching", func(ctx SpecContext) { - expectedError := fmt.Errorf("no permission") - tester.setPostgresExpectations(func() { - expectedValue := sqlmock.NewRows([]string{""}).AddRow("1") + When("reclaim policy is retain", func() { + It("on deletion it removes finalizers and does NOT drop the DB", func(ctx SpecContext) { + database.Spec.ReclaimPolicy = apiv1.DatabaseReclaimRetain + Expect(fakeClient.Update(ctx, database)).To(Succeed()) + + // Mocking DetectDB + expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") dbMock.ExpectQuery(databaseDetectionQuery).WithArgs(database.Spec.Name). WillReturnRows(expectedValue) - expectedQuery := fmt.Sprintf("ALTER DATABASE %s OWNER TO %s", + // Mocking CreateDB + expectedCreate := sqlmock.NewResult(0, 1) + expectedQuery := fmt.Sprintf( + "CREATE DATABASE %s OWNER %s", pgx.Identifier{database.Spec.Name}.Sanitize(), pgx.Identifier{database.Spec.Owner}.Sanitize(), ) - dbMock.ExpectExec(expectedQuery).WillReturnError(expectedError) - }) + dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Database) { - Expect(obj.Status.Applied).Should(HaveValue(BeFalse())) - Expect(obj.GetStatusMessage()).Should(ContainSubstring(expectedError.Error())) - }) + err := reconcileDatabase(ctx, fakeClient, r, database) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newDatabaseTesterAdapter(database)) - }) - - When("reclaim policy is delete", func() { - It("on deletion it removes finalizers and drops DB", func(ctx SpecContext) { - tester.setPostgresExpectations(func() { - // Mocking DetectDB - expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") - dbMock.ExpectQuery(databaseDetectionQuery).WithArgs(database.Spec.Name). - WillReturnRows(expectedValue) - - // Mocking CreateDB - expectedCreate := sqlmock.NewResult(0, 1) - expectedQuery := fmt.Sprintf( - "CREATE DATABASE %s OWNER %s", - pgx.Identifier{database.Spec.Name}.Sanitize(), - pgx.Identifier{database.Spec.Owner}.Sanitize(), - ) - dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - - // Mocking Drop Database - expectedDrop := fmt.Sprintf("DROP DATABASE IF EXISTS %s", - pgx.Identifier{database.Spec.Name}.Sanitize(), - ) - dbMock.ExpectExec(expectedDrop).WillReturnResult(sqlmock.NewResult(0, 1)) - }) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Database) { - // Plain successful reconciliation, finalizers have been created - Expect(obj.GetFinalizers()).NotTo(BeEmpty()) - Expect(obj.Status.Applied).Should(HaveValue(BeTrue())) - Expect(obj.Status.Message).Should(BeEmpty()) - }) - tester.reconcile() - tester.setObjectMutator(func(obj *apiv1.Database) { - // The next 2 lines are a hacky bit to make sure the next reconciler - // call doesn't skip on account of Generation == ObservedGeneration. - // See fake.Client known issues with `Generation` - // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder - obj.SetGeneration(obj.GetGeneration() + 1) - Expect(fakeClient.Update(ctx, obj)).To(Succeed()) - - // We now look at the behavior when we delete the Database object - Expect(fakeClient.Delete(ctx, obj)).To(Succeed()) - }) - tester.setExpectMissingObject() - tester.reconcile() - tester.assert(ctx, newDatabaseTesterAdapter(database)) - }) - }) + // Plain successful reconciliation, finalizers have been created + Expect(database.GetFinalizers()).NotTo(BeEmpty()) + Expect(database.Status.Applied).Should(HaveValue(BeTrue())) + Expect(database.Status.Message).Should(BeEmpty()) - When("reclaim policy is retain", func() { - It("on deletion it removes finalizers and does NOT drop the DB", func(ctx SpecContext) { - database.Spec.ReclaimPolicy = apiv1.DatabaseReclaimRetain + // The next 2 lines are a hacky bit to make sure the next reconciler + // call doesn't skip on account of Generation == ObservedGeneration. + // See fake.Client known issues with `Generation` + // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder + database.SetGeneration(database.GetGeneration() + 1) Expect(fakeClient.Update(ctx, database)).To(Succeed()) - tester.setPostgresExpectations(func() { - // Mocking DetectDB - expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") - dbMock.ExpectQuery(databaseDetectionQuery).WithArgs(database.Spec.Name). - WillReturnRows(expectedValue) - - // Mocking CreateDB - expectedCreate := sqlmock.NewResult(0, 1) - expectedQuery := fmt.Sprintf( - "CREATE DATABASE %s OWNER %s", - pgx.Identifier{database.Spec.Name}.Sanitize(), - pgx.Identifier{database.Spec.Owner}.Sanitize(), - ) - dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - }) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Database) { - // Plain successful reconciliation, finalizers have been created - Expect(obj.GetFinalizers()).NotTo(BeEmpty()) - Expect(obj.Status.Applied).Should(HaveValue(BeTrue())) - Expect(obj.Status.Message).Should(BeEmpty()) - }) - tester.reconcile() - tester.setObjectMutator(func(obj *apiv1.Database) { - // The next 2 lines are a hacky bit to make sure the next reconciler - // call doesn't skip on account of Generation == ObservedGeneration. - // See fake.Client known issues with `Generation` - // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder - obj.SetGeneration(obj.GetGeneration() + 1) - Expect(fakeClient.Update(ctx, obj)).To(Succeed()) - - // We now look at the behavior when we delete the Database object - Expect(fakeClient.Delete(ctx, obj)).To(Succeed()) - }) - tester.setExpectMissingObject() - tester.reconcile() - tester.assert(ctx, newDatabaseTesterAdapter(database)) + // We now look at the behavior when we delete the Database object + Expect(fakeClient.Delete(ctx, database)).To(Succeed()) + + err = reconcileDatabase(ctx, fakeClient, r, database) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) }) @@ -297,13 +264,12 @@ var _ = Describe("Managed Database status", func() { database.Spec.ClusterRef.Name = "cluster-other" Expect(fakeClient.Update(ctx, database)).To(Succeed()) - tester.reconcileFunc = r.Reconcile - tester.setUpdatedObjectExpectations(func(obj *apiv1.Database) { - Expect(obj.Status.Applied).Should(HaveValue(BeFalse())) - Expect(obj.Status.Message).Should(ContainSubstring( - fmt.Sprintf("%q not found", database.Spec.ClusterRef.Name))) - }) - tester.assert(ctx, newDatabaseTesterAdapter(database)) + err := reconcileDatabase(ctx, fakeClient, r, database) + Expect(err).ToNot(HaveOccurred()) + + Expect(database.Status.Applied).Should(HaveValue(BeFalse())) + Expect(database.Status.Message).Should(ContainSubstring( + fmt.Sprintf("%q not found", database.Spec.ClusterRef.Name))) }) It("skips reconciliation if database object isn't found (deleted database)", func(ctx SpecContext) { @@ -339,21 +305,19 @@ var _ = Describe("Managed Database status", func() { database.Spec.Ensure = apiv1.EnsureAbsent Expect(fakeClient.Update(ctx, database)).To(Succeed()) - tester.setPostgresExpectations(func() { - expectedValue := sqlmock.NewResult(0, 1) - expectedQuery := fmt.Sprintf( - "DROP DATABASE IF EXISTS %s", - pgx.Identifier{database.Spec.Name}.Sanitize(), - ) - dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedValue) - }) + expectedValue := sqlmock.NewResult(0, 1) + expectedQuery := fmt.Sprintf( + "DROP DATABASE IF EXISTS %s", + pgx.Identifier{database.Spec.Name}.Sanitize(), + ) + dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedValue) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Database) { - Expect(obj.Status.Applied).To(HaveValue(BeTrue())) - Expect(obj.Status.Message).To(BeEmpty()) - Expect(obj.Status.ObservedGeneration).To(BeEquivalentTo(1)) - }) - tester.assert(ctx, newDatabaseTesterAdapter(database)) + err := reconcileDatabase(ctx, fakeClient, r, database) + Expect(err).ToNot(HaveOccurred()) + + Expect(database.Status.Applied).To(HaveValue(BeTrue())) + Expect(database.Status.Message).To(BeEmpty()) + Expect(database.Status.ObservedGeneration).To(BeEquivalentTo(1)) }) It("marks as failed if the target Database is already being managed", func(ctx SpecContext) { @@ -380,15 +344,14 @@ var _ = Describe("Managed Database status", func() { // Expect(fakeClient.Create(ctx, currentManager)).To(Succeed()) Expect(fakeClient.Create(ctx, dbDuplicate)).To(Succeed()) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Database) { - expectedError := fmt.Sprintf("%q is already managed by object %q", - dbDuplicate.Spec.Name, database.Name) - Expect(obj.Status.Applied).To(HaveValue(BeFalse())) - Expect(obj.Status.Message).To(ContainSubstring(expectedError)) - Expect(obj.Status.ObservedGeneration).To(BeZero()) - }) + err := reconcileDatabase(ctx, fakeClient, r, dbDuplicate) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newDatabaseTesterAdapter(dbDuplicate)) + expectedError := fmt.Sprintf("%q is already managed by object %q", + dbDuplicate.Spec.Name, database.Name) + Expect(dbDuplicate.Status.Applied).To(HaveValue(BeFalse())) + Expect(dbDuplicate.Status.Message).To(ContainSubstring(expectedError)) + Expect(dbDuplicate.Status.ObservedGeneration).To(BeZero()) }) It("properly signals a database is on a replica cluster", func(ctx SpecContext) { @@ -398,10 +361,28 @@ var _ = Describe("Managed Database status", func() { } Expect(fakeClient.Patch(ctx, cluster, client.MergeFrom(initialCluster))).To(Succeed()) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Database) { - Expect(obj.Status.Applied).Should(BeNil()) - Expect(obj.Status.Message).Should(ContainSubstring("waiting for the cluster to become primary")) - }) - tester.assert(ctx, newDatabaseTesterAdapter(database)) + err := reconcileDatabase(ctx, fakeClient, r, database) + Expect(err).ToNot(HaveOccurred()) + + Expect(database.Status.Applied).Should(BeNil()) + Expect(database.Status.Message).Should(ContainSubstring("waiting for the cluster to become primary")) }) }) + +func reconcileDatabase( + ctx context.Context, + fakeClient client.Client, + r *DatabaseReconciler, + database *apiv1.Database, +) error { + GinkgoT().Helper() + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{ + Namespace: database.GetNamespace(), + Name: database.GetName(), + }}) + Expect(err).ToNot(HaveOccurred()) + return fakeClient.Get(ctx, client.ObjectKey{ + Namespace: database.GetNamespace(), + Name: database.GetName(), + }, database) +} diff --git a/internal/management/controller/generic_controller_asserts_test.go b/internal/management/controller/generic_controller_asserts_test.go deleted file mode 100644 index b33e4edb1a..0000000000 --- a/internal/management/controller/generic_controller_asserts_test.go +++ /dev/null @@ -1,119 +0,0 @@ -package controller - -import ( - "context" - - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - - . "github.com/onsi/gomega" -) - -type postgresObjectManager interface { - GetStatusApplied() *bool - GetStatusMessage() string - SetObservedGeneration(gen int64) - GetClientObject() client.Object -} - -type ( - objectMutatorFunc[T client.Object] func(obj T) - postgresExpectationsFunc func() - updatedObjectExpectationsFunc[T client.Object] func(newObj T) - reconciliation[T client.Object] struct { - objectMutator objectMutatorFunc[T] - postgresExpectations postgresExpectationsFunc - updatedObjectExpectations updatedObjectExpectationsFunc[T] - expectMissingObject bool - } -) - -type postgresReconciliationTester[T client.Object] struct { - cli client.Client - reconcileFunc func(ctx context.Context, req ctrl.Request) (ctrl.Result, error) - objectMutator objectMutatorFunc[T] - postgresExpectations postgresExpectationsFunc - updatedObjectExpectations updatedObjectExpectationsFunc[T] - expectMissingObject bool - reconciliations []reconciliation[T] -} - -func (pr *postgresReconciliationTester[T]) setObjectMutator(objectMutator objectMutatorFunc[T]) { - pr.objectMutator = objectMutator -} - -func (pr *postgresReconciliationTester[T]) setPostgresExpectations( - postgresExpectations postgresExpectationsFunc, -) { - pr.postgresExpectations = postgresExpectations -} - -func (pr *postgresReconciliationTester[T]) setUpdatedObjectExpectations( - updatedObjectExpectations updatedObjectExpectationsFunc[T], -) { - pr.updatedObjectExpectations = updatedObjectExpectations -} - -func (pr *postgresReconciliationTester[T]) setExpectMissingObject() { - pr.expectMissingObject = true -} - -func (pr *postgresReconciliationTester[T]) reconcile() { - if pr.postgresExpectations == nil && pr.updatedObjectExpectations == nil && !pr.expectMissingObject { - return - } - - pr.reconciliations = append(pr.reconciliations, reconciliation[T]{ - objectMutator: pr.objectMutator, - postgresExpectations: pr.postgresExpectations, - updatedObjectExpectations: pr.updatedObjectExpectations, - expectMissingObject: pr.expectMissingObject, - }) - - pr.objectMutator = nil - pr.postgresExpectations = nil - pr.updatedObjectExpectations = nil - pr.expectMissingObject = false -} - -func (pr *postgresReconciliationTester[T]) assert( - ctx context.Context, - wrapper postgresObjectManager, -) { - obj := wrapper.GetClientObject() - Expect(obj.GetFinalizers()).To(BeEmpty()) - - pr.reconcile() - for _, r := range pr.reconciliations { - if r.postgresExpectations != nil { - r.postgresExpectations() - } - - if r.objectMutator != nil { - r.objectMutator(wrapper.GetClientObject().(T)) - } - - _, err := pr.reconcileFunc(ctx, ctrl.Request{NamespacedName: types.NamespacedName{ - Namespace: obj.GetNamespace(), - Name: obj.GetName(), - }}) - Expect(err).ToNot(HaveOccurred()) - - err = pr.cli.Get(ctx, client.ObjectKey{ - Namespace: obj.GetNamespace(), - Name: obj.GetName(), - }, wrapper.GetClientObject()) - if r.expectMissingObject { - Expect(err).To(HaveOccurred()) - Expect(apierrors.IsNotFound(err)).To(BeTrue()) - } else { - Expect(err).ToNot(HaveOccurred()) - } - - if r.updatedObjectExpectations != nil { - r.updatedObjectExpectations(wrapper.GetClientObject().(T)) - } - } -} diff --git a/internal/management/controller/publication_controller_test.go b/internal/management/controller/publication_controller_test.go index 33bfab7a2d..ea77ba6002 100644 --- a/internal/management/controller/publication_controller_test.go +++ b/internal/management/controller/publication_controller_test.go @@ -17,12 +17,14 @@ limitations under the License. package controller import ( + "context" "database/sql" "fmt" "github.com/DATA-DOG/go-sqlmock" "github.com/jackc/pgx/v5" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" @@ -43,26 +45,6 @@ const publicationDetectionQuery = `SELECT count(*) FROM pg_publication WHERE pubname = $1` -type publicationTesterAdapter struct { - *apiv1.Publication -} - -func (p publicationTesterAdapter) GetStatusApplied() *bool { - return p.Status.Applied -} - -func (p publicationTesterAdapter) SetObservedGeneration(gen int64) { - p.Status.ObservedGeneration = gen -} - -func (p publicationTesterAdapter) GetClientObject() client.Object { - return p.Publication -} - -func newPublicationTesterAdapter(p *apiv1.Publication) postgresObjectManager { - return publicationTesterAdapter{p} -} - var _ = Describe("Managed publication controller tests", func() { var ( dbMock sqlmock.Sqlmock @@ -72,7 +54,6 @@ var _ = Describe("Managed publication controller tests", func() { r *PublicationReconciler fakeClient client.Client err error - tester postgresReconciliationTester[*apiv1.Publication] ) BeforeEach(func() { @@ -133,10 +114,6 @@ var _ = Describe("Managed publication controller tests", func() { utils.PublicationFinalizerName, r.evaluateDropPublication, ) - tester = postgresReconciliationTester[*apiv1.Publication]{ - reconcileFunc: r.Reconcile, - cli: fakeClient, - } }) AfterEach(func() { @@ -144,92 +121,85 @@ var _ = Describe("Managed publication controller tests", func() { }) It("adds finalizer and sets status ready on success", func(ctx SpecContext) { - tester.setPostgresExpectations(func() { - noHits := sqlmock.NewRows([]string{""}).AddRow("0") + noHits := sqlmock.NewRows([]string{""}).AddRow("0") + dbMock.ExpectQuery(publicationDetectionQuery).WithArgs(publication.Spec.Name). + WillReturnRows(noHits) + + expectedCreate := sqlmock.NewResult(0, 1) + expectedQuery := fmt.Sprintf( + "CREATE PUBLICATION %s FOR ALL TABLES", + pgx.Identifier{publication.Spec.Name}.Sanitize(), + ) + dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) + + err := reconcilePublication(ctx, fakeClient, r, publication) + Expect(err).ToNot(HaveOccurred()) + + Expect(publication.Status.Applied).Should(HaveValue(BeTrue())) + Expect(publication.GetStatusMessage()).Should(BeEmpty()) + Expect(publication.GetFinalizers()).NotTo(BeEmpty()) + }) + + It("publication object inherits error after patching", func(ctx SpecContext) { + expectedError := fmt.Errorf("no permission") + oneHit := sqlmock.NewRows([]string{""}).AddRow("1") + dbMock.ExpectQuery(publicationDetectionQuery).WithArgs(publication.Spec.Name). + WillReturnRows(oneHit) + + expectedQuery := fmt.Sprintf("ALTER PUBLICATION %s SET TABLES IN SCHEMA \"public\"", + pgx.Identifier{publication.Spec.Name}.Sanitize(), + ) + dbMock.ExpectExec(expectedQuery).WillReturnError(expectedError) + + err := reconcilePublication(ctx, fakeClient, r, publication) + Expect(err).ToNot(HaveOccurred()) + + Expect(publication.Status.Applied).Should(HaveValue(BeFalse())) + Expect(publication.Status.Message).Should(ContainSubstring(expectedError.Error())) + }) + + When("reclaim policy is delete", func() { + It("on deletion it removes finalizers and drops the Publication", func(ctx SpecContext) { + // Mocking Detect publication + expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") dbMock.ExpectQuery(publicationDetectionQuery).WithArgs(publication.Spec.Name). - WillReturnRows(noHits) + WillReturnRows(expectedValue) + // Mocking Create publication expectedCreate := sqlmock.NewResult(0, 1) expectedQuery := fmt.Sprintf( "CREATE PUBLICATION %s FOR ALL TABLES", pgx.Identifier{publication.Spec.Name}.Sanitize(), ) dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - }) - - tester.setUpdatedObjectExpectations(func(obj *apiv1.Publication) { - Expect(obj.Status.Applied).Should(HaveValue(BeTrue())) - Expect(obj.GetStatusMessage()).Should(BeEmpty()) - Expect(obj.GetFinalizers()).NotTo(BeEmpty()) - }) - - tester.assert(ctx, newPublicationTesterAdapter(publication)) - }) - - It("publication object inherits error after patching", func(ctx SpecContext) { - expectedError := fmt.Errorf("no permission") - tester.setPostgresExpectations(func() { - oneHit := sqlmock.NewRows([]string{""}).AddRow("1") - dbMock.ExpectQuery(publicationDetectionQuery).WithArgs(publication.Spec.Name). - WillReturnRows(oneHit) - expectedQuery := fmt.Sprintf("ALTER PUBLICATION %s SET TABLES IN SCHEMA \"public\"", + // Mocking Drop Publication + expectedDrop := fmt.Sprintf("DROP PUBLICATION IF EXISTS %s", pgx.Identifier{publication.Spec.Name}.Sanitize(), ) - dbMock.ExpectExec(expectedQuery).WillReturnError(expectedError) - }) + dbMock.ExpectExec(expectedDrop).WillReturnResult(sqlmock.NewResult(0, 1)) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Publication) { - Expect(obj.Status.Applied).Should(HaveValue(BeFalse())) - Expect(obj.Status.Message).Should(ContainSubstring(expectedError.Error())) - }) + err := reconcilePublication(ctx, fakeClient, r, publication) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newPublicationTesterAdapter(publication)) - }) + // Plain successful reconciliation, finalizers have been created + Expect(publication.GetFinalizers()).NotTo(BeEmpty()) + Expect(publication.Status.Applied).Should(HaveValue(BeTrue())) + Expect(publication.Status.Message).Should(BeEmpty()) - When("reclaim policy is delete", func() { - It("on deletion it removes finalizers and drops the Publication", func(ctx SpecContext) { - tester.setPostgresExpectations(func() { - // Mocking Detect publication - expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") - dbMock.ExpectQuery(publicationDetectionQuery).WithArgs(publication.Spec.Name). - WillReturnRows(expectedValue) - - // Mocking Create publication - expectedCreate := sqlmock.NewResult(0, 1) - expectedQuery := fmt.Sprintf( - "CREATE PUBLICATION %s FOR ALL TABLES", - pgx.Identifier{publication.Spec.Name}.Sanitize(), - ) - dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - - // Mocking Drop Publication - expectedDrop := fmt.Sprintf("DROP PUBLICATION IF EXISTS %s", - pgx.Identifier{publication.Spec.Name}.Sanitize(), - ) - dbMock.ExpectExec(expectedDrop).WillReturnResult(sqlmock.NewResult(0, 1)) - }) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Publication) { - // Plain successful reconciliation, finalizers have been created - Expect(obj.GetFinalizers()).NotTo(BeEmpty()) - Expect(obj.Status.Applied).Should(HaveValue(BeTrue())) - Expect(obj.Status.Message).Should(BeEmpty()) - }) - tester.reconcile() - tester.setObjectMutator(func(obj *apiv1.Publication) { - // The next 2 lines are a hacky bit to make sure the next reconciler - // call doesn't skip on account of Generation == ObservedGeneration. - // See fake.Client known issues with `Generation` - // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder - obj.SetGeneration(obj.GetGeneration() + 1) - Expect(fakeClient.Update(ctx, obj)).To(Succeed()) - - // We now look at the behavior when we delete the Database object - Expect(fakeClient.Delete(ctx, obj)).To(Succeed()) - }) - tester.setExpectMissingObject() - tester.reconcile() - tester.assert(ctx, newPublicationTesterAdapter(publication)) + // The next 2 lines are a hacky bit to make sure the next reconciler + // call doesn't skip on account of Generation == ObservedGeneration. + // See fake.Client known issues with `Generation` + // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder + publication.SetGeneration(publication.GetGeneration() + 1) + Expect(fakeClient.Update(ctx, publication)).To(Succeed()) + + // We now look at the behavior when we delete the Database object + Expect(fakeClient.Delete(ctx, publication)).To(Succeed()) + + err = reconcilePublication(ctx, fakeClient, r, publication) + Expect(err).To(HaveOccurred()) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) }) @@ -238,41 +208,40 @@ var _ = Describe("Managed publication controller tests", func() { publication.Spec.ReclaimPolicy = apiv1.PublicationReclaimRetain Expect(fakeClient.Update(ctx, publication)).To(Succeed()) - tester.setPostgresExpectations(func() { - // Mocking Detect publication - expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") - dbMock.ExpectQuery(publicationDetectionQuery).WithArgs(publication.Spec.Name). - WillReturnRows(expectedValue) - - // Mocking Create publication - expectedCreate := sqlmock.NewResult(0, 1) - expectedQuery := fmt.Sprintf( - "CREATE PUBLICATION %s FOR ALL TABLES", - pgx.Identifier{publication.Spec.Name}.Sanitize(), - ) - dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - }) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Publication) { - // Plain successful reconciliation, finalizers have been created - Expect(obj.GetFinalizers()).NotTo(BeEmpty()) - Expect(obj.Status.Applied).Should(HaveValue(BeTrue())) - Expect(obj.Status.Message).Should(BeEmpty()) - }) - tester.reconcile() - tester.setObjectMutator(func(obj *apiv1.Publication) { - // The next 2 lines are a hacky bit to make sure the next reconciler - // call doesn't skip on account of Generation == ObservedGeneration. - // See fake.Client known issues with `Generation` - // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder - obj.SetGeneration(obj.GetGeneration() + 1) - Expect(fakeClient.Update(ctx, obj)).To(Succeed()) - - // We now look at the behavior when we delete the Database object - Expect(fakeClient.Delete(ctx, obj)).To(Succeed()) - }) - tester.setExpectMissingObject() - tester.reconcile() - tester.assert(ctx, newPublicationTesterAdapter(publication)) + // Mocking Detect publication + expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") + dbMock.ExpectQuery(publicationDetectionQuery).WithArgs(publication.Spec.Name). + WillReturnRows(expectedValue) + + // Mocking Create publication + expectedCreate := sqlmock.NewResult(0, 1) + expectedQuery := fmt.Sprintf( + "CREATE PUBLICATION %s FOR ALL TABLES", + pgx.Identifier{publication.Spec.Name}.Sanitize(), + ) + dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) + + err := reconcilePublication(ctx, fakeClient, r, publication) + Expect(err).ToNot(HaveOccurred()) + + // Plain successful reconciliation, finalizers have been created + Expect(publication.GetFinalizers()).NotTo(BeEmpty()) + Expect(publication.Status.Applied).Should(HaveValue(BeTrue())) + Expect(publication.Status.Message).Should(BeEmpty()) + + // The next 2 lines are a hacky bit to make sure the next reconciler + // call doesn't skip on account of Generation == ObservedGeneration. + // See fake.Client known issues with `Generation` + // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder + publication.SetGeneration(publication.GetGeneration() + 1) + Expect(fakeClient.Update(ctx, publication)).To(Succeed()) + + // We now look at the behavior when we delete the Database object + Expect(fakeClient.Delete(ctx, publication)).To(Succeed()) + + err = reconcilePublication(ctx, fakeClient, r, publication) + Expect(err).To(HaveOccurred()) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) }) @@ -293,19 +262,16 @@ var _ = Describe("Managed publication controller tests", func() { }, } - tester.reconcileFunc = r.Reconcile - // Updating the publication object to reference the newly created Cluster publication.Spec.ClusterRef.Name = "cluster-other" Expect(fakeClient.Update(ctx, publication)).To(Succeed()) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Publication) { - Expect(obj.Status.Applied).Should(HaveValue(BeFalse())) - Expect(obj.GetStatusMessage()).Should(ContainSubstring( - fmt.Sprintf("%q not found", publication.Spec.ClusterRef.Name))) - }) + err := reconcilePublication(ctx, fakeClient, r, publication) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newPublicationTesterAdapter(publication)) + Expect(publication.Status.Applied).Should(HaveValue(BeFalse())) + Expect(publication.GetStatusMessage()).Should(ContainSubstring( + fmt.Sprintf("%q not found", publication.Spec.ClusterRef.Name))) }) It("skips reconciliation if publication object isn't found (deleted publication)", func(ctx SpecContext) { @@ -358,15 +324,14 @@ var _ = Describe("Managed publication controller tests", func() { // Expect(fakeClient.Create(ctx, currentManager)).To(Succeed()) Expect(fakeClient.Create(ctx, pubDuplicate)).To(Succeed()) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Publication) { - expectedError := fmt.Sprintf("%q is already managed by object %q", - pubDuplicate.Spec.Name, publication.Name) - Expect(obj.Status.Applied).To(HaveValue(BeFalse())) - Expect(obj.Status.Message).To(ContainSubstring(expectedError)) - Expect(obj.Status.ObservedGeneration).To(BeZero()) - }) + err := reconcilePublication(ctx, fakeClient, r, pubDuplicate) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newPublicationTesterAdapter(pubDuplicate)) + expectedError := fmt.Sprintf("%q is already managed by object %q", + pubDuplicate.Spec.Name, publication.Name) + Expect(pubDuplicate.Status.Applied).To(HaveValue(BeFalse())) + Expect(pubDuplicate.Status.Message).To(ContainSubstring(expectedError)) + Expect(pubDuplicate.Status.ObservedGeneration).To(BeZero()) }) It("properly signals a publication is on a replica cluster", func(ctx SpecContext) { @@ -376,11 +341,28 @@ var _ = Describe("Managed publication controller tests", func() { } Expect(fakeClient.Patch(ctx, cluster, client.MergeFrom(initialCluster))).To(Succeed()) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Publication) { - Expect(obj.Status.Applied).Should(BeNil()) - Expect(obj.Status.Message).Should(ContainSubstring("waiting for the cluster to become primary")) - }) + err := reconcilePublication(ctx, fakeClient, r, publication) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newPublicationTesterAdapter(publication)) + Expect(publication.Status.Applied).Should(BeNil()) + Expect(publication.Status.Message).Should(ContainSubstring("waiting for the cluster to become primary")) }) }) + +func reconcilePublication( + ctx context.Context, + fakeClient client.Client, + r *PublicationReconciler, + publication *apiv1.Publication, +) error { + GinkgoT().Helper() + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{ + Namespace: publication.GetNamespace(), + Name: publication.GetName(), + }}) + Expect(err).ToNot(HaveOccurred()) + return fakeClient.Get(ctx, client.ObjectKey{ + Namespace: publication.GetNamespace(), + Name: publication.GetName(), + }, publication) +} diff --git a/internal/management/controller/subscription_controller_test.go b/internal/management/controller/subscription_controller_test.go index edbd1e2814..f6afdc0c4e 100644 --- a/internal/management/controller/subscription_controller_test.go +++ b/internal/management/controller/subscription_controller_test.go @@ -17,6 +17,7 @@ limitations under the License. package controller import ( + "context" "database/sql" "fmt" @@ -24,6 +25,7 @@ import ( "github.com/jackc/pgx/v5" "github.com/lib/pq" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" @@ -44,26 +46,6 @@ const subscriptionDetectionQuery = `SELECT count(*) FROM pg_subscription WHERE subname = $1` -type subscriptionTesterAdapter struct { - *apiv1.Subscription -} - -func (s *subscriptionTesterAdapter) GetStatusApplied() *bool { - return s.Status.Applied -} - -func (s *subscriptionTesterAdapter) SetObservedGeneration(gen int64) { - s.Status.ObservedGeneration = gen -} - -func (s *subscriptionTesterAdapter) GetClientObject() client.Object { - return s.Subscription -} - -func newSubscriptionTesterAdapter(subscription *apiv1.Subscription) postgresObjectManager { - return &subscriptionTesterAdapter{Subscription: subscription} -} - var _ = Describe("Managed subscription controller tests", func() { var ( dbMock sqlmock.Sqlmock @@ -74,7 +56,6 @@ var _ = Describe("Managed subscription controller tests", func() { fakeClient client.Client connString string err error - tester postgresReconciliationTester[*apiv1.Subscription] ) BeforeEach(func() { @@ -145,11 +126,6 @@ var _ = Describe("Managed subscription controller tests", func() { utils.SubscriptionFinalizerName, r.evaluateDropSubscription, ) - - tester = postgresReconciliationTester[*apiv1.Subscription]{ - reconcileFunc: r.Reconcile, - cli: fakeClient, - } }) AfterEach(func() { @@ -157,11 +133,62 @@ var _ = Describe("Managed subscription controller tests", func() { }) It("adds finalizer and sets status ready on success", func(ctx SpecContext) { - tester.setPostgresExpectations(func() { - noHits := sqlmock.NewRows([]string{""}).AddRow("0") + noHits := sqlmock.NewRows([]string{""}).AddRow("0") + dbMock.ExpectQuery(subscriptionDetectionQuery).WithArgs(subscription.Spec.Name). + WillReturnRows(noHits) + + expectedCreate := sqlmock.NewResult(0, 1) + expectedQuery := fmt.Sprintf( + "CREATE SUBSCRIPTION %s CONNECTION %s PUBLICATION %s", + pgx.Identifier{subscription.Spec.Name}.Sanitize(), + pq.QuoteLiteral(connString), + pgx.Identifier{subscription.Spec.PublicationName}.Sanitize(), + ) + dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) + + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{ + Namespace: subscription.GetNamespace(), + Name: subscription.GetName(), + }}) + Expect(err).ToNot(HaveOccurred()) + err = fakeClient.Get(ctx, client.ObjectKey{ + Namespace: subscription.GetNamespace(), + Name: subscription.GetName(), + }, subscription) + Expect(err).ToNot(HaveOccurred()) + + Expect(subscription.Status.Applied).Should(HaveValue(BeTrue())) + Expect(subscription.GetStatusMessage()).Should(BeEmpty()) + Expect(subscription.GetFinalizers()).NotTo(BeEmpty()) + }) + + It("subscription object inherits error after patching", func(ctx SpecContext) { + expectedError := fmt.Errorf("no permission") + oneHit := sqlmock.NewRows([]string{""}).AddRow("1") + dbMock.ExpectQuery(subscriptionDetectionQuery).WithArgs(subscription.Spec.Name). + WillReturnRows(oneHit) + + expectedQuery := fmt.Sprintf("ALTER SUBSCRIPTION %s SET PUBLICATION %s", + pgx.Identifier{subscription.Spec.Name}.Sanitize(), + pgx.Identifier{subscription.Spec.PublicationName}.Sanitize(), + ) + dbMock.ExpectExec(expectedQuery).WillReturnError(expectedError) + + err = reconcileSubscription(ctx, fakeClient, r, subscription) + Expect(err).ToNot(HaveOccurred()) + + Expect(subscription.Status.Applied).Should(HaveValue(BeFalse())) + Expect(subscription.Status.Message).Should(ContainSubstring(expectedError.Error())) + }) + + When("reclaim policy is delete", func() { + It("on deletion it removes finalizers and drops the subscription", func(ctx SpecContext) { + // Mocking detection of subscriptions + expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") dbMock.ExpectQuery(subscriptionDetectionQuery).WithArgs(subscription.Spec.Name). - WillReturnRows(noHits) + WillReturnRows(expectedValue) + // Mocking create subscription expectedCreate := sqlmock.NewResult(0, 1) expectedQuery := fmt.Sprintf( "CREATE SUBSCRIPTION %s CONNECTION %s PUBLICATION %s", @@ -170,84 +197,34 @@ var _ = Describe("Managed subscription controller tests", func() { pgx.Identifier{subscription.Spec.PublicationName}.Sanitize(), ) dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - }) - - tester.setUpdatedObjectExpectations(func(obj *apiv1.Subscription) { - Expect(obj.Status.Applied).Should(HaveValue(BeTrue())) - Expect(obj.GetStatusMessage()).Should(BeEmpty()) - Expect(obj.GetFinalizers()).NotTo(BeEmpty()) - }) - - tester.assert(ctx, newSubscriptionTesterAdapter(subscription)) - }) - - It("subscription object inherits error after patching", func(ctx SpecContext) { - expectedError := fmt.Errorf("no permission") - tester.setPostgresExpectations(func() { - oneHit := sqlmock.NewRows([]string{""}).AddRow("1") - dbMock.ExpectQuery(subscriptionDetectionQuery).WithArgs(subscription.Spec.Name). - WillReturnRows(oneHit) - expectedQuery := fmt.Sprintf("ALTER SUBSCRIPTION %s SET PUBLICATION %s", + // Mocking Drop subscription + expectedDrop := fmt.Sprintf("DROP SUBSCRIPTION IF EXISTS %s", pgx.Identifier{subscription.Spec.Name}.Sanitize(), - pgx.Identifier{subscription.Spec.PublicationName}.Sanitize(), ) - dbMock.ExpectExec(expectedQuery).WillReturnError(expectedError) - }) + dbMock.ExpectExec(expectedDrop).WillReturnResult(sqlmock.NewResult(0, 1)) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Subscription) { - Expect(obj.Status.Applied).Should(HaveValue(BeFalse())) - Expect(obj.Status.Message).Should(ContainSubstring(expectedError.Error())) - }) + err = reconcileSubscription(ctx, fakeClient, r, subscription) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newSubscriptionTesterAdapter(subscription)) - }) + // Plain successful reconciliation, finalizers have been created + Expect(subscription.GetFinalizers()).NotTo(BeEmpty()) + Expect(subscription.Status.Applied).Should(HaveValue(BeTrue())) + Expect(subscription.Status.Message).Should(BeEmpty()) - When("reclaim policy is delete", func() { - It("on deletion it removes finalizers and drops the subscription", func(ctx SpecContext) { - tester.setPostgresExpectations(func() { - // Mocking detection of subscriptions - expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") - dbMock.ExpectQuery(subscriptionDetectionQuery).WithArgs(subscription.Spec.Name). - WillReturnRows(expectedValue) - - // Mocking create subscription - expectedCreate := sqlmock.NewResult(0, 1) - expectedQuery := fmt.Sprintf( - "CREATE SUBSCRIPTION %s CONNECTION %s PUBLICATION %s", - pgx.Identifier{subscription.Spec.Name}.Sanitize(), - pq.QuoteLiteral(connString), - pgx.Identifier{subscription.Spec.PublicationName}.Sanitize(), - ) - dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - - // Mocking Drop subscription - expectedDrop := fmt.Sprintf("DROP SUBSCRIPTION IF EXISTS %s", - pgx.Identifier{subscription.Spec.Name}.Sanitize(), - ) - dbMock.ExpectExec(expectedDrop).WillReturnResult(sqlmock.NewResult(0, 1)) - }) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Subscription) { - // Plain successful reconciliation, finalizers have been created - Expect(obj.GetFinalizers()).NotTo(BeEmpty()) - Expect(obj.Status.Applied).Should(HaveValue(BeTrue())) - Expect(obj.Status.Message).Should(BeEmpty()) - }) - tester.reconcile() - tester.setObjectMutator(func(obj *apiv1.Subscription) { - // The next 2 lines are a hacky bit to make sure the next reconciler - // call doesn't skip on account of Generation == ObservedGeneration. - // See fake.Client known issues with `Generation` - // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder - obj.SetGeneration(obj.GetGeneration() + 1) - Expect(fakeClient.Update(ctx, obj)).To(Succeed()) - - // We now look at the behavior when we delete the Database object - Expect(fakeClient.Delete(ctx, obj)).To(Succeed()) - }) - tester.setExpectMissingObject() - tester.reconcile() - tester.assert(ctx, newSubscriptionTesterAdapter(subscription)) + // The next 2 lines are a hacky bit to make sure the next reconciler + // call doesn't skip on account of Generation == ObservedGeneration. + // See fake.Client known issues with `Generation` + // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder + subscription.SetGeneration(subscription.GetGeneration() + 1) + Expect(fakeClient.Update(ctx, subscription)).To(Succeed()) + + // We now look at the behavior when we delete the Database object + Expect(fakeClient.Delete(ctx, subscription)).To(Succeed()) + + err = reconcileSubscription(ctx, fakeClient, r, subscription) + Expect(err).To(HaveOccurred()) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) }) @@ -256,43 +233,42 @@ var _ = Describe("Managed subscription controller tests", func() { subscription.Spec.ReclaimPolicy = apiv1.SubscriptionReclaimRetain Expect(fakeClient.Update(ctx, subscription)).To(Succeed()) - tester.setPostgresExpectations(func() { - // Mocking Detect subscription - expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") - dbMock.ExpectQuery(subscriptionDetectionQuery).WithArgs(subscription.Spec.Name). - WillReturnRows(expectedValue) - - // Mocking Create subscription - expectedCreate := sqlmock.NewResult(0, 1) - expectedQuery := fmt.Sprintf( - "CREATE SUBSCRIPTION %s CONNECTION %s PUBLICATION %s", - pgx.Identifier{subscription.Spec.Name}.Sanitize(), - pq.QuoteLiteral(connString), - pgx.Identifier{subscription.Spec.PublicationName}.Sanitize(), - ) - dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) - }) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Subscription) { - // Plain successful reconciliation, finalizers have been created - Expect(obj.GetFinalizers()).NotTo(BeEmpty()) - Expect(obj.Status.Applied).Should(HaveValue(BeTrue())) - Expect(obj.Status.Message).Should(BeEmpty()) - }) - tester.reconcile() - tester.setObjectMutator(func(obj *apiv1.Subscription) { - // The next 2 lines are a hacky bit to make sure the next reconciler - // call doesn't skip on account of Generation == ObservedGeneration. - // See fake.Client known issues with `Generation` - // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder - obj.SetGeneration(obj.GetGeneration() + 1) - Expect(fakeClient.Update(ctx, obj)).To(Succeed()) - - // We now look at the behavior when we delete the Database object - Expect(fakeClient.Delete(ctx, obj)).To(Succeed()) - }) - tester.setExpectMissingObject() - tester.reconcile() - tester.assert(ctx, newSubscriptionTesterAdapter(subscription)) + // Mocking Detect subscription + expectedValue := sqlmock.NewRows([]string{""}).AddRow("0") + dbMock.ExpectQuery(subscriptionDetectionQuery).WithArgs(subscription.Spec.Name). + WillReturnRows(expectedValue) + + // Mocking Create subscription + expectedCreate := sqlmock.NewResult(0, 1) + expectedQuery := fmt.Sprintf( + "CREATE SUBSCRIPTION %s CONNECTION %s PUBLICATION %s", + pgx.Identifier{subscription.Spec.Name}.Sanitize(), + pq.QuoteLiteral(connString), + pgx.Identifier{subscription.Spec.PublicationName}.Sanitize(), + ) + dbMock.ExpectExec(expectedQuery).WillReturnResult(expectedCreate) + + err = reconcileSubscription(ctx, fakeClient, r, subscription) + Expect(err).ToNot(HaveOccurred()) + + // Plain successful reconciliation, finalizers have been created + Expect(subscription.GetFinalizers()).NotTo(BeEmpty()) + Expect(subscription.Status.Applied).Should(HaveValue(BeTrue())) + Expect(subscription.Status.Message).Should(BeEmpty()) + + // The next 2 lines are a hacky bit to make sure the next reconciler + // call doesn't skip on account of Generation == ObservedGeneration. + // See fake.Client known issues with `Generation` + // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake@v0.19.0#NewClientBuilder + subscription.SetGeneration(subscription.GetGeneration() + 1) + Expect(fakeClient.Update(ctx, subscription)).To(Succeed()) + + // We now look at the behavior when we delete the Database object + Expect(fakeClient.Delete(ctx, subscription)).To(Succeed()) + + err = reconcileSubscription(ctx, fakeClient, r, subscription) + Expect(err).To(HaveOccurred()) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) }) @@ -313,19 +289,16 @@ var _ = Describe("Managed subscription controller tests", func() { }, } - tester.reconcileFunc = r.Reconcile - // Updating the subscription object to reference the newly created Cluster subscription.Spec.ClusterRef.Name = "cluster-other" Expect(fakeClient.Update(ctx, subscription)).To(Succeed()) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Subscription) { - Expect(obj.Status.Applied).Should(HaveValue(BeFalse())) - Expect(obj.Status.Message).Should(ContainSubstring( - fmt.Sprintf("%q not found", subscription.Spec.ClusterRef.Name))) - }) + err = reconcileSubscription(ctx, fakeClient, r, subscription) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newSubscriptionTesterAdapter(subscription)) + Expect(subscription.Status.Applied).Should(HaveValue(BeFalse())) + Expect(subscription.Status.Message).Should(ContainSubstring( + fmt.Sprintf("%q not found", subscription.Spec.ClusterRef.Name))) }) It("skips reconciliation if subscription object isn't found (deleted subscription)", func(ctx SpecContext) { @@ -381,14 +354,13 @@ var _ = Describe("Managed subscription controller tests", func() { // Expect(fakeClient.Create(ctx, currentManager)).To(Succeed()) Expect(fakeClient.Create(ctx, subDuplicate)).To(Succeed()) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Subscription) { - expectedError := fmt.Sprintf("%q is already managed by object %q", - subDuplicate.Spec.Name, subscription.Name) - Expect(obj.Status.Applied).Should(HaveValue(BeFalse())) - Expect(obj.Status.Message).Should(ContainSubstring(expectedError)) - }) + err = reconcileSubscription(ctx, fakeClient, r, subDuplicate) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newSubscriptionTesterAdapter(subDuplicate)) + expectedError := fmt.Sprintf("%q is already managed by object %q", + subDuplicate.Spec.Name, subscription.Name) + Expect(subDuplicate.Status.Applied).Should(HaveValue(BeFalse())) + Expect(subDuplicate.Status.Message).Should(ContainSubstring(expectedError)) }) It("properly signals a subscription is on a replica cluster", func(ctx SpecContext) { @@ -398,11 +370,28 @@ var _ = Describe("Managed subscription controller tests", func() { } Expect(fakeClient.Patch(ctx, cluster, client.MergeFrom(initialCluster))).To(Succeed()) - tester.setUpdatedObjectExpectations(func(obj *apiv1.Subscription) { - Expect(obj.Status.Applied).Should(BeNil()) - Expect(obj.Status.Message).Should(ContainSubstring("waiting for the cluster to become primary")) - }) + err = reconcileSubscription(ctx, fakeClient, r, subscription) + Expect(err).ToNot(HaveOccurred()) - tester.assert(ctx, newSubscriptionTesterAdapter(subscription)) + Expect(subscription.Status.Applied).Should(BeNil()) + Expect(subscription.Status.Message).Should(ContainSubstring("waiting for the cluster to become primary")) }) }) + +func reconcileSubscription( + ctx context.Context, + fakeClient client.Client, + r *SubscriptionReconciler, + subscription *apiv1.Subscription, +) error { + GinkgoT().Helper() + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{ + Namespace: subscription.GetNamespace(), + Name: subscription.GetName(), + }}) + Expect(err).ToNot(HaveOccurred()) + return fakeClient.Get(ctx, client.ObjectKey{ + Namespace: subscription.GetNamespace(), + Name: subscription.GetName(), + }, subscription) +}