Repository: vshn/k8up
Pull request #363 (2021-02-16 10:16:29)

Various maintainability improvements
## Summary

* Removes some duplication in cfg/config.go
* Simplifies controller setup in main.go
* Addresses static analysis complaints
<!--
Thank you for your pull request. Please provide a description above and
review the checklist below.
-->

## Checklist
<!--
Remove items that do not apply. For completed items, change [ ] to [x].
-->

- [x] Keep pull requests small so they can be easily reviewed.
- [x] Categorize the PR by setting a good title and adding one of the labels:
      `bug`, `enhancement`, `documentation`, `change`, `breaking`,
      as they show up in the changelog
- [x] Update tests.

<!--
NOTE: these things are not required to open a PR and can be done afterwards,
while the PR is open.
-->



Patch:
diff --git a/cfg/config.go b/cfg/config.go
index a904bc16..4a38d014 100644
--- a/cfg/config.go
+++ b/cfg/config.go
@@ -96,17 +96,13 @@ func NewDefaultConfig() *Configuration {
 }
 
 func (c Configuration) ValidateSyntax() error {
-	if _, err := resource.ParseQuantity(c.GlobalMemoryResourceRequest); err != nil && c.GlobalMemoryResourceRequest != "" {
-		return fmt.Errorf("cannot parse global memory request: %w", err)
-	}
-	if _, err := resource.ParseQuantity(c.GlobalMemoryResourceLimit); err != nil && c.GlobalMemoryResourceLimit != "" {
-		return fmt.Errorf("cannot parse global memory limit: %w", err)
-	}
-	if _, err := resource.ParseQuantity(c.GlobalCPUResourceRequest); err != nil && c.GlobalCPUResourceRequest != "" {
-		return fmt.Errorf("cannot parse global CPU request: %w", err)
-	}
-	if _, err := resource.ParseQuantity(c.GlobalCPUResourceLimit); err != nil && c.GlobalCPUResourceLimit != "" {
-		return fmt.Errorf("cannot parse global CPU limit: %w", err)
+	if err := c.validateQuantities(map[string]string{
+		"memory request": c.GlobalMemoryResourceRequest,
+		"memory limit":   c.GlobalMemoryResourceLimit,
+		"cpu request":    c.GlobalCPUResourceRequest,
+		"cpu limit":      c.GlobalCPUResourceLimit,
+	}); err != nil {
+		return err
 	}
 	if c.OperatorNamespace == "" {
 		return fmt.Errorf("operator namespace cannot be empty")
@@ -114,6 +110,15 @@ func (c Configuration) ValidateSyntax() error {
 	return nil
 }
 
+func (c Configuration) validateQuantities(q map[string]string) error {
+	for k, v := range q {
+		if _, err := resource.ParseQuantity(v); err != nil && v != "" {
+			return fmt.Errorf("cannot parse global %s: %w", k, err)
+		}
+	}
+	return nil
+}
+
 func (c Configuration) GetGlobalDefaultResources() (res corev1.ResourceRequirements) {
 	if r, err := resource.ParseQuantity(c.GlobalMemoryResourceRequest); err == nil && c.GlobalMemoryResourceRequest != "" {
 		if res.Requests == nil {
diff --git a/controllers/archive_controller.go b/controllers/archive_controller.go
index d8fdc55d..99278463 100644
--- a/controllers/archive_controller.go
+++ b/controllers/archive_controller.go
@@ -51,7 +51,11 @@ func (r *ArchiveReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	return ctrl.Result{RequeueAfter: time.Second * 30}, archiveHandler.Handle()
 }
 
-func (r *ArchiveReconciler) SetupWithManager(mgr ctrl.Manager) error {
+// SetupWithManager configures the reconciler.
+func (r *ArchiveReconciler) SetupWithManager(mgr ctrl.Manager, l logr.Logger) error {
+	r.Client = mgr.GetClient()
+	r.Scheme = mgr.GetScheme()
+	r.Log = l
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&k8upv1alpha1.Archive{}).
 		WithEventFilter(predicate.GenerationChangedPredicate{}).
diff --git a/controllers/backup_controller.go b/controllers/backup_controller.go
index 275f081d..b0e1960e 100644
--- a/controllers/backup_controller.go
+++ b/controllers/backup_controller.go
@@ -61,7 +61,11 @@ func (r *BackupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	return ctrl.Result{RequeueAfter: time.Second * 30}, backupHandler.Handle()
 }
 
-func (r *BackupReconciler) SetupWithManager(mgr ctrl.Manager) error {
+// SetupWithManager configures the reconciler.
+func (r *BackupReconciler) SetupWithManager(mgr ctrl.Manager, l logr.Logger) error {
+	r.Client = mgr.GetClient()
+	r.Scheme = mgr.GetScheme()
+	r.Log = l
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&k8upv1alpha1.Backup{}).
 		WithEventFilter(predicate.GenerationChangedPredicate{}).
diff --git a/controllers/check_controller.go b/controllers/check_controller.go
index 87ff8d3e..7914ffef 100644
--- a/controllers/check_controller.go
+++ b/controllers/check_controller.go
@@ -53,7 +53,11 @@ func (r *CheckReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 	return ctrl.Result{RequeueAfter: time.Second * 30}, checkHandler.Handle()
 }
 
-func (r *CheckReconciler) SetupWithManager(mgr ctrl.Manager) error {
+// SetupWithManager configures the reconciler.
+func (r *CheckReconciler) SetupWithManager(mgr ctrl.Manager, l logr.Logger) error {
+	r.Client = mgr.GetClient()
+	r.Scheme = mgr.GetScheme()
+	r.Log = l
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&k8upv1alpha1.Check{}).
 		WithEventFilter(predicate.GenerationChangedPredicate{}).
diff --git a/controllers/controller_setup.go b/controllers/controller_setup.go
new file mode 100644
index 00000000..847d149c
--- /dev/null
+++ b/controllers/controller_setup.go
@@ -0,0 +1,11 @@
+package controllers
+
+import (
+	"github.com/go-logr/logr"
+	ctrl "sigs.k8s.io/controller-runtime"
+)
+
+// ReconcilerSetup is a common interface to configure reconcilers.
+type ReconcilerSetup interface {
+	SetupWithManager(mgr ctrl.Manager, l logr.Logger) error
+}
diff --git a/controllers/job_controller.go b/controllers/job_controller.go
index 780e1ded..aa370a15 100644
--- a/controllers/job_controller.go
+++ b/controllers/job_controller.go
@@ -45,7 +45,11 @@ func (r *JobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R
 	return ctrl.Result{}, handler.NewJobHandler(config, jobObj).Handle()
 }
 
-func (r *JobReconciler) SetupWithManager(mgr ctrl.Manager) error {
+// SetupWithManager configures the reconciler.
+func (r *JobReconciler) SetupWithManager(mgr ctrl.Manager, l logr.Logger) error {
+	r.Client = mgr.GetClient()
+	r.Scheme = mgr.GetScheme()
+	r.Log = l
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&batchv1.Job{}).
 		Complete(r)
diff --git a/controllers/prune_controller.go b/controllers/prune_controller.go
index 240630a6..a0be7d5c 100644
--- a/controllers/prune_controller.go
+++ b/controllers/prune_controller.go
@@ -55,7 +55,11 @@ func (r *PruneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 	return ctrl.Result{RequeueAfter: time.Second * 30}, pruneHandler.Handle()
 }
 
-func (r *PruneReconciler) SetupWithManager(mgr ctrl.Manager) error {
+// SetupWithManager configures the reconciler.
+func (r *PruneReconciler) SetupWithManager(mgr ctrl.Manager, l logr.Logger) error {
+	r.Client = mgr.GetClient()
+	r.Scheme = mgr.GetScheme()
+	r.Log = l
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&k8upv1alpha1.Prune{}).
 		WithEventFilter(predicate.GenerationChangedPredicate{}).
diff --git a/controllers/restore_controller.go b/controllers/restore_controller.go
index 3ca36a93..f7632822 100644
--- a/controllers/restore_controller.go
+++ b/controllers/restore_controller.go
@@ -55,7 +55,11 @@ func (r *RestoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	return ctrl.Result{RequeueAfter: time.Second * 30}, restoreHandler.Handle()
 }
 
-func (r *RestoreReconciler) SetupWithManager(mgr ctrl.Manager) error {
+// SetupWithManager configures the reconciler.
+func (r *RestoreReconciler) SetupWithManager(mgr ctrl.Manager, l logr.Logger) error {
+	r.Client = mgr.GetClient()
+	r.Scheme = mgr.GetScheme()
+	r.Log = l
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&k8upv1alpha1.Restore{}).
 		WithEventFilter(predicate.GenerationChangedPredicate{}).
diff --git a/controllers/schedule_controller.go b/controllers/schedule_controller.go
index 009ed9f2..f1ea931b 100644
--- a/controllers/schedule_controller.go
+++ b/controllers/schedule_controller.go
@@ -61,7 +61,10 @@ func (r *ScheduleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 }
 
 // SetupWithManager configures the reconciler.
-func (r *ScheduleReconciler) SetupWithManager(mgr ctrl.Manager) error {
+func (r *ScheduleReconciler) SetupWithManager(mgr ctrl.Manager, l logr.Logger) error {
+	r.Client = mgr.GetClient()
+	r.Scheme = mgr.GetScheme()
+	r.Log = l
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&k8upv1alpha1.Schedule{}).
 		WithEventFilter(predicate.GenerationChangedPredicate{}).
diff --git a/go.mod b/go.mod
index 2a8fc234..89dc079c 100644
--- a/go.mod
+++ b/go.mod
@@ -3,6 +3,7 @@ module github.com/vshn/k8up
 go 1.15
 
 require (
+    // When updating crd-ref-docs, verify that there were no changes from Elastic to hostile licenses.
 	github.com/elastic/crd-ref-docs v0.0.7
 	github.com/go-logr/logr v0.4.0
 	github.com/go-logr/zapr v0.4.0
diff --git a/main.go b/main.go
index 3a5c66f3..98def962 100644
--- a/main.go
+++ b/main.go
@@ -69,61 +69,19 @@ func main() {
 		os.Exit(1)
 	}
 
-	if err = (&controllers.ScheduleReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("Schedule"),
-		Scheme: mgr.GetScheme(),
-	}).SetupWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "Schedule")
-		os.Exit(1)
-	}
-	if err = (&controllers.BackupReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("Backup"),
-		Scheme: mgr.GetScheme(),
-	}).SetupWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "Backup")
-		os.Exit(1)
-	}
-	if err = (&controllers.RestoreReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("Restore"),
-		Scheme: mgr.GetScheme(),
-	}).SetupWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "Restore")
-		os.Exit(1)
-	}
-	if err = (&controllers.ArchiveReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("Archive"),
-		Scheme: mgr.GetScheme(),
-	}).SetupWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "Archive")
-		os.Exit(1)
-	}
-	if err = (&controllers.CheckReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("Check"),
-		Scheme: mgr.GetScheme(),
-	}).SetupWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "Check")
-		os.Exit(1)
-	}
-	if err = (&controllers.PruneReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("Prune"),
-		Scheme: mgr.GetScheme(),
-	}).SetupWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "Prune")
-		os.Exit(1)
-	}
-	if err = (&controllers.JobReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("Job"),
-		Scheme: mgr.GetScheme(),
-	}).SetupWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "Job")
-		os.Exit(1)
+	for name, reconciler := range map[string]controllers.ReconcilerSetup{
+		"Schedule": &controllers.ScheduleReconciler{},
+		"Backup":   &controllers.BackupReconciler{},
+		"Restore":  &controllers.RestoreReconciler{},
+		"Archive":  &controllers.ArchiveReconciler{},
+		"Check":    &controllers.CheckReconciler{},
+		"Prune":    &controllers.PruneReconciler{},
+		"Job":      &controllers.JobReconciler{},
+	} {
+		if err := reconciler.SetupWithManager(mgr, ctrl.Log.WithName("controllers").WithName(name)); err != nil {
+			setupLog.Error(err, "unable to create controller", "controller", name)
+			os.Exit(1)
+		}
 	}
 	// +kubebuilder:scaffold:builder
 


