Apply review feedback

- Remove outdated comment
- Improve error handling when setting default Options
- Replace deprecated sink recording function
- log errors from StartEventWatcher

Signed-off-by: Borja Clemente <[email protected]>

Author: clebs

pkg/cluster/cluster.go

@@ -19,6 +19,7 @@ package cluster
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net/http"
 
 	"github.com/go-logr/logr"
@@ -162,8 +163,7 @@ func New(config *rest.Config, opts ...Option) (Cluster, error) {
 	}
 	options, err := setOptionsDefaults(options, config)
 	if err != nil {
-		options.Logger.Error(err, "Failed to set defaults")
-		return nil, err
+		return nil, fmt.Errorf("failed setting cluster default options: %w", err)
 	}
 
 	// Create the mapper provider

pkg/internal/recorder/recorder.go

@@ -51,9 +51,9 @@ type Provider struct {
 	evtClient       corev1client.EventInterface
 	makeBroadcaster EventBroadcasterProducer
 
-	broadcasterOnce sync.Once
-	broadcaster     events.EventBroadcaster
-	stopCh          chan struct{}
+	broadcasterOnce         sync.Once
+	broadcaster             events.EventBroadcaster
+	cancelSinkRecordingFunc context.CancelFunc
 	// Deprecated: will be removed in a future release. Use the broadcaster above instead.
 	deprecatedBroadcaster record.EventBroadcaster
 	stopBroadcaster       bool
@@ -81,7 +81,7 @@ func (p *Provider) Stop(shutdownCtx context.Context) {
 		if p.stopBroadcaster {
 			p.lock.Lock()
 			broadcaster.Shutdown()
-			close(p.stopCh)
+			p.cancelSinkRecordingFunc()
 			deprecatedBroadcaster.Shutdown()
 			p.stopped = true
 			p.lock.Unlock()
@@ -115,14 +115,18 @@ func (p *Provider) getBroadcaster() (record.EventBroadcaster, events.EventBroadc
 			})
 
 		// init new broadcaster
-		p.stopCh = make(chan struct{})
-		p.broadcaster.StartRecordingToSink(p.stopCh)
-		_, _ = p.broadcaster.StartEventWatcher(func(event runtime.Object) {
+		ctx, cancel := context.WithCancel(context.Background())
+		p.cancelSinkRecordingFunc = cancel
+		p.broadcaster.StartRecordingToSinkWithContext(ctx)
+		_, err := p.broadcaster.StartEventWatcher(func(event runtime.Object) {
 			e, isEvt := event.(*eventsv1.Event)
 			if isEvt {
 				p.logger.V(1).Info(e.Note, "type", e.Type, "object", e.Related, "action", e.Action, "reason", e.Reason)
 			}
 		})
+		if err != nil {
+			p.logger.Error(err, "error starting event watcher for broadcaster")
+		}
 	})
 
 	return p.deprecatedBroadcaster, p.broadcaster
@@ -190,6 +194,7 @@ func (l *lazyRecorder) Eventf(regarding runtime.Object, related runtime.Object,
 }
 
 // deprecatedRecorder implements the old events API during the tranisiton and will be removed in a future release.
+//
 // Deprecated: will be removed in a future release.
 type deprecatedRecorder struct {
 	prov *Provider

pkg/manager/manager.go

@@ -341,8 +341,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
 	// Set default values for options fields
 	options, err := setOptionsDefaults(config, options)
 	if err != nil {
-		options.Logger.Error(err, "Failed to set defaults")
-		return nil, err
+		return nil, fmt.Errorf("failed setting manager default options: %w", err)
 	}
 
 	cluster, err := cluster.New(config, func(clusterOptions *cluster.Options) {

pkg/recorder/recorder.go

@@ -32,6 +32,5 @@ type Provider interface {
 	// Deprecated: this uses the old events API and will be removed in a future release. Please use GetEventRecorder instead.
 	GetEventRecorderFor(name string) record.EventRecorder
 	// GetEventRecorder returns a EventRecorder with given name.
-	// The old API is not 100% supported anymore, use the new one whenever possible.
 	GetEventRecorder(name string) events.EventRecorder
 }