chore: updates based pr feedback #269

Author: denopink

anclafx/provide.go

@@ -3,10 +3,8 @@
 package anclafx
 
 import (
-	"github.com/prometheus/client_golang/prometheus"
 	"github.com/xmidt-org/ancla"
 	"github.com/xmidt-org/ancla/chrysom"
-	"github.com/xmidt-org/touchstone"
 	"go.uber.org/fx"
 )
 
@@ -23,17 +21,6 @@ func Provide() fx.Option {
 			chrysom.ProvideDefaultListenerReader,
 			chrysom.ProvideListenerClient,
 		),
-		touchstone.Gauge(
-			prometheus.GaugeOpts{
-				Name: chrysom.WebhookListSizeGaugeName,
-				Help: chrysom.WebhookListSizeGaugeHelp,
-			}),
-		touchstone.CounterVec(
-			prometheus.CounterOpts{
-				Name: chrysom.PollsTotalCounterName,
-				Help: chrysom.PollsTotalCounterHelp,
-			},
-			chrysom.OutcomeLabel,
-		),
+		chrysom.ProvideMetrics(),
 	)
 }

chrysom/basicClient.go

@@ -66,7 +66,7 @@ type BasicClientConfig struct {
 
 // BasicClient is the client used to make requests to Argus.
 type BasicClient struct {
-	client       HTTPClient
+	client       *http.Client
 	auth         Acquirer
 	storeBaseURL string
 	bucket       string
@@ -88,16 +88,12 @@ const (
 // Items is a slice of model.Item(s) .
 type Items []model.Item
 
-type HTTPClient interface {
-	Do(*http.Request) (*http.Response, error)
-}
-
 type BasicClientIn struct {
 	fx.In
 
 	BasicClientConfig BasicClientConfig
 	// (Optional) Defaults to http.DefaultClient.
-	HTTPClient HTTPClient
+	HTTPClient *http.Client `name:"chrysom_http_client"`
 }
 
 // ProvideBasicClient provides a new BasicClient.
@@ -112,7 +108,7 @@ func ProvideBasicClient(in BasicClientIn) (*BasicClient, error) {
 
 // NewBasicClient creates a new BasicClient that can be used to
 // make requests to Argus.
-func NewBasicClient(config BasicClientConfig, client HTTPClient) (*BasicClient, error) {
+func NewBasicClient(config BasicClientConfig, client *http.Client) (*BasicClient, error) {
 	err := validateBasicConfig(&config)
 	if err != nil {
 		return nil, err

chrysom/basicClient_test.go

@@ -32,17 +32,17 @@ func TestValidateBasicConfig(t *testing.T) {
 	type testCase struct {
 		Description    string
 		Input          *BasicClientConfig
-		Client         HTTPClient
+		Client         *http.Client
 		ExpectedErr    error
 		ExpectedConfig *BasicClientConfig
 	}
 
 	allDefaultsCaseConfig := &BasicClientConfig{
-		Address: "http://awesome-argus-hostname.io",
+		Address: "example.com",
 		Bucket:  "bucket-name",
 	}
 	allDefinedCaseConfig := &BasicClientConfig{
-		Address: "http://legit-argus-hostname.io",
+		Address: "example.com",
 		Bucket:  "amazing-bucket",
 	}
 
@@ -57,22 +57,22 @@ func TestValidateBasicConfig(t *testing.T) {
 		{
 			Description: "No bucket",
 			Input: &BasicClientConfig{
-				Address: "http://awesome-argus-hostname.io",
+				Address: "example.com",
 			},
 			ExpectedErr: ErrBucketEmpty,
 		},
 		{
 			Description: "All default values",
 			Input: &BasicClientConfig{
-				Address: "http://awesome-argus-hostname.io",
+				Address: "example.com",
 				Bucket:  "bucket-name",
 			},
 			ExpectedConfig: allDefaultsCaseConfig,
 		},
 		{
 			Description: "All defined",
 			Input: &BasicClientConfig{
-				Address: "http://legit-argus-hostname.io",
+				Address: "example.com",
 				Bucket:  "amazing-bucket",
 			},
 			ExpectedConfig: allDefinedCaseConfig,
@@ -110,15 +110,15 @@ func TestSendRequest(t *testing.T) {
 		{
 			Description: "New Request fails",
 			Method:      "what method?",
-			URL:         "http://argus-hostname.io",
+			URL:         "example.com",
 			ExpectedErr: errNewRequestFailure,
 			MockAuth:    "",
 			MockError:   nil,
 		},
 		{
 			Description: "Auth acquirer fails",
 			Method:      http.MethodGet,
-			URL:         "http://argus-hostname.io",
+			URL:         "example.com",
 			MockError:   errFails,
 			MockAuth:    "",
 			ExpectedErr: ErrAuthAcquirerFailure,
@@ -134,7 +134,7 @@ func TestSendRequest(t *testing.T) {
 		{
 			Description: "Happy path",
 			Method:      http.MethodPut,
-			URL:         "http://argus-hostname.io",
+			URL:         "example.com",
 			Body:        []byte("testing"),
 			Owner:       "HappyCaseOwner",
 			ExpectedResponse: response{
@@ -147,7 +147,7 @@ func TestSendRequest(t *testing.T) {
 		{
 			Description: "Happy path with default http client",
 			Method:      http.MethodPut,
-			URL:         "http://argus-hostname.io",
+			URL:         "example.com",
 			Body:        []byte("testing"),
 			Owner:       "HappyCaseOwner",
 			ExpectedResponse: response{
@@ -176,13 +176,13 @@ func TestSendRequest(t *testing.T) {
 			server := httptest.NewServer(echoHandler)
 			defer server.Close()
 
-			var httpClient HTTPClient = server.Client()
+			httpClient := server.Client()
 			if tc.NilHTTPClient {
 				httpClient = nil
 			}
 
 			client, err := NewBasicClient(BasicClientConfig{
-				Address: "http://argus-hostname.io",
+				Address: "example.com",
 				Bucket:  "bucket-name",
 			}, httpClient)
 

chrysom/listenerClient.go

@@ -46,10 +46,13 @@ type ListenerClientIn struct {
 	// Listener fetches a copy of all items within a bucket on
 	// an interval based on `BasicClientConfig.PullInterval`.
 	// (Optional). If not provided, listening won't be enabled for this client.
-	Listener          Listener
-	Config            BasicClientConfig
+	Listener Listener
+	// Config configures the ancla client and its listeners.
+	Config BasicClientConfig
+	// PollsTotalCounter measures the number of polls (and their success/failure outcomes) to fetch new items.
 	PollsTotalCounter *prometheus.CounterVec `name:"chrysom_polls_total"`
-	Reader            Reader
+	// Reader is the DB interface used to fetch new items using `GeItems`.
+	Reader Reader
 }
 
 // ListenerClient is the client used to poll Argus for updates.

chrysom/metrics.go

@@ -3,6 +3,12 @@
 
 package chrysom
 
+import (
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/xmidt-org/touchstone"
+	"go.uber.org/fx"
+)
+
 // Names
 const (
 	WebhookListSizeGaugeName = "webhook_list_size"
@@ -21,3 +27,20 @@ const (
 	SuccessOutcome = "success"
 	FailureOutcome = "failure"
 )
+
+func ProvideMetrics() fx.Option {
+	return fx.Options(
+		touchstone.Gauge(
+			prometheus.GaugeOpts{
+				Name: WebhookListSizeGaugeName,
+				Help: WebhookListSizeGaugeHelp,
+			}),
+		touchstone.CounterVec(
+			prometheus.CounterOpts{
+				Name: PollsTotalCounterName,
+				Help: PollsTotalCounterHelp,
+			},
+			OutcomeLabel,
+		),
+	)
+}

service.go

@@ -114,13 +114,16 @@ func ProvideService(in ClientServiceIn) *ClientService {
 }
 
 // TODO: Refactor and move Watch and Listener related code to chrysom.
+type DefaultListenersIn struct {
+	fx.In
 
-type DefaultListenerOut struct {
-	Watchers []Watch `group:"watchers,flatten"`
+	WebhookListSizeGauge prometheus.Gauge `name:"webhook_list_size"`
 }
 
-type DefaultListenersIn struct {
-	WebhookListSizeGauge prometheus.Gauge `name:"webhook_list_size"`
+type DefaultListenerOut struct {
+	fx.Out
+
+	Watchers []Watch `group:"watchers,flatten"`
 }
 
 func ProvideDefaultListeners(in DefaultListenersIn) DefaultListenerOut {

webhookValidationConfig.go

@@ -51,7 +51,7 @@ type OptionsConfig struct {
 }
 
 // BuildURLChecker translates the configuration into url Checker to be run on the webhook.
-func BuildURLChecker(config ValidatorConfig) (*urlegit.Checker, error) {
+func (config *ValidatorConfig) BuildURLChecker() (*urlegit.Checker, error) {
 	var o []urlegit.Option
 	if len(config.URL.Schemes) > 0 {
 		o = append(o, urlegit.OnlyAllowSchemes(config.URL.Schemes...))

webhookValidationConfig_test.go

@@ -102,7 +102,7 @@ func TestBuildValidURLFuncs(t *testing.T) {
 	for _, tc := range tcs {
 		t.Run(tc.desc, func(t *testing.T) {
 			assert := assert.New(t)
-			vals, err := BuildURLChecker(tc.config)
+			vals, err := tc.config.BuildURLChecker()
 			if tc.expectedErr != nil {
 				assert.True(errors.Is(err, tc.expectedErr),
 					fmt.Errorf("error [%v] doesn't contain error [%v] in its err chain",
@@ -116,7 +116,7 @@ func TestBuildValidURLFuncs(t *testing.T) {
 }
 
 func TestBuildOptions(t *testing.T) {
-	checker, err := BuildURLChecker(buildAllConfig)
+	checker, err := buildAllConfig.BuildURLChecker()
 	assert.NoError(t, err)
 	opts := buildAllConfig.BuildOptions(checker)
 	assert.NotNil(t, opts)