Add some feedback

Author: bjee19

internal/mode/static/provisioner/handler.go

@@ -104,11 +104,10 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger,
 				panic(fmt.Errorf("unknown resource type %T", e.Resource))
 			}
 		case *events.DeleteEvent:
-			obj := e
 			switch e.Type.(type) {
 			case *gatewayv1.Gateway:
 				if !h.provisioner.isLeader() {
-					h.provisioner.setResourceToDelete(obj.NamespacedName)
+					h.provisioner.setResourceToDelete(e.NamespacedName)
 				}
 
 				if err := h.provisioner.deprovisionNginx(ctx, e.NamespacedName); err != nil {

internal/mode/static/provisioner/objects.go

@@ -645,7 +645,7 @@ func (p *NginxProvisioner) buildNginxPodTemplateSpec(
 		spec.Spec.ImagePullSecrets = append(spec.Spec.ImagePullSecrets, ref)
 	}
 
-	// need to sort ports so everytime buildNginxPodTemplateSpec is called it will generate the exact same
+	// need to sort secret names so everytime buildNginxPodTemplateSpec is called it will generate the exact same
 	// array of secrets. This is needed to satisfy deterministic results of the method.
 	sort.Slice(spec.Spec.ImagePullSecrets, func(i, j int) bool {
 		return spec.Spec.ImagePullSecrets[i].Name < spec.Spec.ImagePullSecrets[j].Name

internal/mode/static/provisioner/objects_test.go

@@ -447,31 +447,31 @@ func TestBuildNginxResourceObjects_DockerSecrets(t *testing.T) {
 		Data: map[string][]byte{"data": []byte("docker")},
 	}
 
-	dockerSecretTeaName := dockerTestSecretName + "-Tea"
-	dockerSecretTea := &corev1.Secret{
+	dockerSecretRegistry1Name := dockerTestSecretName + "-registry1"
+	dockerSecretRegistry1 := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      dockerSecretTeaName,
+			Name:      dockerSecretRegistry1Name,
 			Namespace: ngfNamespace,
 		},
-		Data: map[string][]byte{"data": []byte("docker-Tea")},
+		Data: map[string][]byte{"data": []byte("docker-registry1")},
 	}
 
-	dockerSecretChaiName := dockerTestSecretName + "-Chai"
-	dockerSecretChai := &corev1.Secret{
+	dockerSecretRegistry2Name := dockerTestSecretName + "-registry2"
+	dockerSecretRegistry2 := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      dockerSecretChaiName,
+			Name:      dockerSecretRegistry2Name,
 			Namespace: ngfNamespace,
 		},
-		Data: map[string][]byte{"data": []byte("docker-Chai")},
+		Data: map[string][]byte{"data": []byte("docker-registry2")},
 	}
-	fakeClient := fake.NewFakeClient(dockerSecret, dockerSecretTea, dockerSecretChai)
+	fakeClient := fake.NewFakeClient(dockerSecret, dockerSecretRegistry1, dockerSecretRegistry2)
 
 	provisioner := &NginxProvisioner{
 		cfg: Config{
 			GatewayPodConfig: &config.GatewayPodConfig{
 				Namespace: ngfNamespace,
 			},
-			NginxDockerSecretNames: []string{dockerTestSecretName, dockerSecretTeaName, dockerSecretChaiName},
+			NginxDockerSecretNames: []string{dockerTestSecretName, dockerSecretRegistry1Name, dockerSecretRegistry2Name},
 		},
 		k8sClient: fakeClient,
 		baseLabelSelector: metav1.LabelSelector{
@@ -508,16 +508,16 @@ func TestBuildNginxResourceObjects_DockerSecrets(t *testing.T) {
 	g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerTestSecretName)))
 	g.Expect(secret.GetLabels()).To(Equal(expLabels))
 
-	chaiSecretObj := objects[1]
-	secret, ok = chaiSecretObj.(*corev1.Secret)
+	registry1SecretObj := objects[1]
+	secret, ok = registry1SecretObj.(*corev1.Secret)
 	g.Expect(ok).To(BeTrue())
-	g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerSecretChaiName)))
+	g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerSecretRegistry1Name)))
 	g.Expect(secret.GetLabels()).To(Equal(expLabels))
 
