Minor refactor of skipNotification unit/E2E tests

Signed-off-by: Jonathan West <[email protected]>
argoproj-labs · Aug 6, 2024 · 07d3485 · 07d3485
1 parent 31c6f76
commit 07d3485
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 89 deletions.
diff --git a/controllers/resources.go b/controllers/resources.go
@@ -697,7 +697,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsSecrets(ctx context.Context,
 
 	}
 
-	// If SkipNotificationSecretDeployment is true, and the secret exists, delete it
+	// If SkipNotificationSecretDeployment is true, and the secret exists (and is owned by us), delete it
 	if cr.Spec.SkipNotificationSecretDeployment {
 
 		// If the controller created/owns the Secret, delete it

diff --git a/controllers/resources_test.go b/controllers/resources_test.go
@@ -1008,10 +1008,13 @@ var _ = Describe("Resource creation and cleanup tests", func() {
 			Expect(err).ToNot(HaveOccurred())
 		})
 
-		It("Verify that RolloutManager with SkipNotificationSecretDeployment doesn't create a secret", func() {
+		DescribeTable("Verify that for RolloutManager with SkipNotificationSecretDeployment, it either creates or doesn't create a Secret, based on that value", func(skipNotificationInitialValue bool) {
 
-			By("Creating RolloutManager with SkipNotificationSecretDeployment set to true")
-			a.Spec.SkipNotificationSecretDeployment = true
+			By(fmt.Sprintf("Creating RolloutManager with SkipNotificationSecretDeployment set to initial value '%v'", skipNotificationInitialValue))
+			a.Spec.SkipNotificationSecretDeployment = skipNotificationInitialValue
+			Expect(r.Client.Update(ctx, &a)).To(Succeed())
+
+			By("calling reconcileRolloutsSecrets")
 			Expect(r.reconcileRolloutsSecrets(ctx, a)).To(Succeed())
 
 			secret := &corev1.Secret{
@@ -1021,38 +1024,36 @@ var _ = Describe("Resource creation and cleanup tests", func() {
 				},
 				Type: corev1.SecretTypeOpaque,
 			}
-			Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).ToNot(Succeed(), "secret should not exist after reconcile call")
 
-			By("Updating the RolloutManager with SkipNotificationSecretDeployment set to false")
-			a.Spec.SkipNotificationSecretDeployment = false
-			Expect(r.reconcileRolloutsSecrets(ctx, a)).To(Succeed())
-			Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).To(Succeed(), "secret should exist after reconcile call")
-		})
+			if a.Spec.SkipNotificationSecretDeployment {
+				Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).ToNot(Succeed(), "secret should not exist after reconcile call")
+			} else {
+				Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).To(Succeed(), "secret should exist after reconcile call")
+			}
+
+			By(fmt.Sprintf("Updating the RolloutManager with SkipNotificationSecretDeployment set to '%v'", !skipNotificationInitialValue))
+			a.Spec.SkipNotificationSecretDeployment = !skipNotificationInitialValue
+			Expect(r.Client.Update(ctx, &a)).To(Succeed())
 
-		It("Verify that RolloutManager can handle the case where SkipNotificationSecretDeployment is set on an existing instance", func() {
-			By("Creating RolloutManager with SkipNotificationSecretDeployment set to false")
-			a.Spec.SkipNotificationSecretDeployment = false
+			By("calling reconcileRolloutsSecrets")
 			Expect(r.reconcileRolloutsSecrets(ctx, a)).To(Succeed())
 
-			secret := &corev1.Secret{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      DefaultRolloutsNotificationSecretName,
-					Namespace: a.Namespace,
-				},
-				Type: corev1.SecretTypeOpaque,
+			if a.Spec.SkipNotificationSecretDeployment {
+				Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).ToNot(Succeed(), "secret should not exist after reconcile call")
+			} else {
+				Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).To(Succeed(), "secret should exist after reconcile call")
 			}
-			Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).To(Succeed(), "secret should exist after reconcile call")
 
