adds code commentary and improves a log message

bootstrap/api/v1alpha1/agentbootstrapconfig_types.go

@@ -87,6 +87,7 @@ type AgentBootstrapConfigStatus struct {
 	// AgentRef references the agent this agent bootstrap config has booted
 	AgentRef *corev1.LocalObjectReference `json:"agentRef,omitempty"`
 
+	// TODO: should this be on the spec?
 	// ISODownloadURL is the url for the live-iso to be downloaded from Assisted Installer
 	ISODownloadURL string `json:"isoDownloadURL,omitempty"`
 

bootstrap/internal/controller/agent_controller.go

@@ -139,6 +139,8 @@ func (r *AgentReconciler) getMachineFromBMH(
 	return machine, nil
 }
 
+// findBMH returns the BareMetalHost whose BootMACAddress matches an interface
+// on the Agent's status.
 func (r *AgentReconciler) findBMH(
 	ctx context.Context,
 	agent *aiv1beta1.Agent,
@@ -147,6 +149,10 @@ func (r *AgentReconciler) findBMH(
 		return nil, errors.New("agent doesn't have inventory yet")
 	}
 
+	// TODO maybe we can find a better way than listing and scanning every BMH
+	// that exists, such as explicitly watching BMHs and setting up a
+	// FieldIndexer.
+	// Example: https://github.com/kubernetes-sigs/kubebuilder/blob/master/docs/book/src/reference/watching-resources/testdata/external-indexed-field/controller.go
 	bmhs := &metal3v1alpha1.BareMetalHostList{}
 	if err := r.Client.List(ctx, bmhs); err != nil {
 		return nil, err

bootstrap/internal/controller/agentbootstrapconfig_controller.go

@@ -228,6 +228,7 @@ func (r *AgentBootstrapConfigReconciler) Reconcile(ctx context.Context, req ctrl
 		return ctrl.Result{}, err
 	}
 
+	// check if the InfraEnv controller has set the ISO URL yet. If not, keep waiting.
 	if config.Status.ISODownloadURL == "" {
 		conditions.MarkFalse(
 			config,
@@ -239,6 +240,7 @@ func (r *AgentBootstrapConfigReconciler) Reconcile(ctx context.Context, req ctrl
 		return ctrl.Result{}, nil
 	}
 
+	// ensure the live ISO URL and type are set on the metal3 MachineTemplate
 	if err := r.setMetal3MachineTemplateImage(ctx, config, machine); err != nil {
 		conditions.MarkFalse(
 			config,
@@ -432,12 +434,14 @@ func (r *AgentBootstrapConfigReconciler) setMetal3MachineImage(
 			return err
 		}
 		log.V(logutil.TraceLevel).
-			Info("Added ISO URLs to metal3 machines", "machine", metal3Machine.Name, "namespace", metal3Machine.Namespace)
+			Info("Added ISO URL to metal3 machine", "machine", metal3Machine.Name, "namespace", metal3Machine.Namespace)
 	}
 	return nil
 }
 
-// Retrieves InfrastructureRefKey
+// getInfrastructureRefKey retrieves InfrastructureRefKey by looking for an
+// owner reference to a AgentControlPlane, and if not found, an owner reference
+// to a MachineDeployment. Returns an error if the Machine is owned by neither.
 func (r *AgentBootstrapConfigReconciler) getInfrastructureRefKey(
 	ctx context.Context,
 	machine *clusterv1.Machine,
@@ -461,7 +465,9 @@ func (r *AgentBootstrapConfigReconciler) getInfrastructureRefKey(
 	}, nil
 }
 
-// Overrides image reference by setting the LiveISO present on the AgentBootstrapConfig.Status.ISODownloadURL
+// setMetal3MachineTemplateImage overrides image reference by copying the
+// LiveISO URL from AgentBootstrapConfig.Status.ISODownloadURL to metal3
+// MachineTemplate Spec.
 func (r *AgentBootstrapConfigReconciler) setMetal3MachineTemplateImage(
 	ctx context.Context,
 	config *bootstrapv1alpha1.AgentBootstrapConfig,
@@ -553,6 +559,11 @@ func (r *AgentBootstrapConfigReconciler) SetupWithManager(mgr ctrl.Manager) erro
 		).
 		Watches(
 			&hivev1.ClusterDeployment{},
+			// TODO does this depend on the name and namespace for the
+			// ClusterDeployment being the same as the name and namespace of the
+			// ABC? Could we put an owner reference on the CD instead and use
+			// that as a concrete relationship, along with the
+			// EnqueueRequestForOwner handler?
 			&handler.EnqueueRequestForObject{},
 		).
 		Complete(r)
@@ -563,6 +574,7 @@ func (r *AgentBootstrapConfigReconciler) FilterMachine(_ context.Context, o clie
 	result := []ctrl.Request{}
 	m, ok := o.(*clusterv1.Machine)
 	if !ok {
+		// TODO handle this without a panic
 		panic(fmt.Sprintf("Expected a Machine but got a %T", o))
 	}
 	// m.Spec.ClusterName

bootstrap/internal/controller/infraenv_controller.go

@@ -70,13 +70,17 @@ func (r *InfraEnvReconciler) attachISOToAgentBootstrapConfigs(ctx context.Contex
 	}
 
 	for _, agentBootstrapConfig := range agentBootstrapConfigs.Items {
+		// skip ABCs where InfraEnvRef is nil or is set to some other InfraEnv
 		if agentBootstrapConfig.Status.InfraEnvRef == nil ||
 			(agentBootstrapConfig.Status.InfraEnvRef != nil &&
 				agentBootstrapConfig.Status.InfraEnvRef.Name != infraEnv.Name) {
 			continue
 		}
 
 		// Add ISO to agentBootstrapConfig status
+		// TODO: It's unconventional for a controller to set status on a
+		// resource that it doesn't own. This looks to me like further
+		// indication that the download URL should be in the ABC Spec.
 		agentBootstrapConfig.Status.ISODownloadURL = infraEnv.Status.ISODownloadURL
 		if err := r.Client.Status().Update(ctx, &agentBootstrapConfig); err != nil {
 			return errors.Wrap(err, "failed to update agentbootstrapconfig")