-	teaSecretObj := objects[2]
-	secret, ok = teaSecretObj.(*corev1.Secret)
+	registry2SecretObj := objects[2]
+	secret, ok = registry2SecretObj.(*corev1.Secret)
 	g.Expect(ok).To(BeTrue())
-	g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerSecretTeaName)))
+	g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerSecretRegistry2Name)))
 	g.Expect(secret.GetLabels()).To(Equal(expLabels))
 
 	depObj := objects[7]
@@ -530,10 +530,10 @@ func TestBuildNginxResourceObjects_DockerSecrets(t *testing.T) {
 			Name: controller.CreateNginxResourceName(resourceName, dockerTestSecretName),
 		},
 		{
-			Name: controller.CreateNginxResourceName(resourceName, dockerSecretChaiName),
+			Name: controller.CreateNginxResourceName(resourceName, dockerSecretRegistry1Name),
 		},
 		{
-			Name: controller.CreateNginxResourceName(resourceName, dockerSecretTeaName),
+			Name: controller.CreateNginxResourceName(resourceName, dockerSecretRegistry2Name),
 		},
 	}))
 }

tests/framework/resourcemanager.go

@@ -708,14 +708,8 @@ func GetReadyNGFPodNames(
 		return nil, errors.New("unable to find NGF Pod(s)")
 	}
 
-	var names []string
-	for _, pod := range podList.Items {
-		for _, cond := range pod.Status.Conditions {
-			if cond.Type == core.PodReady && cond.Status == core.ConditionTrue {
-				names = append(names, pod.Name)
-			}
-		}
-	}
+	names := getReadyPodNames(podList)
+
 	return names, nil
 }
 
@@ -742,6 +736,12 @@ func GetReadyNginxPodNames(
 		return nil, errors.New("unable to find NGINX Pod(s)")
 	}
 
+	names := getReadyPodNames(podList)
+
+	return names, nil
+}
+
+func getReadyPodNames(podList core.PodList) []string {
 	var names []string
 	for _, pod := range podList.Items {
 		for _, cond := range pod.Status.Conditions {
@@ -751,7 +751,7 @@ func GetReadyNginxPodNames(
 		}
 	}
 
-	return names, nil
+	return names
 }
 
 func countNumberOfReadyParents(parents []v1.RouteParentStatus) int {
@@ -768,33 +768,6 @@ func countNumberOfReadyParents(parents []v1.RouteParentStatus) int {
 	return readyCount
 }
 
-func (rm *ResourceManager) WaitForAppsToBeReadyWithPodCount(namespace string, podCount int) error {
-	ctx, cancel := context.WithTimeout(context.Background(), rm.TimeoutConfig.CreateTimeout)
-	defer cancel()
-
-	return rm.WaitForAppsToBeReadyWithCtxWithPodCount(ctx, namespace, podCount)
-}
-
-func (rm *ResourceManager) WaitForAppsToBeReadyWithCtxWithPodCount(
-	ctx context.Context,
-	namespace string,
-	podCount int,
-) error {
-	if err := rm.WaitForPodsToBeReadyWithCount(ctx, namespace, podCount); err != nil {
-		return err
-	}
-
-	if err := rm.waitForHTTPRoutesToBeReady(ctx, namespace); err != nil {
-		return err
-	}
-
-	if err := rm.waitForGRPCRoutesToBeReady(ctx, namespace); err != nil {
-		return err
-	}
-
-	return rm.waitForGatewaysToBeReady(ctx, namespace)
-}
-
 // WaitForPodsToBeReadyWithCount waits for all Pods in the specified namespace to be ready or
 // until the provided context is canceled.
 func (rm *ResourceManager) WaitForPodsToBeReadyWithCount(ctx context.Context, namespace string, count int) error {

tests/suite/system_suite_test.go

@@ -323,7 +323,7 @@ var _ = BeforeSuite(func() {
 		"telemetry",          // - running telemetry test (NGF will be deployed as part of the test)
 		"scale",              // - running scale test (this test will deploy its own version)
 		"reconfiguration",    // - running reconfiguration test (test will deploy its own instances)
-		"graceful-recovery",
+		"graceful-recovery",  // - running graceful-recovery test (test will deploy its own instances)
 	}
 	for _, s := range skipSubstrings {
 		if strings.Contains(labelFilter, s) {