-			By("Updating the RolloutManager with SkipNotificationSecretDeployment set to true")
-			a.Spec.SkipNotificationSecretDeployment = true
-			Expect(r.reconcileRolloutsSecrets(ctx, a)).To(Succeed())
-			Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).ToNot(Succeed(), "secret should not exist after reconcile call")
-		})
+		},
+			Entry("SkipNotificationSecretDeployment is initially true", true),
+			Entry("SkipNotificationSecretDeployment is initially false", false),
+		)
 
 		It("Verify that RolloutManager does not update an existing notification secret if it doesn't have the ownership", func() {
 			By("Creating RolloutManager with SkipNotificationSecretDeployment set to true")
 
 			a.Spec.SkipNotificationSecretDeployment = true
+			Expect(r.Client.Update(ctx, &a)).To(Succeed())
 			Expect(r.reconcileRolloutsSecrets(ctx, a)).To(Succeed())
 
 			secret := &corev1.Secret{
@@ -1062,16 +1063,16 @@ var _ = Describe("Resource creation and cleanup tests", func() {
 				},
 				Type: corev1.SecretTypeOpaque,
 			}
-			Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).ToNot(Succeed(), "secret should exist after reconcile call")
+			Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).ToNot(Succeed(), "secret should not exist after reconcile call")
 
 			By("Create the secret without the owner reference")
 			Expect(r.Client.Create(ctx, secret)).To(Succeed())
 
-			By("Updating the RolloutManager")
+			By("Call reconcileRolloutsSecrets")
 			Expect(r.reconcileRolloutsSecrets(ctx, a)).To(Succeed())
 
 			By("Verifying that the secret was not updated")
-			Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).To(Succeed(), "secret should exist after reconcile call")
+			Expect(fetchObject(ctx, r.Client, a.Namespace, secret.Name, secret)).To(Succeed(), "secret should exist after reconcile call, even though SkipNotificationSecretDeployment is still true")
 			Expect(secret.OwnerReferences).To(BeNil())
 
 			By("Adding another owner reference")

diff --git a/tests/e2e/rollout_tests_all.go b/tests/e2e/rollout_tests_all.go
@@ -2,6 +2,7 @@ package e2e
 
 import (
 	"context"
+	"fmt"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -506,77 +507,53 @@ func RunRolloutsTests(namespaceScopedParam bool) {
 			})
 		})
 
-		When("A RolloutManager is created with the SkipNotificationSecretDeployment option to true", func() {
-			It("should create the controller without the notification secret", func() {
+		DescribeTable("RolloutManager is initially created with a given SkipNotificationSecretDeployment (true/false), then it swaps", func(initialSkipNotificationValue bool) {
 
-				rolloutsManager := rolloutsmanagerv1alpha1.RolloutManager{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "basic-rollouts-manager-with-skip-notification-secret",
-						Namespace: fixture.TestE2ENamespace,
-					},
-					Spec: rolloutsmanagerv1alpha1.RolloutManagerSpec{
-						NamespaceScoped:                  namespaceScopedParam,
-						SkipNotificationSecretDeployment: true,
-					},
-				}
-
-				Expect(k8sClient.Create(ctx, &rolloutsManager)).To(Succeed())
-
-				Eventually(rolloutsManager, "1m", "1s").Should(rolloutManagerFixture.HavePhase(rolloutsmanagerv1alpha1.PhaseAvailable))
-
-				secret := corev1.Secret{
-					ObjectMeta: metav1.ObjectMeta{Name: controllers.DefaultRolloutsNotificationSecretName, Namespace: rolloutManager.Namespace},
-				}
-				Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(&secret), &secret)).ToNot(Succeed())
-
-				By("Setting the SkipNotificationSecretDeployment to false")
-				err := k8s.UpdateWithoutConflict(ctx, &rolloutsManager, k8sClient, func(obj client.Object) {
-					goObj, ok := obj.(*rolloutsmanagerv1alpha1.RolloutManager)
-					Expect(ok).To(BeTrue())
-					goObj.Spec.SkipNotificationSecretDeployment = false
-				})
-
-				Expect(err).ToNot(HaveOccurred())
-				Eventually(&secret, "10s", "1s").Should(k8s.ExistByName(k8sClient))
-
-			})
-		})
+			By(fmt.Sprintf("creating RolloutManager with SkipNotificationSecretDeployment set to '%v'", initialSkipNotificationValue))
 
