ansible: pull watches into own package (#1523)

* ansible: pull watches into own package

In the same way that Helm has a watches package responsible for loading
the watches file, Ansible should be structured similiarly to separate
concerns, primarily, loading and validating the watches and creating
runners.

* temporarily remove runner tests

* Fix error msg formatting

* Add unit tests for the ansible runner package

- Move the runner struct next to the functions that accept it (back to
where it was before)
- Remove fields from runner that aren't needed anymore
  since we load the watches first, we can put manageStatus,
  reconcilePeriod, watchDependentResources, watchClusterScopedResources
  on the controller

* Finalizer can specify vars with no role/playbook

It is valid for a watch to have a finalizer with no role/playbook so
long as it specifies vars to be run with the playbook/role specified on
the watch.

* Remove extraneous comments and constants

Author: djzager

pkg/ansible/operator/operator.go

@@ -29,78 +29,80 @@ import (
 	"github.com/operator-framework/operator-sdk/pkg/ansible/flags"
 	"github.com/operator-framework/operator-sdk/pkg/ansible/proxy/controllermap"
 	"github.com/operator-framework/operator-sdk/pkg/ansible/runner"
+	"github.com/operator-framework/operator-sdk/pkg/ansible/watches"
 
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 	"sigs.k8s.io/controller-runtime/pkg/runtime/signals"
 )
 
+var log = logf.Log.WithName("manager")
+
 // Run - A blocking function which starts a controller-runtime manager
 // It starts an Operator by reading in the values in `./watches.yaml`, adds a controller
 // to the manager, and finally running the manager.
 func Run(done chan error, mgr manager.Manager, f *flags.AnsibleOperatorFlags, cMap *controllermap.ControllerMap) {
-	watches, err := runner.NewFromWatches(f.WatchesFile)
+	watchesSlice, err := watches.Load(f.WatchesFile)
 	if err != nil {
-		logf.Log.WithName("manager").Error(err, "Failed to get watches")
+		log.Error(err, "Failed to parse watches")
 		done <- err
 		return
 	}
 	rand.Seed(time.Now().Unix())
 	c := signals.SetupSignalHandler()
 
-	for gvk, runner := range watches {
-
-		// if the WORKER_* environment variable is set, use that value.
-		// Otherwise, use the value from the CLI. This is definitely
-		// counter-intuitive but it allows the operator admin adjust the
-		// number of workers based on their cluster resources. While the
-		// author may use the CLI option to specify a suggested
-		// configuration for the operator.
-		maxWorkers := getMaxWorkers(gvk, f.MaxWorkers)
+	for _, watch := range watchesSlice {
+		runner, err := runner.New(watch)
+		if err != nil {
+			log.Error(err, "Failed to create runner")
+			done <- err
+			return
+		}
 
 		o := controller.Options{
-			GVK:          gvk,
-			Runner:       runner,
-			ManageStatus: runner.GetManageStatus(),
-			MaxWorkers:   maxWorkers,
-		}
-		applyFlagsToControllerOptions(f, &o)
-		if d, ok := runner.GetReconcilePeriod(); ok {
-			o.ReconcilePeriod = d
+			GVK:             watch.GroupVersionKind,
+			Runner:          runner,
+			ManageStatus:    watch.ManageStatus,
+			MaxWorkers:      getMaxWorkers(watch.GroupVersionKind, f.MaxWorkers),
+			ReconcilePeriod: watch.ReconcilePeriod,
 		}
+
 		ctr := controller.Add(mgr, o)
 		if ctr == nil {
 			done <- errors.New("failed to add controller")
 			return
 		}
 		cMap.Store(o.GVK, &controllermap.Contents{Controller: *ctr,
-			WatchDependentResources:     runner.GetWatchDependentResources(),
-			WatchClusterScopedResources: runner.GetWatchClusterScopedResources(),
+			WatchDependentResources:     watch.WatchDependentResources,
+			WatchClusterScopedResources: watch.WatchClusterScopedResources,
 			OwnerWatchMap:               controllermap.NewWatchMap(),
 			AnnotationWatchMap:          controllermap.NewWatchMap(),
 		})
 	}
 	done <- mgr.Start(c)
 }
 
-func getMaxWorkers(gvk schema.GroupVersionKind, defvalue int) int {
-	envvar := formatEnvVar(gvk.Kind, gvk.Group)
-	maxWorkers, err := strconv.Atoi(os.Getenv(envvar))
-	if err != nil {
-		// we don't care why we couldn't parse it just use one.
-		// maybe we should log that we are defaulting to 1.
-		logf.Log.WithName("manager").V(0).Info(fmt.Sprintf("Using default value for workers %d", defvalue))
-		return defvalue
+// if the WORKER_* environment variable is set, use that value.
+// Otherwise, use the value from the CLI. This is definitely
+// counter-intuitive but it allows the operator admin adjust the
+// number of workers based on their cluster resources. While the
+// author may use the CLI option to specify a suggested
+// configuration for the operator.
+func getMaxWorkers(gvk schema.GroupVersionKind, defValue int) int {
+	envVar := strings.ToUpper(strings.Replace(
+		fmt.Sprintf("WORKER_%s_%s", gvk.Kind, gvk.Group),
+		".",
+		"_",
+		-1,
+	))
+	switch maxWorkers, err := strconv.Atoi(os.Getenv(envVar)); {
+	case maxWorkers <= 1:
+		return 1
+	case err != nil:
+		// we don't care why we couldn't parse it just use default
+		log.Info("Failed to parse %v from environment. Using default %v", envVar, defValue)
+		return defValue
+	default:
+		return maxWorkers
 	}
-
-	return maxWorkers
-}
-
-func formatEnvVar(kind string, group string) string {
-	envvar := fmt.Sprintf("WORKER_%s_%s", kind, group)
-	return strings.ToUpper(strings.Replace(envvar, ".", "_", -1))
-}
-
-func applyFlagsToControllerOptions(f *flags.AnsibleOperatorFlags, o *controller.Options) {
-	o.ReconcilePeriod = f.ReconcilePeriod
 }

pkg/ansible/operator/operator_test.go

@@ -26,47 +26,14 @@ import (
 
 // TODO: add a test for the Run method
 
-func TestFormatEnvVar(t *testing.T) {
-	testCases := []struct {
-		name     string
-		kind     string
-		group    string
-		expected string
-	}{
-		{
-			name:     "easy path",
-			kind:     "FooCluster",
-			group:    "cache.example.com",
-			expected: "WORKER_FOOCLUSTER_CACHE_EXAMPLE_COM",
-		},
-		{
-			name:     "missing kind",
-			kind:     "",
-			group:    "cache.example.com",
-			expected: "WORKER__CACHE_EXAMPLE_COM",
-		},
-		{
-			name:     "missing group",
-			kind:     "FooCluster",
-			group:    "",
-			expected: "WORKER_FOOCLUSTER_",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			assert.Equal(t, tc.expected, formatEnvVar(tc.kind, tc.group))
-		})
-	}
-}
-
 func TestMaxWorkers(t *testing.T) {
 	testCases := []struct {
 		name      string
 		gvk       schema.GroupVersionKind
 		defvalue  int
 		expected  int
 		setenvvar bool
+		envvar    string
 	}{
 		{
 			name: "no env, use default value",
@@ -78,6 +45,7 @@ func TestMaxWorkers(t *testing.T) {
 			defvalue:  1,
 			expected:  1,
 			setenvvar: false,
+			envvar:    "WORKER_MEMCACHESERVICE_CACHE_EXAMPLE_COM",
 		},
 		{
 			name: "env set to 3, expect 3",
@@ -89,14 +57,15 @@ func TestMaxWorkers(t *testing.T) {
 			defvalue:  1,
 			expected:  3,
 			setenvvar: true,
+			envvar:    "WORKER_MEMCACHESERVICE_CACHE_EXAMPLE_COM",
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			os.Unsetenv(formatEnvVar(tc.gvk.Kind, tc.gvk.Group))
+			os.Unsetenv(tc.envvar)
 			if tc.setenvvar {
-				os.Setenv(formatEnvVar(tc.gvk.Kind, tc.gvk.Group), strconv.Itoa(tc.expected))
+				os.Setenv(tc.envvar, strconv.Itoa(tc.expected))
 			}
 			assert.Equal(t, tc.expected, getMaxWorkers(tc.gvk, tc.defvalue))
 		})