fix: address Lytol review nits

- Add BatchDelete helper to internal; use in Chronicle, Connect,
  Package Manager, and Workbench cleanup/suspend functions
- Rename isProductEnabled to checkBool(ptr *bool, defaultVal bool)
  and apply consistently for both enabled and teardown patterns
  across site_controller.go and site_controller_networkpolicies.go

Author: ian-flores

docs/guides/upgrading.md

@@ -174,11 +174,15 @@ If you are upgrading an existing installation that has already run the operator
 1. Identify the components with existing DB password secrets:
 
    ```bash
-   kubectl get secrets -n posit-team -o name | grep -v db-password
+   for comp in workbench connect packagemanager; do
+     kubectl get secret "${comp}" -n posit-team --ignore-not-found -o name
+   done
    ```
 
 2. For each component (workbench, connect, packagemanager), rename the secret:
 
+   > **Warning:** If `${NEW_NAME}` already exists in the cluster, do not apply this migration — the operator has already generated a new password and you must re-synchronize the database password manually.
+
    ```bash
    # Get the old secret data
    OLD_NAME=<component-name>
@@ -187,7 +191,7 @@ If you are upgrading an existing installation that has already run the operator
 
    # Create new secret with old data
    kubectl get secret "${OLD_NAME}" -n "${NAMESPACE}" -o json \
-     | python3 -c "import json,sys; d=json.load(sys.stdin); d['metadata']['name']='${NEW_NAME}'; [d['metadata'].pop(k,None) for k in ['resourceVersion','uid','creationTimestamp','ownerReferences']]; print(json.dumps(d))" \
+     | python3 -c "import json,sys; d=json.load(sys.stdin); d['metadata']['name']='${NEW_NAME}'; [d['metadata'].pop(k,None) for k in ['resourceVersion','uid','creationTimestamp','managedFields','ownerReferences']]; print(json.dumps(d))" \
      | kubectl apply -f -
 
    # Delete old secret

internal/controller/core/chronicle_controller.go

@@ -340,26 +340,15 @@ func (r *ChronicleReconciler) CleanupChronicle(ctx context.Context, req ctrl.Req
 
 	key := client.ObjectKey{Name: c.ComponentName(), Namespace: req.Namespace}
 
-	// SERVICE
-	if err := internal.BasicDelete(ctx, r, l, key, &corev1.Service{}); err != nil {
-		return ctrl.Result{}, err
-	}
-
-	// STATEFULSET
-	if err := internal.BasicDelete(ctx, r, l, key, &v1.StatefulSet{}); err != nil {
-		return ctrl.Result{}, err
-	}
 	// NOTE: Chronicle's StatefulSet does not use VolumeClaimTemplates (it uses EmptyDir or S3).
 	// If VolumeClaimTemplates are added in the future, their PVCs must be deleted explicitly
 	// here because Kubernetes does not garbage-collect StatefulSet PVCs automatically.
-
-	// CONFIGMAP
-	if err := internal.BasicDelete(ctx, r, l, key, &corev1.ConfigMap{}); err != nil {
-		return ctrl.Result{}, err
-	}
-
-	// SERVICE ACCOUNTS
-	if err := internal.BasicDelete(ctx, r, l, key, &corev1.ServiceAccount{}); err != nil {
+	if err := internal.BatchDelete(ctx, r, l, key,
+		&corev1.Service{},
+		&v1.StatefulSet{},
+		&corev1.ConfigMap{},
+		&corev1.ServiceAccount{},
+	); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -383,13 +372,10 @@ func (r *ChronicleReconciler) suspendDeployedService(ctx context.Context, req ct
 
 	key := client.ObjectKey{Name: c.ComponentName(), Namespace: req.Namespace}
 
-	// SERVICE
-	if err := internal.BasicDelete(ctx, r, l, key, &corev1.Service{}); err != nil {
-		return ctrl.Result{}, err
-	}
-
-	// STATEFULSET (Chronicle uses StatefulSet, not Deployment)
-	if err := internal.BasicDelete(ctx, r, l, key, &v1.StatefulSet{}); err != nil {
+	if err := internal.BatchDelete(ctx, r, l, key,
+		&corev1.Service{},
+		&v1.StatefulSet{}, // Chronicle uses StatefulSet, not Deployment
+	); err != nil {
 		return ctrl.Result{}, err
 	}
 

internal/controller/core/connect.go

@@ -886,13 +886,11 @@ func (r *ConnectReconciler) suspendDeployedService(ctx context.Context, req ctrl
 
 	key := client.ObjectKey{Name: c.ComponentName(), Namespace: req.Namespace}
 
-	if err := internal.BasicDelete(ctx, r, l, key, &networkingv1.Ingress{}); err != nil {
-		return ctrl.Result{}, err
-	}
-	if err := internal.BasicDelete(ctx, r, l, key, &corev1.Service{}); err != nil {
-		return ctrl.Result{}, err
-	}
-	if err := internal.BasicDelete(ctx, r, l, key, &v1.Deployment{}); err != nil {
+	if err := internal.BatchDelete(ctx, r, l, key,
+		&networkingv1.Ingress{},
+		&corev1.Service{},
+		&v1.Deployment{},
+	); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -912,48 +910,17 @@ func (r *ConnectReconciler) cleanupDeployedService(ctx context.Context, req ctrl
 		Namespace: req.Namespace,
 	}
 
-	// INGRESS
-
-	existingIngress := &networkingv1.Ingress{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingIngress); err != nil {
-		return err
-	}
-
-	// SERVICE
-
-	existingService := &corev1.Service{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingService); err != nil {
-		return err
-	}
-
-	// DEPLOYMENT
-
-	existingDeployment := &v1.Deployment{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingDeployment); err != nil {
-		return err
-	}
-
-	// VOLUME
 	// NOTE: we delete the volume universally, even if create was false...
 	//   this ensures that we have the resource completely removed, whether it
 	//   was created, forgotten, or never created.
-
-	existingPvc := &corev1.PersistentVolumeClaim{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingPvc); err != nil {
-		return err
-	}
-
-	// SERVICE ACCOUNT
-
-	existingServiceAccount := &corev1.ServiceAccount{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingServiceAccount); err != nil {
-		return err
-	}
-
-	// CONFIGMAP
-
-	existingConfigmap := &corev1.ConfigMap{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingConfigmap); err != nil {
+	if err := internal.BatchDelete(ctx, r, l, key,
+		&networkingv1.Ingress{},
+		&corev1.Service{},
+		&v1.Deployment{},
+		&corev1.PersistentVolumeClaim{},
+		&corev1.ServiceAccount{},
+		&corev1.ConfigMap{},
+	); err != nil {
 		return err
 	}
 

internal/controller/core/package_manager.go

@@ -52,18 +52,11 @@ func (r *PackageManagerReconciler) suspendDeployedService(ctx context.Context, r
 
 	key := client.ObjectKey{Name: pm.ComponentName(), Namespace: req.Namespace}
 
-	// INGRESS
-	if err := internal.BasicDelete(ctx, r, l, key, &networkingv1.Ingress{}); err != nil {
-		return ctrl.Result{}, err
-	}
-
-	// SERVICE
-	if err := internal.BasicDelete(ctx, r, l, key, &corev1.Service{}); err != nil {
-		return ctrl.Result{}, err
-	}
-
-	// DEPLOYMENT
-	if err := internal.BasicDelete(ctx, r, l, key, &v1.Deployment{}); err != nil {
+	if err := internal.BatchDelete(ctx, r, l, key,
+		&networkingv1.Ingress{},
+		&corev1.Service{},
+		&v1.Deployment{},
+	); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -83,48 +76,17 @@ func (r *PackageManagerReconciler) cleanupDeployedService(ctx context.Context, r
 		Namespace: req.Namespace,
 	}
 
-	// INGRESS
-
-	existingIngress := &networkingv1.Ingress{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingIngress); err != nil {
-		return err
-	}
-
-	// SERVICE
-
-	existingService := &corev1.Service{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingService); err != nil {
-		return err
-	}
-
-	// DEPLOYMENT
-
-	existingDeployment := &v1.Deployment{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingDeployment); err != nil {
-		return err
-	}
-
-	// VOLUME
 	// NOTE: we delete the volume universally, even if create was false...
 	//   this ensures that we have the resource completely removed, whether it
 	//   was created, forgotten, or never created.
-
-	existingPvc := &corev1.PersistentVolumeClaim{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingPvc); err != nil {
-		return err
-	}
-
-	// SERVICE ACCOUNT
-
-	existingServiceAccount := &corev1.ServiceAccount{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingServiceAccount); err != nil {
-		return err
-	}
-
-	// CONFIGMAP
-
-	existingConfigmap := &corev1.ConfigMap{}
-	if err := internal.BasicDelete(ctx, r, l, key, existingConfigmap); err != nil {
+	if err := internal.BatchDelete(ctx, r, l, key,
+		&networkingv1.Ingress{},
+		&corev1.Service{},
+		&v1.Deployment{},
+		&corev1.PersistentVolumeClaim{},
+		&corev1.ServiceAccount{},
+		&corev1.ConfigMap{},
+	); err != nil {
 		return err
 	}
 

internal/controller/core/site_controller.go

@@ -24,9 +24,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-// isProductEnabled returns true if the product is enabled (nil defaults to enabled).
-func isProductEnabled(b *bool) bool {
-	return b == nil || *b
+// checkBool dereferences a bool pointer, returning defaultVal if nil.
+func checkBool(b *bool, defaultVal bool) bool {
+	if b == nil {
+		return defaultVal
+	}
+	return *b
 }
 
 // SiteReconciler reconciles a Site object
@@ -161,26 +164,26 @@ func (r *SiteReconciler) reconcileResources(ctx context.Context, req ctrl.Reques
 	// VOLUMES
 
 	// Determine if Connect is enabled (used for volume provisioning and later for reconciliation)
-	connectEnabled := site.Spec.Connect.Enabled == nil || *site.Spec.Connect.Enabled
-	connectTeardown := site.Spec.Connect.Teardown != nil && *site.Spec.Connect.Teardown
+	connectEnabled := checkBool(site.Spec.Connect.Enabled, true)
+	connectTeardown := checkBool(site.Spec.Connect.Teardown, false)
 	if connectTeardown && connectEnabled {
 		l.Info("connect.teardown is set but connect.enabled is not false; teardown has no effect until enabled=false")
 	}
 
-	workbenchEnabled := isProductEnabled(site.Spec.Workbench.Enabled)
-	workbenchTeardown := site.Spec.Workbench.Teardown != nil && *site.Spec.Workbench.Teardown
+	workbenchEnabled := checkBool(site.Spec.Workbench.Enabled, true)
+	workbenchTeardown := checkBool(site.Spec.Workbench.Teardown, false)
 	if workbenchTeardown && workbenchEnabled {
 		l.Info("workbench.teardown is set but workbench.enabled is not false; teardown has no effect until enabled=false")
 	}
 
-	pmEnabled := isProductEnabled(site.Spec.PackageManager.Enabled)
-	pmTeardown := site.Spec.PackageManager.Teardown != nil && *site.Spec.PackageManager.Teardown
+	pmEnabled := checkBool(site.Spec.PackageManager.Enabled, true)
+	pmTeardown := checkBool(site.Spec.PackageManager.Teardown, false)
 	if pmTeardown && pmEnabled {
 		l.Info("packageManager.teardown is set but packageManager.enabled is not false; teardown has no effect until enabled=false")
 	}
 
-	chronicleEnabled := isProductEnabled(site.Spec.Chronicle.Enabled)
-	chronicleTeardown := site.Spec.Chronicle.Teardown != nil && *site.Spec.Chronicle.Teardown
+	chronicleEnabled := checkBool(site.Spec.Chronicle.Enabled, true)
+	chronicleTeardown := checkBool(site.Spec.Chronicle.Teardown, false)
 	if chronicleTeardown && chronicleEnabled {
 		l.Info("chronicle.teardown is set but chronicle.enabled is not false; teardown has no effect until enabled=false")
 	}

internal/controller/core/site_controller_networkpolicies.go

@@ -39,7 +39,7 @@ func (r *SiteReconciler) reconcileNetworkPolicies(ctx context.Context, req ctrl.
 	}
 
 	// Chronicle network policy
-	chronicleEnabled := isProductEnabled(site.Spec.Chronicle.Enabled)
+	chronicleEnabled := checkBool(site.Spec.Chronicle.Enabled, true)
 	if chronicleEnabled {
 		if err := r.reconcileChronicleNetworkPolicy(ctx, req.Namespace, l, site); err != nil {
 			l.Error(err, "error ensuring chronicle network policy")
@@ -53,7 +53,7 @@ func (r *SiteReconciler) reconcileNetworkPolicies(ctx context.Context, req ctrl.
 	}
 
 	// Connect network policies
-	connectEnabled := site.Spec.Connect.Enabled == nil || *site.Spec.Connect.Enabled
+	connectEnabled := checkBool(site.Spec.Connect.Enabled, true)
 	if connectEnabled {
 		if err := r.reconcileConnectNetworkPolicy(ctx, req.Namespace, l, site); err != nil {
 			l.Error(err, "error ensuring connect network policy")
@@ -82,7 +82,7 @@ func (r *SiteReconciler) reconcileNetworkPolicies(ctx context.Context, req ctrl.
 	}
 
 	// Package Manager network policy
-	pmEnabled := isProductEnabled(site.Spec.PackageManager.Enabled)
+	pmEnabled := checkBool(site.Spec.PackageManager.Enabled, true)
 	if pmEnabled {
 		if err := r.reconcilePackageManagerNetworkPolicy(ctx, req.Namespace, l, site); err != nil {
 			l.Error(err, "error ensuring package manager network policy")
@@ -96,7 +96,7 @@ func (r *SiteReconciler) reconcileNetworkPolicies(ctx context.Context, req ctrl.
 	}
 
 	// Workbench network policies
-	workbenchEnabled := isProductEnabled(site.Spec.Workbench.Enabled)
+	workbenchEnabled := checkBool(site.Spec.Workbench.Enabled, true)
 	if workbenchEnabled {
 		if err := r.reconcileWorkbenchNetworkPolicy(ctx, req.Namespace, l, site); err != nil {
 			l.Error(err, "error ensuring workbench network policy")

internal/controller/core/workbench.go

@@ -1034,22 +1034,11 @@ func (r *WorkbenchReconciler) CleanupWorkbench(ctx context.Context, req ctrl.Req
 func (r *WorkbenchReconciler) deleteServingResources(ctx context.Context, req ctrl.Request, w *positcov1beta1.Workbench, l logr.Logger) error {
 	key := client.ObjectKey{Name: w.ComponentName(), Namespace: req.Namespace}
 
-	// INGRESS
-	if err := internal.BasicDelete(ctx, r, l, key, &networkingv1.Ingress{}); err != nil {
-		return err
-	}
-
-	// SERVICE
-	if err := internal.BasicDelete(ctx, r, l, key, &corev1.Service{}); err != nil {
-		return err
-	}
-
-	// DEPLOYMENT
-	if err := internal.BasicDelete(ctx, r, l, key, &appsv1.Deployment{}); err != nil {
-		return err
-	}
-
-	return nil
+	return internal.BatchDelete(ctx, r, l, key,
+		&networkingv1.Ingress{},
+		&corev1.Service{},
+		&appsv1.Deployment{},
+	)
 }
 
 // suspendDeployedService removes serving resources (Deployment, Service, Ingress)

internal/kubernetes_crudding.go

@@ -77,6 +77,16 @@ func BasicDelete(ctx context.Context, r product.SomeReconciler, l logr.Logger, k
 	return nil
 }
 
+// BatchDelete calls BasicDelete for each object in objects using the same key. Returns on the first error.
+func BatchDelete(ctx context.Context, r product.SomeReconciler, l logr.Logger, key client.ObjectKey, objects ...client.Object) error {
+	for _, obj := range objects {
+		if err := BasicDelete(ctx, r, l, key, obj); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // PvcCreateOrUpdate is careful only to patch valid fields _if they have changed_. Otherwise, leave things
 // alone! In particular, StorageClassName will throw a diff every time if we leave it as blank (the default), because
 // Kubernetes fills in the StorageClassName