From a6eb73af559f9ba7438c529420f6e55c83b045af Mon Sep 17 00:00:00 2001 From: Bartosz Majsak Date: Fri, 12 Jan 2024 17:36:12 +0100 Subject: [PATCH] chore: shifts FeatureTracker creation to Feature's Apply phase (#795) --- pkg/feature/builder.go | 6 --- pkg/feature/feature.go | 8 +++- .../features/serverless_feature_test.go | 43 ++++++++++--------- 3 files changed, 30 insertions(+), 27 deletions(-) diff --git a/pkg/feature/builder.go b/pkg/feature/builder.go index 2fa9160e77f..b4b9632b441 100644 --- a/pkg/feature/builder.go +++ b/pkg/feature/builder.go @@ -168,12 +168,6 @@ func (fb *featureBuilder) Load() (*Feature, error) { } } - if feature.Enabled { - if err := feature.createResourceTracker(); err != nil { - return feature, err - } - } - return feature, nil } diff --git a/pkg/feature/feature.go b/pkg/feature/feature.go index 397fda734a3..1605bd68947 100644 --- a/pkg/feature/feature.go +++ b/pkg/feature/feature.go @@ -49,6 +49,10 @@ func (f *Feature) Apply() error { return nil } + if err := f.createResourceTracker(); err != nil { + return err + } + // Verify all precondition and collect errors var multiErr *multierror.Error for _, precondition := range f.preconditions { @@ -229,7 +233,9 @@ func (f *Feature) createResourceTracker() error { // Register its own cleanup f.addCleanup(func(feature *Feature) error { - if err := f.DynamicClient.Resource(gvr.ResourceTracker).Delete(context.TODO(), f.Spec.Tracker.Name, metav1.DeleteOptions{}); err != nil && !k8serrors.IsNotFound(err) { + err := f.DynamicClient.Resource(gvr.ResourceTracker).Delete(context.TODO(), f.Spec.Tracker.Name, metav1.DeleteOptions{}) + + if !k8serrors.IsNotFound(err) { return err } diff --git a/tests/integration/features/serverless_feature_test.go b/tests/integration/features/serverless_feature_test.go index 26d7b4192f1..550ba6bc543 100644 --- a/tests/integration/features/serverless_feature_test.go +++ b/tests/integration/features/serverless_feature_test.go @@ -83,13 +83,16 @@ var _ = Describe("Serverless feature", func() { Load() Expect(err).ToNot(HaveOccurred()) + + // Creates the actual Feature instance so that associated FeatureTracker is created as well + Expect(testFeature.Apply()).To(Succeed()) }) Context("verifying preconditions", func() { When("operator is not installed", func() { It("operator presence check should return an error", func() { - Expect(serverless.EnsureServerlessOperatorInstalled(testFeature)).To(HaveOccurred()) + Expect(serverless.EnsureServerlessOperatorInstalled(testFeature)).ToNot(Succeed()) }) }) @@ -99,10 +102,10 @@ var _ = Describe("Serverless feature", func() { BeforeEach(func() { // Create KNativeServing the CRD knativeServingCrdObj = &apiextensionsv1.CustomResourceDefinition{} - Expect(yaml.Unmarshal([]byte(knativeServingCrd), knativeServingCrdObj)).ToNot(HaveOccurred()) + Expect(yaml.Unmarshal([]byte(knativeServingCrd), knativeServingCrdObj)).To(Succeed()) c, err := client.New(envTest.Config, client.Options{}) Expect(err).ToNot(HaveOccurred()) - Expect(c.Create(context.TODO(), knativeServingCrdObj)).ToNot(HaveOccurred()) + Expect(c.Create(context.TODO(), knativeServingCrdObj)).To(Succeed()) crdOptions := envtest.CRDInstallOptions{PollInterval: interval, MaxTime: timeout} err = envtest.WaitForCRDs(envTest.Config, []*apiextensionsv1.CustomResourceDefinition{knativeServingCrdObj}, crdOptions) @@ -115,25 +118,25 @@ var _ = Describe("Serverless feature", func() { }) It("operator presence check should succeed", func() { - Expect(serverless.EnsureServerlessOperatorInstalled(testFeature)).ToNot(HaveOccurred()) + Expect(serverless.EnsureServerlessOperatorInstalled(testFeature)).To(Succeed()) }) It("KNative serving absence check should succeed if serving is not installed", func() { - Expect(serverless.EnsureServerlessAbsent(testFeature)).ToNot(HaveOccurred()) + Expect(serverless.EnsureServerlessAbsent(testFeature)).To(Succeed()) }) It("KNative serving absence check should fail when serving is present", func() { ns := envtestutil.AppendRandomNameTo(testNamespacePrefix) nsResource := createNamespace(ns) - Expect(envTestClient.Create(context.TODO(), nsResource)).ToNot(HaveOccurred()) + Expect(envTestClient.Create(context.TODO(), nsResource)).To(Succeed()) defer objectCleaner.DeleteAll(nsResource) knativeServing := &unstructured.Unstructured{} - Expect(yaml.Unmarshal([]byte(knativeServingInstance), knativeServing)).ToNot(HaveOccurred()) + Expect(yaml.Unmarshal([]byte(knativeServingInstance), knativeServing)).To(Succeed()) knativeServing.SetNamespace(nsResource.Name) - Expect(envTestClient.Create(context.TODO(), knativeServing)).ToNot(HaveOccurred()) + Expect(envTestClient.Create(context.TODO(), knativeServing)).To(Succeed()) - Expect(serverless.EnsureServerlessAbsent(testFeature)).To(HaveOccurred()) + Expect(serverless.EnsureServerlessAbsent(testFeature)).ToNot(Succeed()) }) }) }) @@ -163,16 +166,16 @@ var _ = Describe("Serverless feature", func() { Expect(yaml.Unmarshal([]byte(openshiftClusterIngress), osIngressResource)).ToNot(HaveOccurred()) c, err := client.New(envTest.Config, client.Options{}) Expect(err).ToNot(HaveOccurred()) - Expect(c.Create(context.TODO(), osIngressResource)).ToNot(HaveOccurred()) + Expect(c.Create(context.TODO(), osIngressResource)).To(Succeed()) // Default value is blank -> testFeature.Spec.Serving.IngressGateway.Domain = "" - Expect(serverless.ServingIngressDomain(testFeature)).ToNot(HaveOccurred()) + Expect(serverless.ServingIngressDomain(testFeature)).To(Succeed()) Expect(testFeature.Spec.KnativeIngressDomain).To(Equal("*.foo.io")) }) It("should use user value when set in the DSCI", func() { testFeature.Spec.Serving.IngressGateway.Domain = testDomainFooCom - Expect(serverless.ServingIngressDomain(testFeature)).ToNot(HaveOccurred()) + Expect(serverless.ServingIngressDomain(testFeature)).To(Succeed()) Expect(testFeature.Spec.KnativeIngressDomain).To(Equal(testDomainFooCom)) }) }) @@ -183,16 +186,16 @@ var _ = Describe("Serverless feature", func() { It("should create a TLS secret if certificate is SelfSigned", func() { ns := envtestutil.AppendRandomNameTo(testNamespacePrefix) nsResource := createNamespace(ns) - Expect(envTestClient.Create(context.TODO(), nsResource)).ToNot(HaveOccurred()) + Expect(envTestClient.Create(context.TODO(), nsResource)).To(Succeed()) defer objectCleaner.DeleteAll(nsResource) testFeature.Spec.ControlPlane.Namespace = nsResource.Name testFeature.Spec.Serving.IngressGateway.Certificate.Type = infrav1.SelfSigned testFeature.Spec.Serving.IngressGateway.Domain = testDomainFooCom - Expect(serverless.ServingDefaultValues(testFeature)).ToNot(HaveOccurred()) - Expect(serverless.ServingIngressDomain(testFeature)).ToNot(HaveOccurred()) + Expect(serverless.ServingDefaultValues(testFeature)).To(Succeed()) + Expect(serverless.ServingIngressDomain(testFeature)).To(Succeed()) - Expect(serverless.ServingCertificateResource(testFeature)).ToNot(HaveOccurred()) + Expect(serverless.ServingCertificateResource(testFeature)).To(Succeed()) secret, err := testFeature.Clientset.CoreV1().Secrets(nsResource.Name).Get(context.TODO(), serverless.DefaultCertificateSecretName, v1.GetOptions{}) Expect(err).ToNot(HaveOccurred()) @@ -202,16 +205,16 @@ var _ = Describe("Serverless feature", func() { It("should not create any TLS secret if certificate is user provided", func() { ns := envtestutil.AppendRandomNameTo(testNamespacePrefix) nsResource := createNamespace(ns) - Expect(envTestClient.Create(context.TODO(), nsResource)).ToNot(HaveOccurred()) + Expect(envTestClient.Create(context.TODO(), nsResource)).To(Succeed()) defer objectCleaner.DeleteAll(nsResource) testFeature.Spec.ControlPlane.Namespace = nsResource.Name testFeature.Spec.Serving.IngressGateway.Certificate.Type = infrav1.Provided testFeature.Spec.Serving.IngressGateway.Domain = "*.foo.com" - Expect(serverless.ServingDefaultValues(testFeature)).ToNot(HaveOccurred()) - Expect(serverless.ServingIngressDomain(testFeature)).ToNot(HaveOccurred()) + Expect(serverless.ServingDefaultValues(testFeature)).To(Succeed()) + Expect(serverless.ServingIngressDomain(testFeature)).To(Succeed()) - Expect(serverless.ServingCertificateResource(testFeature)).ToNot(HaveOccurred()) + Expect(serverless.ServingCertificateResource(testFeature)).To(Succeed()) list, err := testFeature.Clientset.CoreV1().Secrets(nsResource.Name).List(context.TODO(), v1.ListOptions{}) Expect(err).ToNot(HaveOccurred())