Apply suggestions from code review

Co-authored-by: Corentin Néau <[email protected]>

setCondition doesn't need receiver

translate TODOs into notes

integrationtests/controller/bundle/bundle_targets_test.go

@@ -53,6 +53,9 @@ var _ = Describe("Bundle targets", Ordered, func() {
 		Expect(err).NotTo(HaveOccurred())
 		for _, bd := range bdList.Items {
 			err := k8sClient.Delete(ctx, &bd)
+			// BundleDeployments are now deleted in a loop by the controller, hence this delete operation
+			// should not be necessary. Pending further tests, we choose to ignore errors indicating that the bundle
+			// deployment has already been deleted here.
 			Expect(client.IgnoreNotFound(err)).NotTo(HaveOccurred())
 		}
 	})

internal/cmd/agent/controller/bundledeployment_controller.go

@@ -102,11 +102,11 @@ func (r *BundleDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		logger.V(1).Error(err, "Failed to deploy bundle", "status", status)
 
 		// do not use the returned status, instead set the condition and possibly a timestamp
-		bd.Status = r.setCondition(bd.Status, err, condition.Cond(fleetv1.BundleDeploymentConditionDeployed))
+		bd.Status = setCondition(bd.Status, err, condition.Cond(fleetv1.BundleDeploymentConditionDeployed))
 
 		merr = append(merr, fmt.Errorf("failed deploying bundle: %w", err))
 	} else {
-		bd.Status = r.setCondition(status, nil, condition.Cond(fleetv1.BundleDeploymentConditionDeployed))
+		bd.Status = setCondition(status, nil, condition.Cond(fleetv1.BundleDeploymentConditionDeployed))
 	}
 
 	// if we can't retrieve the resources, we don't need to try any of the other operations and requeue now
@@ -128,11 +128,11 @@ func (r *BundleDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Req
 			logger.Error(err, "Cannot monitor deployed bundle")
 
 			// if there is an error, do not use the returned status from monitor
-			bd.Status = r.setCondition(bd.Status, err, condition.Cond(fleetv1.BundleDeploymentConditionMonitored))
+			bd.Status = setCondition(bd.Status, err, condition.Cond(fleetv1.BundleDeploymentConditionMonitored))
 			merr = append(merr, fmt.Errorf("failed updating status: %w", err))
 		} else {
 			// we add to the status from deployer.DeployBundle
-			bd.Status = r.setCondition(status, nil, condition.Cond(fleetv1.BundleDeploymentConditionMonitored))
+			bd.Status = setCondition(status, nil, condition.Cond(fleetv1.BundleDeploymentConditionMonitored))
 		}
 
 		// Run drift correction
