Merge pull request #61 from pwittrock/comments

Address Controller lib PR comments

Author: k8s-ci-robot

pkg/manager/internal.go

@@ -137,10 +137,9 @@ func (cm *controllerManager) Start(stop <-chan struct{}) error {
 		cm.mu.Lock()
 		defer cm.mu.Unlock()
 
-		// Start the Cache.
 		cm.stop = stop
 
-		// Allow the function to start the cache to be mocked out for testing
+		// Start the Cache. Allow the function to start the cache to be mocked out for testing
 		if cm.startCache == nil {
 			cm.startCache = cm.cache.Start
 		}
@@ -154,7 +153,7 @@ func (cm *controllerManager) Start(stop <-chan struct{}) error {
 		// TODO(community): Check the return value and write a test
 		cm.cache.WaitForCacheSync(stop)
 
-		// Start the runnables after the promises
+		// Start the runnables after the cache has synced
 		for _, c := range cm.runnables {
 			// Controllers block, but we want to return an error if any have an error starting.
 			// Write any Start errors to a channel so we can return them

pkg/manager/manager.go

@@ -72,7 +72,7 @@ type Options struct {
 	// Defaults to the kubernetes/client-go scheme.Scheme
 	Scheme *runtime.Scheme
 
-	// Mapper is the rest mapper used to map go types to Kubernetes APIs
+	// MapperProvider provides the rest mapper used to map go types to Kubernetes APIs
 	MapperProvider func(c *rest.Config) (meta.RESTMapper, error)
 
 	// Dependency injection for testing
@@ -98,52 +98,57 @@ func (r RunnableFunc) Start(s <-chan struct{}) error {
 
 // New returns a new Manager for creating Controllers.
 func New(config *rest.Config, options Options) (Manager, error) {
-	cm := &controllerManager{config: config, scheme: options.Scheme, errChan: make(chan error)}
-
 	// Initialize a rest.config if none was specified
-	if cm.config == nil {
+	if config == nil {
 		return nil, fmt.Errorf("must specify Config")
 	}
 
-	// Use the Kubernetes client-go scheme if none is specified
-	if cm.scheme == nil {
-		cm.scheme = scheme.Scheme
-	}
-
 	// Set default values for options fields
 	options = setOptionsDefaults(options)
 
-	mapper, err := options.MapperProvider(cm.config)
+	// Create the mapper provider
+	mapper, err := options.MapperProvider(config)
 	if err != nil {
 		log.Error(err, "Failed to get API Group-Resources")
 		return nil, err
 	}
 
 	// Create the Client for Write operations.
-	writeObj, err := options.newClient(cm.config, client.Options{Scheme: cm.scheme, Mapper: mapper})
+	writeObj, err := options.newClient(config, client.Options{Scheme: options.Scheme, Mapper: mapper})
 	if err != nil {
 		return nil, err
 	}
 
-	cm.cache, err = options.newCache(cm.config, cache.Options{Scheme: cm.scheme, Mapper: mapper})
+	// Create the cache for the cached read client and registering informers
+	cache, err := options.newCache(config, cache.Options{Scheme: options.Scheme, Mapper: mapper})
 	if err != nil {
 		return nil, err
-	}
-
-	cm.fieldIndexes = cm.cache
-	cm.client = client.DelegatingClient{Reader: cm.cache, Writer: writeObj}
 
+	}
 	// Create the recorder provider to inject event recorders for the components.
-	cm.recorderProvider, err = options.newRecorderProvider(cm.config, cm.scheme)
+	recorderProvider, err := options.newRecorderProvider(config, options.Scheme)
 	if err != nil {
 		return nil, err
 	}
 
-	return cm, nil
+	return &controllerManager{
+		config:           config,
+		scheme:           options.Scheme,
+		errChan:          make(chan error),
+		cache:            cache,
+		fieldIndexes:     cache,
+		client:           client.DelegatingClient{Reader: cache, Writer: writeObj},
+		recorderProvider: recorderProvider,
+	}, nil
 }
 
 // setOptionsDefaults set default values for Options fields
 func setOptionsDefaults(options Options) Options {
+	// Use the Kubernetes client-go scheme if none is specified
+	if options.Scheme == nil {
+		options.Scheme = scheme.Scheme
+	}
+
 	if options.MapperProvider == nil {
 		options.MapperProvider = apiutil.NewDiscoveryRESTMapper
 	}
@@ -154,7 +159,6 @@ func setOptionsDefaults(options Options) Options {
 	}
 
 	// Allow newCache to be mocked
-	// TODO(directxman12): Figure out how to allow users to request a client without requesting a watch
 	if options.newCache == nil {
 		options.newCache = cache.New
 	}