-		When("A RolloutManager is created with the SkipNotificationSecretDeployment option to false", func() {
-			It("should create the controller with the notification secret", func() {
-
-				rolloutsManager := rolloutsmanagerv1alpha1.RolloutManager{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "basic-rollouts-manager-without-skip-notification-secret",
-						Namespace: fixture.TestE2ENamespace,
-					},
-					Spec: rolloutsmanagerv1alpha1.RolloutManagerSpec{
-						NamespaceScoped:                  namespaceScopedParam,
-						SkipNotificationSecretDeployment: false,
-					},
-				}
+			rolloutsManager := rolloutsmanagerv1alpha1.RolloutManager{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "basic-rollouts-manager-with-skip-notification-secret",
+					Namespace: fixture.TestE2ENamespace,
+				},
+				Spec: rolloutsmanagerv1alpha1.RolloutManagerSpec{
+					NamespaceScoped:                  namespaceScopedParam,
+					SkipNotificationSecretDeployment: initialSkipNotificationValue,
+				},
+			}
 
-				Expect(k8sClient.Create(ctx, &rolloutsManager)).To(Succeed())
+			Expect(k8sClient.Create(ctx, &rolloutsManager)).To(Succeed())
 
-				Eventually(rolloutsManager, "1m", "1s").Should(rolloutManagerFixture.HavePhase(rolloutsmanagerv1alpha1.PhaseAvailable))
+			Eventually(rolloutsManager, "1m", "1s").Should(rolloutManagerFixture.HavePhase(rolloutsmanagerv1alpha1.PhaseAvailable))
 
-				secret := corev1.Secret{
-					ObjectMeta: metav1.ObjectMeta{Name: controllers.DefaultRolloutsNotificationSecretName, Namespace: rolloutManager.Namespace},
-				}
+			secret := corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: controllers.DefaultRolloutsNotificationSecretName, Namespace: rolloutManager.Namespace},
+			}
+			if rolloutsManager.Spec.SkipNotificationSecretDeployment {
+				Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(&secret), &secret)).ToNot(Succeed())
+			} else {
 				Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(&secret), &secret)).To(Succeed())
+			}
 
-				By("Setting the SkipNotificationSecretDeployment to true")
-				err := k8s.UpdateWithoutConflict(ctx, &rolloutsManager, k8sClient, func(obj client.Object) {
-					goObj, ok := obj.(*rolloutsmanagerv1alpha1.RolloutManager)
-					Expect(ok).To(BeTrue())
-					goObj.Spec.SkipNotificationSecretDeployment = true
-				})
+			By(fmt.Sprintf("setting the SkipNotificationSecretDeployment to '%v'", !initialSkipNotificationValue))
+			err := k8s.UpdateWithoutConflict(ctx, &rolloutsManager, k8sClient, func(obj client.Object) {
+				rmObj, ok := obj.(*rolloutsmanagerv1alpha1.RolloutManager)
+				Expect(ok).To(BeTrue())
+				rmObj.Spec.SkipNotificationSecretDeployment = !initialSkipNotificationValue
+			})
+			Expect(err).ToNot(HaveOccurred())
 
-				Expect(err).ToNot(HaveOccurred())
+			if rolloutsManager.Spec.SkipNotificationSecretDeployment {
 				Eventually(&secret, "10s", "1s").ShouldNot(k8s.ExistByName(k8sClient))
 
-			})
-		})
+			} else {
+				Eventually(&secret, "10s", "1s").Should(k8s.ExistByName(k8sClient))
+			}
+
+		},
+			Entry("skipNotification is initially true, then set to false", true),
+			Entry("skipNotification is initially false, then set to true", false),
+		)
 
 		When("A RolloutManager is deleted but the notification secret is owned by another controller", func() {
 			It("should not delete the secret", func() {