@@ -222,7 +222,7 @@ func (r *BundleDeploymentReconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 // setCondition sets the condition and updates the timestamp, if the condition changed
-func (r *BundleDeploymentReconciler) setCondition(newStatus fleetv1.BundleDeploymentStatus, err error, cond condition.Cond) fleetv1.BundleDeploymentStatus {
+func setCondition(newStatus fleetv1.BundleDeploymentStatus, err error, cond condition.Cond) fleetv1.BundleDeploymentStatus {
 	cond.SetError(&newStatus, "", ignoreConflict(err))
 	return newStatus
 }

internal/cmd/controller/gitrepo/restrictions.go

@@ -8,7 +8,6 @@ import (
 
 	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
 
-	"github.com/sirupsen/logrus"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -107,9 +106,7 @@ func isAllowedByRegex(currentValue, defaultValue string, patterns []string) (str
 
 		p, err := regexp.Compile(pattern)
 		if err != nil {
-			// TODO low level log statement, log in higher levels only
-			logrus.Infof("GitRepoRestriction failed to compile regex '%s'", pattern)
-			return currentValue, err
+			return currentValue, fmt.Errorf("GitRepoRestriction failed to compile regex '%s': %w", pattern, err)
 		}
 		if p.MatchString(currentValue) {
 			return currentValue, nil

internal/cmd/controller/reconciler/bundle_controller.go

@@ -200,9 +200,9 @@ func upper(op controllerutil.OperationResult) string {
 func (r *BundleReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&fleet.Bundle{}).
-		// TODO Maybe improve with WatchesMetadata, does it have access to labels?
+		// Note: Maybe improve with WatchesMetadata, does it have access to labels?
 		Watches(
-			// Fan out from  bundledeployment to bundle
+			// Fan out from bundledeployment to bundle
 			&fleet.BundleDeployment{},
 			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []ctrl.Request {
 				bd := a.(*fleet.BundleDeployment)

internal/cmd/controller/reconciler/cluster_controller.go

@@ -184,7 +184,10 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 func (r *ClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&fleet.Cluster{}).
-		// TODO Maybe we can filter events?
+		// Note: Maybe we can tune events after cleanup code is
+		// removed? This relies on bundledeployments and gitrepos to
+		// update its status. It also needs to trigger on
+		// cluster.Status.Namespace to create the namespace.
 		Complete(r)
 }
 

internal/cmd/controller/reconciler/gitrepo_controller.go

@@ -60,7 +60,7 @@ func (r *GitRepoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 			return ctrl.Result{}, err
 		}
 
-		// remove the job which was scheduled by imagescan
+		// remove the job scheduled by imagescan, if any
 		_ = r.Scheduler.DeleteJob(imagescan.GitCommitKey(req.Namespace, req.Name))
 
 		if err := purgeImageScans(ctx, r.Client, req.NamespacedName); err != nil {
@@ -111,7 +111,7 @@ func (r *GitRepoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		gitrepo.Status.Summary.Ready,
 		gitrepo.Status.Summary.DesiredReady)
 
-	r.setCondition(&gitrepo.Status, nil)
+	setCondition(&gitrepo.Status, nil)
 
 	err = r.updateStatus(ctx, req.NamespacedName, gitrepo.Status)
 	if err != nil {
@@ -189,15 +189,9 @@ func (r *GitRepoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	return ctrl.Result{}, nil
 }
 
-// setCondition sets the condition and updates the timestamp, if the condition changed
-func (r *GitRepoReconciler) setCondition(status *fleet.GitRepoStatus, err error) {
-	cond := condition.Cond(fleet.GitRepoAcceptedCondition)
-	cond.SetError(status, "", ignoreConflict(err))
-}
-
 // updateErrorStatus sets the condition in the status and tries to update the resource
 func (r *GitRepoReconciler) updateErrorStatus(ctx context.Context, req types.NamespacedName, status fleet.GitRepoStatus, orgErr error) error {
-	r.setCondition(&status, orgErr)
+	setCondition(&status, orgErr)
 	if statusErr := r.updateStatus(ctx, req, status); statusErr != nil {
 		merr := []error{orgErr, fmt.Errorf("failed to update the status: %w", statusErr)}
 		return errutil.NewAggregate(merr)
@@ -219,12 +213,12 @@ func (r *GitRepoReconciler) updateStatus(ctx context.Context, req types.Namespac
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *GitRepoReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	// TODO Maybe use mgr.GetFieldIndexer().IndexField?
+	// Note: Maybe use mgr.GetFieldIndexer().IndexField for better performance?
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&fleet.GitRepo{}).
 		Owns(&gitjob.GitJob{}).
 		Watches(
-			// Fan out from  bundle to gitrepo
+			// Fan out from bundle to gitrepo
 			&fleet.Bundle{},
 			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []ctrl.Request {
 				repo := a.GetLabels()[fleet.RepoLabel]
@@ -318,6 +312,12 @@ func acceptedLastUpdate(conds []genericcondition.GenericCondition) string {
 	return ""
 }
 
+// setCondition sets the condition and updates the timestamp, if the condition changed
+func setCondition(status *fleet.GitRepoStatus, err error) {
+	cond := condition.Cond(fleet.GitRepoAcceptedCondition)
+	cond.SetError(status, "", ignoreConflict(err))
+}
+
 func ignoreConflict(err error) error {
 	if apierrors.IsConflict(err) {
 		return nil

internal/cmd/controller/root.go

@@ -99,15 +99,15 @@ func (f *FleetManager) Run(cmd *cobra.Command, args []string) error {
 	if d := os.Getenv("CATTLE_ELECTION_RENEW_DEADLINE"); d != "" {
 		v, err := time.ParseDuration(d)
 		if err != nil {
-			setupLog.Error(err, "failed to parse CATTLE_ELECTION_LEASE_DURATION", "duration", d)
+			setupLog.Error(err, "failed to parse CATTLE_ELECTION_RENEW_DEADLINE", "duration", d)
 			return err
 		}
 		leaderOpts.RenewDeadline = &v
 	}
 	if d := os.Getenv("CATTLE_ELECTION_RETRY_PERIOD"); d != "" {
 		v, err := time.ParseDuration(d)
 		if err != nil {
-			setupLog.Error(err, "failed to parse CATTLE_ELECTION_LEASE_DURATION", "duration", d)
+			setupLog.Error(err, "failed to parse CATTLE_ELECTION_RETRY_PERIOD", "duration", d)
 			return err
 		}
 		leaderOpts.RetryPeriod = &v

internal/cmd/controller/target/builder.go

@@ -199,7 +199,6 @@ func preprocessHelmValues(logger logr.Logger, opts *fleet.BundleDeploymentOption
 	}
 
 	if !opts.Helm.DisablePreProcess {
-
 		templateValues := map[string]interface{}{}
 		if cluster.Spec.TemplateValues != nil {
 			templateValues = cluster.Spec.TemplateValues.Data

internal/name/name.go

@@ -93,7 +93,7 @@ func HelmReleaseName(str string) string {
 	if helmReleaseName.MatchString(str) && dnsLabelSafe.MatchString(str) {
 		short := Limit(str, v1alpha1.MaxHelmReleaseNameLen)
 		if short != str {
-			logrus.Debugf("shorten bundle name %v to %v", str, Limit(str, v1alpha1.MaxHelmReleaseNameLen))
+			logrus.Debugf("shorten bundle name %v to %v", str, short)
 		}
 		return short
 	}