Test patch:
diff --git a/cfg/config_test.go b/cfg/config_test.go
index 04b097a1..622e6f61 100644
--- a/cfg/config_test.go
+++ b/cfg/config_test.go
@@ -7,10 +7,8 @@ import (
 )
 
 func Test_Configuration_ValidateSyntax(t *testing.T) {
-	type assertConfig = func(t *testing.T, c *Configuration)
 	tests := map[string]struct {
 		givenConfig       *Configuration
-		assertConfig      assertConfig
 		expectErr         bool
 		containErrMessage string
 	}{
@@ -19,6 +17,20 @@ func Test_Configuration_ValidateSyntax(t *testing.T) {
 			expectErr:         true,
 			containErrMessage: "operator namespace",
 		},
+		"GivenConfig_WhenResourceInvalid_ThenExpectError": {
+			givenConfig: NewDefaultConfig().WithOptions(func(c *Configuration) {
+				c.GlobalCPUResourceLimit = "invalid"
+			}),
+			expectErr:         true,
+			containErrMessage: "cpu limit",
+		},
+		"GivenConfig_WhenResourceValid_ThenExpectError": {
+			givenConfig: NewDefaultConfig().WithOptions(func(c *Configuration) {
+				c.GlobalCPUResourceLimit = "20m"
+				c.OperatorNamespace = "something-to-not-trigger-error"
+			}),
+			expectErr: false,
+		},
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
@@ -29,7 +41,6 @@ func Test_Configuration_ValidateSyntax(t *testing.T) {
 				return
 			}
 			assert.NoError(t, err)
-			tt.assertConfig(t, tt.givenConfig)
 		})
 	}
 }
@@ -40,3 +51,8 @@ func Test_Configuration_DefaultConfig(t *testing.T) {
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "operator namespace")
 }
+
+func (c *Configuration) WithOptions(f func(c *Configuration)) *Configuration {
+	f(c)
+	return c
+}