Deep Review: 20260407-125326-rdd-set-command

After updating the template ConfigMap (line 207), the reconciler returns ctrl.Result{}, nil at line 214. The App reconciler Owns(&limav1alpha1.LimaVM{}) but does not own or watch ConfigMaps. When the VM is stopped, the template ConfigMap update produces no LimaVM status change, so no watch event fires. The reconciler never re-triggers and never reaches the running=true propagation at line 219.

			desired := applySpecToTemplate(r.LimaTemplateData, app.Spec)
			if templateCM.Data[limav1alpha1.TemplateConfigMapKey] != desired {
				if templateCM.Data == nil {
					templateCM.Data = make(map[string]string)
				}
				templateCM.Data[limav1alpha1.TemplateConfigMapKey] = desired
				if err := r.Update(ctx, templateCM); err != nil {
					return ctrl.Result{}, fmt.Errorf("failed to update template ConfigMap: %w", err)
				}
				log.Info("Updated template ConfigMap",
					"containerEngine", app.Spec.ContainerEngine.Name,
					"kubernetesEnabled", app.Spec.Kubernetes.Enabled,
					"kubernetesVersion", app.Spec.Kubernetes.Version)
				return ctrl.Result{}, nil
			}
		}
	}

	if !limaVM.Spec.Running && app.Spec.Running {
		limaVM.Spec.Running = true
		if err := r.Update(ctx, limaVM); err != nil {

This manifests when a user runs rdd set containerEngine.name=containerd running=true: the App object receives both changes in a single patch, but the reconciler processes the template change first, returns early, and never propagates the running state.

The reconciler declares Owns(&limav1alpha1.LimaVM{}) but not ConfigMaps:

func (r *AppReconciler) SetupWithManager(mgr ctrl.Manager) error {
	return ctrl.NewControllerManagedBy(mgr).
		For(&v1alpha1.App{}).
		Owns(&limav1alpha1.LimaVM{}).
		Complete(r)
}

Fix: Return ctrl.Result{Requeue: true}, nil so the reconciler re-evaluates after the template update:

 				return ctrl.Result{}, nil
+				return ctrl.Result{Requeue: true}, nil

When no App exists, createAndPatchApp creates a real App (line 266, no DryRunAll option) before performing a dry-run patch (line 285). The real App triggers the App controller, which creates a LimaVM, which creates a Lima instance on disk. The --dry-run flag applies only to the patch step, not the creation chain.

		"spec":       createSpec,
		},
	}

	_, err = dynClient.Resource(appGVR).Create(ctx, obj, metav1.CreateOptions{})
	if apierrors.IsAlreadyExists(err) {
		// Race: another rdd set created it concurrently. Retry as patch.
		var app appv1alpha1.App
		if err := c.Get(ctx, client.ObjectKey{Name: "app"}, &app); err != nil {
			return fmt.Errorf("failed to get App after concurrent create: %w", err)
		}
		return patchApp(ctx, c, &app, specMap, dryRun)
	}
	if err != nil {
		return fmt.Errorf("failed to create App: %w", err)
	}

	if dryRun {
		logrus.Info("App created with defaults")
		var app appv1alpha1.App
		if err := c.Get(ctx, client.ObjectKey{Name: "app"}, &app); err != nil {
			return fmt.Errorf("failed to get App: %w", err)
		}
		return patchApp(ctx, c, &app, specMap, true)
	}

The design doc (cmd_app.md) and BATS test (set.bats:131-137) confirm this is intentional: the admission controller needs a real App to validate the patch. But the semantic mismatch between --dry-run and "creates persistent resources" is a usability hazard.

Fix: Document this behavior in the --dry-run flag description. Consider whether the create step should also use DryRunAll (the subsequent patch would fail with NotFound, which could be caught and reported as "App does not exist; dry-run cannot validate without creating it").

The App controller fetches the template ConfigMap from limaVM.Status.TemplateConfigMap (owned by the LimaVM resource) and updates it directly at lines 194-217. Per docs/design/controllers.md: "Each controller cleans up only the resources it owns." The App controller should communicate desired state through the LimaVM spec or its own ConfigMap, not by reaching into LimaVM's internal state.

	if limaVM.Status.TemplateConfigMap != "" {
		templateCM := &corev1.ConfigMap{}
		if err := r.Get(ctx, client.ObjectKey{Name: limaVM.Status.TemplateConfigMap, Namespace: namespace}, templateCM); err != nil {
			if !apierrors.IsNotFound(err) {
				return ctrl.Result{}, fmt.Errorf("failed to fetch LimaVM template ConfigMap: %w", err)
			}
		} else {
			desired := applySpecToTemplate(r.LimaTemplateData, app.Spec)
			if templateCM.Data[limav1alpha1.TemplateConfigMapKey] != desired {
				if templateCM.Data == nil {
					templateCM.Data = make(map[string]string)
				}
				templateCM.Data[limav1alpha1.TemplateConfigMapKey] = desired
				if err := r.Update(ctx, templateCM); err != nil {
					return ctrl.Result{}, fmt.Errorf("failed to update template ConfigMap: %w", err)
				}
				log.Info("Updated template ConfigMap",
					"containerEngine", app.Spec.ContainerEngine.Name,
					"kubernetesEnabled", app.Spec.Kubernetes.Enabled,
					"kubernetesVersion", app.Spec.Kubernetes.Version)
				return ctrl.Result{}, nil
			}
		}
	}

Fix: If this boundary crossing is an accepted pragmatic choice, document it with a comment citing the design exception. Otherwise, consider adding a LimaVMSpec.TemplateOverrides field or having the App controller re-create its input ConfigMap.

The blanket empty-string rejection at line 450 means rdd set kubernetes.version= fails with "value must not be empty." Since kubernetes.version is an optional string (json:"version,omitempty" in app_types.go), setting it to empty is a valid way to unset it. No alternative exists via rdd set.

// coerceValue converts a raw string to the Go type indicated by the schema.
func coerceValue(schema *apiextensionsv1.JSONSchemaProps, raw string) (any, error) {
	if raw == "" {
		return nil, fmt.Errorf("value must not be empty")
	}

	// Check enum constraints (applies to all types, but mainly strings).
	if len(schema.Enum) > 0 {
		var validValues []string
		found := false

Fix: Allow empty strings for string-type fields. The merge patch sets the field to "", which omitempty treats as unset:

-if raw == "" {
-    return nil, fmt.Errorf("value must not be empty")
-}
+// Allow empty strings for optional string fields (clears the value).
+// Required fields are validated by the API server.

When a required string field has enum constraints (line 310), fillRequiredFields sets no default. All current CRD enum fields have kubebuilder defaults, so this is safe today. If a future revision adds a required enum field without a default, the dynamic Create at line 266 fails with a validation error whose cause is unclear.

		switch prop.Type {
		case "boolean":
			specMap[name] = false
		case "integer":
			specMap[name] = int64(0)
		case "string":
			if len(prop.Enum) == 0 {
				specMap[name] = ""
			}
		}
	}
	for name, val := range specMap {

Fix: Use the first enum value as the default:

 case "string":
     if len(prop.Enum) == 0 {
         specMap[name] = ""
+    } else {
+        var first string
+        if json.Unmarshal(prop.Enum[0].Raw, &first) == nil {
+            specMap[name] = first
+        }
     }