address review comments

Signed-off-by: everettraven <everettraven@gmail.com>

internal/contentmanager/contentmanager.go

@@ -2,7 +2,9 @@ package contentmanager
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"sync"
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -20,15 +22,11 @@ import (
 	oclabels "github.com/operator-framework/operator-controller/internal/labels"
 )
 
-type ContentManager interface {
-	// ManageContent will:
-	// 1. Create a new controller-runtime cache.Cache belonging to the provided ClusterExtension
-	// 2. For each object provided:
-	//   A. Use the provided controller.Controller to establish a watch for the resource
-	ManageContent(context.Context, controller.Controller, *v1alpha1.ClusterExtension, []client.Object) error
-	// RemoveManagedContent will:
-	// 1. Remove/stop cache and any sources/informers for the provided ClusterExtension
-	RemoveManagedContent(*v1alpha1.ClusterExtension)
+type Watcher interface {
+	// Watch will establish watches for resources owned by a ClusterExtension
+	Watch(context.Context, controller.Controller, *v1alpha1.ClusterExtension, []client.Object) error
+	// Unwatch will remove watches for a ClusterExtension
+	Unwatch(*v1alpha1.ClusterExtension)
 }
 
 type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error)
@@ -43,15 +41,17 @@ type instance struct {
 	baseCfg         *rest.Config
 	extensionCaches map[string]extensionCacheData
 	mapper          meta.RESTMapper
+	mu              *sync.Mutex
 }
 
 // New creates a new ContentManager object
-func New(rcm RestConfigMapper, cfg *rest.Config, mapper meta.RESTMapper) ContentManager {
+func New(rcm RestConfigMapper, cfg *rest.Config, mapper meta.RESTMapper) Watcher {
 	return &instance{
 		rcm:             rcm,
 		baseCfg:         cfg,
 		extensionCaches: make(map[string]extensionCacheData),
 		mapper:          mapper,
+		mu:              &sync.Mutex{},
 	}
 }
 
@@ -72,7 +72,6 @@ func buildScheme(objs []client.Object) (*runtime.Scheme, error) {
 
 	for _, obj := range objs {
 		gvk := obj.GetObjectKind().GroupVersionKind()
-		listKind := obj.GetObjectKind().GroupVersionKind().Kind + "List"
 
 		// If the Kind or Version is not set in an object's GroupVersionKind
 		// attempting to add it to the runtime.Scheme will result in a panic.
@@ -93,6 +92,8 @@ func buildScheme(objs []client.Object) (*runtime.Scheme, error) {
 			)
 		}
 
+		listKind := gvk.Kind + "List"
+
 		if !scheme.Recognizes(gvk) {
 			// Since we can't have a mapping to every possible Go type in existence
 			// based on the GVK we need to use the unstructured types for mapping
@@ -113,13 +114,12 @@ func buildScheme(objs []client.Object) (*runtime.Scheme, error) {
 	return scheme, nil
 }
 
-// ManageContent configures a controller-runtime cache.Cache and establishes watches for the provided resources.
+// Watch configures a controller-runtime cache.Cache and establishes watches for the provided resources.
 // It utilizes the provided ClusterExtension to set a DefaultLabelSelector on the cache.Cache
 // to ensure it is only caching and reacting to content that belongs to the ClusterExtension.
 // For each client.Object provided, a new source.Kind is created and used in a call to the Watch() method
 // of the provided controller.Controller to establish new watches for the managed resources.
-func (i *instance) ManageContent(ctx context.Context, ctrl controller.Controller, ce *v1alpha1.ClusterExtension, objs []client.Object) error {
-	// Return nil if the objs slice is empty before any further action
+func (i *instance) Watch(ctx context.Context, ctrl controller.Controller, ce *v1alpha1.ClusterExtension, objs []client.Object) error {
 	if len(objs) == 0 || ce == nil || ctrl == nil {
 		return nil
 	}
@@ -131,11 +131,7 @@ func (i *instance) ManageContent(ctx context.Context, ctrl controller.Controller
 
 	scheme, err := buildScheme(objs)
 	if err != nil {
-		return fmt.Errorf(
-			"building scheme for %s; %w",
-			ce.GetName(),
-			err,
-		)
+		return fmt.Errorf("building scheme for ClusterExtension %q: %w", ce.GetName(), err)
 	}
 
 	tgtLabels := labels.Set{
@@ -175,29 +171,41 @@ func (i *instance) ManageContent(ctx context.Context, ctrl controller.Controller
 	// Doing this in a follow-up gives us the opportunity to verify that this functions
 	// as expected when wired up in the ClusterExtension reconciler before going too deep
 	// in optimizations.
+	i.mu.Lock()
 	if data, ok := i.extensionCaches[ce.GetName()]; ok {
 		data.Cancel()
 	}
+	i.mu.Unlock()
 
 	ctx, cancel := context.WithCancel(ctx)
 	go c.Start(ctx) //nolint:errcheck
+
+	if !c.WaitForCacheSync(ctx) {
+		cancel()
+		return errors.New("cache could not sync")
+	}
+
+	i.mu.Lock()
 	i.extensionCaches[ce.Name] = extensionCacheData{
 		Cache:  c,
 		Cancel: cancel,
 	}
+	i.mu.Unlock()
 
 	return nil
 }
 
-// RemoveManagedContent will stop the cache for the provided ClusterExtension
+// Unwatch will stop the cache for the provided ClusterExtension
 // stopping any watches on managed content
-func (i *instance) RemoveManagedContent(ce *v1alpha1.ClusterExtension) {
+func (i *instance) Unwatch(ce *v1alpha1.ClusterExtension) {
 	if ce == nil {
 		return
 	}
 
+	i.mu.Lock()
 	if data, ok := i.extensionCaches[ce.GetName()]; ok {
 		data.Cancel()
 		delete(i.extensionCaches, ce.GetName())
 	}
+	i.mu.Unlock()
 }

internal/contentmanager/contentmanager_test.go

@@ -20,7 +20,7 @@ import (
 	"github.com/operator-framework/operator-controller/api/v1alpha1"
 )
 
-func TestManageContent(t *testing.T) {
+func TestWatch(t *testing.T) {
 	tests := []struct {
 		name    string
 		rcm     RestConfigMapper
@@ -102,7 +102,7 @@ func TestManageContent(t *testing.T) {
 			require.NoError(t, err)
 
 			instance := New(tc.rcm, tc.config, mgr.GetRESTMapper())
-			got := instance.ManageContent(context.Background(), ctrl, tc.ce, tc.objs)
+			got := instance.Watch(context.Background(), ctrl, tc.ce, tc.objs)
 			assert.Equal(t, got != nil, tc.wantErr)
 		})
 	}