Merge pull request #261 from xmidt-org/feat/refactor/simplify-listenerConfig-struct

chore: simplify listener config struct

Author: denopink

chrysom/listenerClient.go

@@ -39,7 +39,7 @@ const (
 )
 
 // ListenerConfig contains config data for polling the Argus client.
-type ListenerClientConfig struct {
+type ListenerConfig struct {
 	// Listener provides a mechanism to fetch a copy of all items within a bucket on
 	// an interval.
 	// (Optional). If not provided, listening won't be enabled for this client.
@@ -73,19 +73,18 @@ type observerConfig struct {
 
 // NewListenerClient creates a new ListenerClient to be used to poll Argus
 // for updates.
-func NewListenerClient(config ListenerClientConfig,
+func NewListenerClient(config ListenerConfig,
 	setLogger func(context.Context, *zap.Logger) context.Context,
 	measures Measures, r Reader,
 ) (*ListenerClient, error) {
-	err := validateListenerConfig(&config)
-	if err != nil {
-		return nil, err
+	if config.Listener == nil {
+		return nil, ErrNoListenerProvided
 	}
-
-	if setLogger == nil {
-		setLogger = func(ctx context.Context, _ *zap.Logger) context.Context {
-			return ctx
-		}
+	if config.Logger == nil {
+		config.Logger = sallust.Default()
+	}
+	if config.PullInterval == 0 {
+		config.PullInterval = defaultPullInterval
 	}
 	if r == nil {
 		return nil, ErrNoReaderProvided
@@ -166,16 +165,3 @@ func (c *ListenerClient) Stop(ctx context.Context) error {
 	atomic.SwapInt32(&c.observer.state, stopped)
 	return nil
 }
-
-func validateListenerConfig(config *ListenerClientConfig) error {
-	if config.Listener == nil {
-		return ErrNoListenerProvided
-	}
-	if config.Logger == nil {
-		config.Logger = sallust.Default()
-	}
-	if config.PullInterval == 0 {
-		config.PullInterval = defaultPullInterval
-	}
-	return nil
-}

chrysom/listenerClient_test.go

@@ -32,7 +32,7 @@ var (
 			},
 			[]string{OutcomeLabel},
 		)}
-	happyListenerClientConfig = ListenerClientConfig{
+	happyListenerConfig = ListenerConfig{
 		Listener:     mockListener,
 		PullInterval: time.Second,
 		Logger:       sallust.Default(),
@@ -109,86 +109,51 @@ func newStartStopClient(includeListener bool) (*ListenerClient, func(), error) {
 		rw.Write(getItemsValidPayload())
 	}))
 
-	config := ListenerClientConfig{
+	config := ListenerConfig{
 		PullInterval: time.Millisecond * 200,
 		Logger:       sallust.Default(),
 	}
 	if includeListener {
 		config.Listener = mockListener
 	}
-	client, err := NewListenerClient(config, nil, mockMeasures, &BasicClient{})
+	client, err := NewListenerClient(config, sallust.With, mockMeasures, &BasicClient{})
 	if err != nil {
 		return nil, nil, err
 	}
 
 	return client, server.Close, nil
 }
 
-func TestValidateListenerConfig(t *testing.T) {
-	tcs := []struct {
-		desc        string
-		expectedErr error
-		config      ListenerClientConfig
-	}{
-		{
-			desc:   "Happy case Success",
-			config: happyListenerClientConfig,
-		},
-		{
-			desc:        "No listener Failure",
-			config:      ListenerClientConfig{},
-			expectedErr: ErrNoListenerProvided,
-		},
-		{
-			desc: "No logger and no pull interval Success",
-			config: ListenerClientConfig{
-				Listener: mockListener,
-			},
-		},
-	}
-	for _, tc := range tcs {
-		t.Run(tc.desc, func(t *testing.T) {
-			assert := assert.New(t)
-			c := tc.config
-			err := validateListenerConfig(&c)
-			assert.True(errors.Is(err, tc.expectedErr),
-				fmt.Errorf("error [%v] doesn't contain error [%v] in its err chain",
-					err, tc.expectedErr),
-			)
-		})
-	}
-}
-
 func TestNewListenerClient(t *testing.T) {
 	tcs := []struct {
 		desc        string
-		config      ListenerClientConfig
+		config      ListenerConfig
 		expectedErr error
 		measures    Measures
 		reader      Reader
 	}{
 		{
 			desc:        "Listener Config Failure",
-			config:      ListenerClientConfig{},
+			config:      ListenerConfig{},
 			expectedErr: ErrNoListenerProvided,
 		},
 		{
 			desc:        "No reader Failure",
-			config:      happyListenerClientConfig,
+			config:      happyListenerConfig,
 			measures:    mockMeasures,
 			expectedErr: ErrNoReaderProvided,
 		},
 		{
 			desc:     "Happy case Success",
-			config:   happyListenerClientConfig,
+			config:   happyListenerConfig,
 			measures: mockMeasures,
 			reader:   &BasicClient{},
 		},
 	}
 	for _, tc := range tcs {
 		t.Run(tc.desc, func(t *testing.T) {
 			assert := assert.New(t)
-			_, err := NewListenerClient(tc.config, nil, tc.measures, tc.reader)
+			_, err := NewListenerClient(tc.config, sallust.With, tc.measures, tc.reader)
 			assert.True(errors.Is(err, tc.expectedErr),
 				fmt.Errorf("error [%v] doesn't contain error [%v] in its err chain",
 					err, tc.expectedErr),

go.mod

@@ -12,6 +12,7 @@ require (
 	github.com/prometheus/client_model v0.6.1
 	github.com/spf13/cast v1.6.0
 	github.com/stretchr/testify v1.9.0
+	github.com/xmidt-org/arrange v0.4.0
 	github.com/xmidt-org/bascule v0.11.6
 	github.com/xmidt-org/httpaux v0.4.0
 	github.com/xmidt-org/sallust v0.2.2
@@ -31,6 +32,7 @@ require (
 	github.com/go-logfmt/logfmt v0.6.0 // indirect
 	github.com/gorilla/mux v1.8.1 // indirect
 	github.com/hashicorp/hcl v1.0.0 // indirect
+	github.com/justinas/alice v1.2.0 // indirect
 	github.com/magiconair/properties v1.8.7 // indirect
 	github.com/mitchellh/mapstructure v1.5.0 // indirect
 	github.com/pelletier/go-toml/v2 v2.1.0 // indirect
@@ -45,7 +47,6 @@ require (
 	github.com/spf13/viper v1.18.2 // indirect
 	github.com/stretchr/objx v0.5.2 // indirect
 	github.com/subosito/gotenv v1.6.0 // indirect
-	github.com/xmidt-org/arrange v0.4.0 // indirect
 	go.uber.org/dig v1.17.1 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect

go.sum

@@ -274,6 +274,7 @@ github.com/jstemmer/go-junit-report v0.0.0-20190106144839-af01ea7f8024/go.mod h1
 github.com/jstemmer/go-junit-report v0.9.1/go.mod h1:Brl9GWCQeLvo8nXZwPNNblvFj/XSXhF0NWZEnDohbsk=
 github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w=
 github.com/julienschmidt/httprouter v1.3.0/go.mod h1:JR6WtHb+2LUe8TCKY3cZOxFyyO8IZAc4RVcycCCAKdM=
+github.com/justinas/alice v1.2.0 h1:+MHSA/vccVCF4Uq37S42jwlkvI2Xzl7zTPCN5BnZNVo=
 github.com/justinas/alice v1.2.0/go.mod h1:fN5HRH/reO/zrUflLfTN43t3vXvKzvZIENsNEe7i7qA=
 github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=

service.go

@@ -66,20 +66,6 @@ type Config struct {
 	Validation ValidatorConfig
 }
 
-// ListenerConfig contains information needed to initialize the Listener Client service.
-type ListenerConfig struct {
-	Config chrysom.ListenerClientConfig
-
-	// Logger for this package.
-	// Gets passed to Argus config before initializing the client.
-	// (Optional). Defaults to a no op logger.
-	Logger *zap.Logger
-
-	// Measures for instrumenting this package.
-	// Gets passed to Argus config before initializing the client.
-	Measures chrysom.Measures
-}
-
 type ClientService struct {
 	argus  chrysom.PushReader
 	logger *zap.Logger
@@ -109,12 +95,12 @@ func NewService(cfg Config, getLogger func(context.Context) *zap.Logger) (*Clien
 // StartListener builds the Argus listener client service from the given configuration.
 // It allows adding watchers for the internal subscription state. Call the returned
 // function when you are done watching for updates.
-func (s *ClientService) StartListener(cfg ListenerConfig, setLogger func(context.Context, *zap.Logger) context.Context, watches ...Watch) (func(), error) {
+func (s *ClientService) StartListener(cfg chrysom.ListenerConfig, setLogger func(context.Context, *zap.Logger) context.Context, metrics chrysom.Measures, watches ...Watch) (func(), error) {
 	if cfg.Logger == nil {
 		cfg.Logger = sallust.Default()
 	}
-	prepArgusListenerClientConfig(&cfg, watches...)
-	listener, err := chrysom.NewListenerClient(cfg.Config, setLogger, cfg.Measures, s.argus)
+	prepArgusListenerConfig(&cfg, metrics, watches...)
+	listener, err := chrysom.NewListenerClient(cfg, setLogger, metrics, s.argus)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create chrysom listener client: %v", err)
 	}
@@ -170,10 +156,10 @@ func prepArgusBasicClientConfig(cfg *Config) error {
 	return nil
 }
 
-func prepArgusListenerClientConfig(cfg *ListenerConfig, watches ...Watch) {
+func prepArgusListenerConfig(cfg *chrysom.ListenerConfig, metrics chrysom.Measures, watches ...Watch) {
 	logger := cfg.Logger
-	watches = append(watches, webhookListSizeWatch(cfg.Measures.WebhookListSizeGauge))
-	cfg.Config.Listener = chrysom.ListenerFunc(func(items chrysom.Items) {
+	watches = append(watches, webhookListSizeWatch(metrics.WebhookListSizeGauge))
+	cfg.Listener = chrysom.ListenerFunc(func(items chrysom.Items) {
 		iws, err := ItemsToInternalWebhooks(items)
 		if err != nil {
 			logger.Error("Failed to convert items to webhooks", zap.Error(err))
@@ -208,29 +194,25 @@ func ProvideService() fx.Option {
 type ListenerIn struct {
 	fx.In
 
-	Measures chrysom.Measures
-	Logger   *zap.Logger
-	Svc      *ClientService
-	Watcher  Watch
-	LC       fx.Lifecycle
+	Measures       chrysom.Measures
+	Logger         *zap.Logger
+	Svc            *ClientService
+	listenerConfig chrysom.ListenerConfig
+	Watcher        Watch
+	LC             fx.Lifecycle
 }
 
 func ProvideListener() fx.Option {
 	return fx.Options(
 		fx.Provide(
-			func(in ListenerIn) (listener ListenerConfig, err error) {
-				listener = ListenerConfig{
-					Measures: in.Measures,
-					Logger:   in.Logger,
-				}
-
-				stopWatches, err := in.Svc.StartListener(listener, setLoggerInContext(), in.Watcher)
+			func(in ListenerIn) (err error) {
+				stopWatches, err := in.Svc.StartListener(in.listenerConfig, setLoggerInContext(), in.Measures, in.Watcher)
 				if err != nil {
-					return listener, fmt.Errorf("webhook service start listener error: %v", err)
+					return fmt.Errorf("webhook service start listener error: %v", err)
 				}
 				in.LC.Append(fx.StopHook(stopWatches))
 
-				return listener, nil
+				return nil
 			},
 		),
 	)

service_test.go

@@ -62,16 +62,14 @@ func TestStartListener(t *testing.T) {
 	tcs := []struct {
 		desc           string
 		serviceConfig  Config
-		listenerConfig ListenerConfig
+		listenerConfig chrysom.ListenerConfig
 		svc            ClientService
 		expectedErr    bool
 	}{
 		{
-			desc: "Success Case",
-			svc:  *mockService,
-			listenerConfig: ListenerConfig{
-				Config: chrysom.ListenerClientConfig{},
-			},
+			desc:           "Success Case",
+			svc:            *mockService,
+			listenerConfig: chrysom.ListenerConfig{},
 		},
 		{
 			desc:        "Chrysom Listener Client Creation Failure",
@@ -81,7 +79,7 @@ func TestStartListener(t *testing.T) {
 	for _, tc := range tcs {
 		t.Run(tc.desc, func(t *testing.T) {
 			assert := assert.New(t)
-			_, err := tc.svc.StartListener(tc.listenerConfig, nil)
+			_, err := tc.svc.StartListener(tc.listenerConfig, sallust.With, chrysom.Measures{})
 			if tc.expectedErr {
 				assert.NotNil(err)
 				return