code review changes

internal/controller/managedzone_controller.go

@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -37,18 +38,11 @@ import (
 
 const (
 	ManagedZoneFinalizer = "kuadrant.io/managed-zone"
-	ZoneErrInternal      = "Error"
-	ZoneErrProvider      = "ProviderError"
 )
 
-type ZoneErr struct {
-	code    string
-	message string
-}
-
-func (ze ZoneErr) Error() string {
-	return ze.message
-}
+var (
+	ErrProvider = errors.New("ProviderError")
+)
 
 // ManagedZoneReconciler reconciles a ManagedZone object
 type ManagedZoneReconciler struct {
@@ -113,9 +107,9 @@ func (r *ManagedZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	if err != nil {
 		reason := "UnknownError"
 		message := fmt.Sprintf("unexpected error %v ", provider.SanitizeError(err))
-		if zerr, ok := err.(ZoneErr); ok {
-			reason = zerr.code
-			message = zerr.message
+		if errors.Is(err, ErrProvider) {
+			reason = ErrProvider.Error()
+			message = err.Error()
 		}
 		setManagedZoneCondition(managedZone, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, reason, message)
 		statusUpdateErr := r.Status().Update(ctx, managedZone)
@@ -173,20 +167,14 @@ func (r *ManagedZoneReconciler) publishManagedZone(ctx context.Context, managedZ
 
 	dnsProvider, err := r.ProviderFactory.ProviderFor(ctx, managedZone, provider.Config{})
 	if err != nil {
-		return ZoneErr{
-			code:    ZoneErrInternal,
-			message: fmt.Sprintf("failed to get provider for the zone: %v", provider.SanitizeError(err)),
-		}
+		return fmt.Errorf("failed to get provider for the zone: %v", provider.SanitizeError(err))
 	}
 	mzResp, err := dnsProvider.EnsureManagedZone(managedZone)
 	if err != nil {
 		managedZone.Status.ID = ""
 		managedZone.Status.RecordCount = 0
 		managedZone.Status.NameServers = nil
-		return ZoneErr{
-			code:    ZoneErrProvider,
-			message: fmt.Sprintf("The DNS provider failed to ensure the managed zone: %v", provider.SanitizeError(err)),
-		}
+		return fmt.Errorf("%w, The DNS provider failed to ensure the managed zone: %v", ErrProvider, provider.SanitizeError(err))
 	}
 
 	managedZone.Status.ID = mzResp.ID
@@ -204,21 +192,15 @@ func (r *ManagedZoneReconciler) deleteManagedZone(ctx context.Context, managedZo
 
 	dnsProvider, err := r.ProviderFactory.ProviderFor(ctx, managedZone, provider.Config{})
 	if err != nil {
-		return ZoneErr{
-			code:    ZoneErrInternal,
-			message: fmt.Sprintf("failed to get DNS provider instance : %v", err),
-		}
+		return fmt.Errorf("failed to get DNS provider instance : %v", err)
 	}
 	err = dnsProvider.DeleteManagedZone(managedZone)
 	if err != nil {
 		if strings.Contains(err.Error(), "was not found") || strings.Contains(err.Error(), "notFound") {
 			log.Log.Info("ManagedZone was not found, continuing", "managedZone", managedZone.Name)
 			return nil
 		}
-		return ZoneErr{
-			code:    ZoneErrProvider,
-			message: fmt.Sprintf("Failed to delete from provider. Provider Error: %v", provider.SanitizeError(err)),
-		}
+		return fmt.Errorf("%w, Failed to delete from provider. Provider Error: %v", ErrProvider, provider.SanitizeError(err))
 	}
 	log.Log.Info("Deleted ManagedZone", "managedZone", managedZone.Name)
 

internal/controller/managedzone_controller_test.go

@@ -21,13 +21,14 @@ package controller
 import (
 	"context"
 	"fmt"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	. "github.com/onsi/gomega/gstruct"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -130,35 +131,38 @@ var _ = Describe("ManagedZoneReconciler", func() {
 			}
 			err := k8sClient.Create(ctx, mz)
 			Expect(err).NotTo(HaveOccurred())
-			Eventually(func() error {
+			Eventually(func(g Gomega) error {
 				err = k8sClient.Get(ctx, client.ObjectKey{Namespace: mz.Namespace, Name: mz.Name}, mz)
-				Expect(err).NotTo(HaveOccurred())
-				if meta.IsStatusConditionTrue(mz.Status.Conditions, string(v1alpha1.ConditionTypeReady)) {
-					return fmt.Errorf("expected the ready condition to be false")
-				}
-				if mz.Status.NameServers != nil {
-					return fmt.Errorf("expected nameservers not to be set on provider error")
-				}
-				if mz.Status.RecordCount != 0 {
-					return fmt.Errorf("expected record count to be 0")
-				}
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(mz.Status.Conditions).To(
+					ContainElement(MatchFields(IgnoreExtras, Fields{
+						"Type":               Equal(string(v1alpha1.ConditionTypeReady)),
+						"Status":             Equal(metav1.ConditionFalse),
+						"Reason":             Equal(ErrProvider.Error()),
+						"ObservedGeneration": Equal(mz.Generation),
+					})),
+				)
+				g.Expect(mz.Finalizers).To(ContainElement(ManagedZoneFinalizer))
 				return nil
-			})
+			}, TestTimeoutMedium, time.Second).Should(Succeed())
 
 			p, err = inmemory.NewProviderFromSecret(ctx, &v1.Secret{}, provider.Config{})
 			Expect(err).ToNot(HaveOccurred())
 			SetDNSProvider(p)
-			Eventually(func() error {
+			Eventually(func(g Gomega) error {
 				err = k8sClient.Get(ctx, client.ObjectKey{Namespace: mz.Namespace, Name: mz.Name}, mz)
 				Expect(err).NotTo(HaveOccurred())
-				if meta.IsStatusConditionFalse(mz.Status.Conditions, string(v1alpha1.ConditionTypeReady)) {
-					return fmt.Errorf("expected the ready condition to be true")
-				}
-				if mz.Status.NameServers == nil {
-					return fmt.Errorf("expected nameservers to be set on provider error")
-				}
+				g.Expect(mz.Status.Conditions).To(
+					ContainElement(MatchFields(IgnoreExtras, Fields{
+						"Type":               Equal(string(v1alpha1.ConditionTypeReady)),
+						"Status":             Equal(metav1.ConditionTrue),
+						"Reason":             Equal("ProviderSuccess"),
+						"ObservedGeneration": Equal(mz.Generation),
+					})),
+				)
+				g.Expect(mz.Finalizers).To(ContainElement(ManagedZoneFinalizer))
 				return nil
-			})
+			}, TestTimeoutMedium, time.Second).Should(Succeed())
 		})
 	})
 })