Repository: newrelic/newrelic-client-go
Pull request #86 (2020-01-10 07:39:34)

Refactoring Config
Looking at alternative ways to store config, and minimize the required imports for using the New Relic Client.


Patch:
diff --git a/internal/http/client.go b/internal/http/client.go
index 217affd..ed3a0c3 100644
--- a/internal/http/client.go
+++ b/internal/http/client.go
@@ -12,6 +12,7 @@ import (
 	"github.com/google/go-querystring/query"
 	retryablehttp "github.com/hashicorp/go-retryablehttp"
 
+	"github.com/newrelic/newrelic-client-go/internal/region"
 	"github.com/newrelic/newrelic-client-go/internal/version"
 	"github.com/newrelic/newrelic-client-go/pkg/config"
 	"github.com/newrelic/newrelic-client-go/pkg/errors"
@@ -52,7 +53,7 @@ func NewClient(cfg config.Config) NewRelicClient {
 	}
 
 	if cfg.BaseURL == "" {
-		cfg.BaseURL = config.DefaultBaseURLs[cfg.Region]
+		cfg.BaseURL = region.DefaultBaseURLs[region.Parse(cfg.Region)]
 	}
 
 	if cfg.UserAgent == "" {
diff --git a/internal/region/region.go b/internal/region/region.go
new file mode 100644
index 0000000..1b1fedf
--- /dev/null
+++ b/internal/region/region.go
@@ -0,0 +1,65 @@
+// Package region describes the operational regions defined for New Relic
+//
+// Regions are geographical locations where the New Relic platform operates
+// and this package provides an abstraction layer for handling them within
+// the New Relic Client and underlying APIs
+package region
+
+import (
+	"strings"
+)
+
+// Region represents the members of the Region enumeration.
+type Region int
+
+const (
+	_ = iota // Ignore zero
+
+	// US represents New Relic's US-based production deployment.
+	US Region = iota
+
+	// EU represents New Relic's EU-based production deployment.
+	EU
+
+	// Staging represents New Relic's US-based staging deployment.
+	// This is for internal New Relic use only.
+	Staging
+)
+
+// Default represents the region returned if nothing was specified
+const Default Region = US
+
+// DefaultBaseURLs represents the base API URLs for the different environments of the New Relic REST API V2.
+var DefaultBaseURLs = map[Region]string{
+	US:      "https://api.newrelic.com/v2",
+	EU:      "https://api.eu.newrelic.com/v2",
+	Staging: "https://staging-api.newrelic.com/v2",
+}
+
+// Parse takes a Region string and returns a RegionType
+func Parse(r string) Region {
+	switch strings.ToLower(r) {
+	case "us":
+		return US
+	case "eu":
+		return EU
+	case "staging":
+		return Staging
+	default:
+		return Default
+	}
+}
+
+// String returns a human readable value for the specified Region
+func (r Region) String() string {
+	switch r {
+	case US:
+		return "US"
+	case EU:
+		return "EU"
+	case Staging:
+		return "Staging"
+	default:
+		return "(Unknown)"
+	}
+}
diff --git a/newrelic/newrelic.go b/newrelic/newrelic.go
index 97bb19a..9437054 100644
--- a/newrelic/newrelic.go
+++ b/newrelic/newrelic.go
@@ -1,6 +1,10 @@
 package newrelic
 
 import (
+	"errors"
+	"net/http"
+	"time"
+
 	"github.com/newrelic/newrelic-client-go/pkg/alerts"
 	"github.com/newrelic/newrelic-client-go/pkg/apm"
 	"github.com/newrelic/newrelic-client-go/pkg/config"
@@ -19,12 +23,87 @@ type NewRelic struct {
 }
 
 // New returns a collection of New Relic APIs.
-func New(config config.Config) NewRelic {
-	return NewRelic{
+func New(apiKey string, opts ...ConfigOption) (*NewRelic, error) {
+	if apiKey == "" {
+		return nil, errors.New("apiKey required")
+	}
+
+	config := config.Config{
+		APIKey: apiKey,
+	}
+
+	// Loop through config options
+	for _, fn := range opts {
+		if nil != fn {
+			if err := fn(&config); err != nil {
+				return nil, err
+			}
+		}
+	}
+
+	nr := &NewRelic{
 		Alerts:     alerts.New(config),
 		APM:        apm.New(config),
 		Dashboards: dashboards.New(config),
 		Plugins:    plugins.New(config),
 		Synthetics: synthetics.New(config),
 	}
+
+	return nr, nil
+}
+
+// ConfigOption configures the Config when provided to NewApplication.
+type ConfigOption func(*config.Config) error
+
+// ConfigRegion sets the New Relic Region this client will use
+func ConfigRegion(region string) ConfigOption {
+	return func(cfg *config.Config) error {
+		cfg.Region = region
+		return nil
+	}
+}
+
+// ConfigHTTPTimeout sets the timeout for HTTP requests
+func ConfigHTTPTimeout(t time.Duration) ConfigOption {
+	return func(cfg *config.Config) error {
+		var timeout = &t
+		cfg.Timeout = timeout
+		return nil
+	}
+}
+
+// ConfigHTTPTransport sets the HTTP Transporter
+func ConfigHTTPTransport(transport *http.RoundTripper) ConfigOption {
+	return func(cfg *config.Config) error {
+		if transport != nil {
+			cfg.HTTPTransport = transport
+			return nil
+		}
+
+		return errors.New("HTTP Transport can not be nil")
+	}
+}
+
+// ConfigUserAgent sets the HTTP UserAgent for API requests
+func ConfigUserAgent(ua string) ConfigOption {
+	return func(cfg *config.Config) error {
+		if ua != "" {
+			cfg.UserAgent = ua
+			return nil
+		}
+
+		return errors.New("user-agent can not be empty")
+	}
+}
+
+// ConfigBaseURL sets the Base URL used to make requests
+func ConfigBaseURL(url string) ConfigOption {
+	return func(cfg *config.Config) error {
+		if url != "" {
+			cfg.BaseURL = url
+			return nil
+		}
+
+		return errors.New("base URL can not be empty")
+	}
 }
diff --git a/pkg/alerts/alerts.go b/pkg/alerts/alerts.go
index cfb2bbd..922ab9a 100644
--- a/pkg/alerts/alerts.go
+++ b/pkg/alerts/alerts.go
@@ -3,6 +3,7 @@ package alerts
 import (
 	"github.com/newrelic/newrelic-client-go/internal/http"
 	"github.com/newrelic/newrelic-client-go/internal/logging"
+	"github.com/newrelic/newrelic-client-go/internal/region"
 	"github.com/newrelic/newrelic-client-go/pkg/config"
 	"github.com/newrelic/newrelic-client-go/pkg/infrastructure"
 )
@@ -20,7 +21,7 @@ func New(config config.Config) Alerts {
 	infraConfig := config
 
 	if config.BaseURL == "" {
-		infraConfig.BaseURL = infrastructure.BaseURLs[config.Region]
+		infraConfig.BaseURL = infrastructure.BaseURLs[region.Parse(config.Region)]
 	}
 
 	infraClient := http.NewClient(infraConfig)
@@ -37,4 +38,4 @@ func New(config config.Config) Alerts {
 }
 
 // BaseURLs represents the base API URLs for the different environments of the New Relic REST API V2.
-var BaseURLs = config.DefaultBaseURLs
+var BaseURLs = region.DefaultBaseURLs
diff --git a/pkg/apm/apm.go b/pkg/apm/apm.go
index 8fbc56f..5d84117 100644
--- a/pkg/apm/apm.go
+++ b/pkg/apm/apm.go
@@ -3,6 +3,7 @@ package apm
 import (
 	"github.com/newrelic/newrelic-client-go/internal/http"
 	"github.com/newrelic/newrelic-client-go/internal/logging"
+	"github.com/newrelic/newrelic-client-go/internal/region"
 	"github.com/newrelic/newrelic-client-go/pkg/config"
 )
 
@@ -25,4 +26,4 @@ func New(config config.Config) APM {
 }
 
 // BaseURLs represents the base API URLs for the different environments of the New Relic REST API V2.
-var BaseURLs = config.DefaultBaseURLs
+var BaseURLs = region.DefaultBaseURLs
diff --git a/pkg/config/config.go b/pkg/config/config.go
index a5142ec..29f9cdd 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -8,47 +8,20 @@ import (
 	"github.com/newrelic/newrelic-client-go/internal/version"
 )
 
-// RegionType represents the members of the Region enumeration.
-type RegionType int
-
-// DefaultBaseURLs represents the base API URLs for the different environments of the New Relic REST API V2.
-var DefaultBaseURLs = map[RegionType]string{
-	Region.US:      "https://api.newrelic.com/v2",
-	Region.EU:      "https://api.eu.newrelic.com/v2",
-	Region.Staging: "https://staging-api.newrelic.com/v2",
-}
-
-const (
-	// US represents New Relic's US-based production deployment.
-	US = iota
-
-	// EU represents New Relic's EU-based production deployment.
-	EU
-
-	// Staging represents New Relic's US-based staging deployment.
-	// This is for internal New Relic use only.
-	Staging
-)
-
-// Region specifies the New Relic environment to target.
-var Region = struct {
-	US      RegionType
-	EU      RegionType
-	Staging RegionType
-}{
-	US:      US,
-	EU:      EU,
-	Staging: Staging,
-}
-
 // Config contains all the configuration data for the API Client.
 type Config struct {
-	BaseURL       string
-	APIKey        string
+	// APIKey to authenticate API requests (REQUIRED)
+	APIKey string
+
+	// Region of the New Relic platform to use
+	// Valid values are: US, EU
+	Region string
+
+	// HTTP
 	Timeout       *time.Duration
 	HTTPTransport *http.RoundTripper
 	UserAgent     string
-	Region        RegionType
+	BaseURL       string
 
 	// LogLevel can be one of the following values:
 	// "panic", "fatal", "error", "warn", "info", "debug", "trace"
diff --git a/pkg/dashboards/dashboards.go b/pkg/dashboards/dashboards.go
index 177c301..f95df9b 100644
--- a/pkg/dashboards/dashboards.go
+++ b/pkg/dashboards/dashboards.go
@@ -6,6 +6,7 @@ import (
 
 	"github.com/newrelic/newrelic-client-go/internal/http"
 	"github.com/newrelic/newrelic-client-go/internal/logging"
+	"github.com/newrelic/newrelic-client-go/internal/region"
 	"github.com/newrelic/newrelic-client-go/pkg/config"
 )
 
@@ -28,7 +29,7 @@ func New(config config.Config) Dashboards {
 }
 
 // BaseURLs represents the base API URLs for the different environments of the New Relic REST API V2.
-var BaseURLs = config.DefaultBaseURLs
+var BaseURLs = region.DefaultBaseURLs
 
 // ListDashboardsParams represents a set of filters to be
 // used when querying New Relic dashboards.
diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go
index 4fdae5b..665b35d 100644
--- a/pkg/errors/errors.go
+++ b/pkg/errors/errors.go
@@ -1,3 +1,4 @@
+// Package errors converts HTTP errors to messages
 package errors
 
 import (
diff --git a/pkg/infrastructure/infrastructure.go b/pkg/infrastructure/infrastructure.go
index e56f88e..8eceb3d 100644
--- a/pkg/infrastructure/infrastructure.go
+++ b/pkg/infrastructure/infrastructure.go
@@ -1,14 +1,14 @@
 package infrastructure
 
 import (
-	"github.com/newrelic/newrelic-client-go/pkg/config"
+	"github.com/newrelic/newrelic-client-go/internal/region"
 )
 
 // BaseURLs describes the base URLs for the Infrastructure Alerts API.
-var BaseURLs = map[config.RegionType]string{
-	config.Region.US:      "https://infra-api.newrelic.com/v2",
-	config.Region.EU:      "https://infra-api.eu.newrelic.com/v2",
-	config.Region.Staging: "https://staging-infra-api.newrelic.com/v2",
+var BaseURLs = map[region.Region]string{
+	region.US:      "https://infra-api.newrelic.com/v2",
+	region.EU:      "https://infra-api.eu.newrelic.com/v2",
+	region.Staging: "https://staging-infra-api.newrelic.com/v2",
 }
 
 // ErrorResponse represents an error response from New Relic Infrastructure.
diff --git a/pkg/synthetics/synthetics.go b/pkg/synthetics/synthetics.go
index ff1d90a..b618f62 100644
--- a/pkg/synthetics/synthetics.go
+++ b/pkg/synthetics/synthetics.go
@@ -5,14 +5,15 @@ import (
 
 	"github.com/newrelic/newrelic-client-go/internal/http"
 	"github.com/newrelic/newrelic-client-go/internal/logging"
+	"github.com/newrelic/newrelic-client-go/internal/region"
 	"github.com/newrelic/newrelic-client-go/pkg/config"
 )
 
 // BaseURLs represents the base API URLs for the different environments of the Synthetics API.
-var BaseURLs = map[config.RegionType]string{
-	config.Region.US:      "https://synthetics.newrelic.com/synthetics/api/v3",
-	config.Region.EU:      "https://synthetics.eu.newrelic.com/synthetics/api/v3",
-	config.Region.Staging: "https://staging-synthetics.newrelic.com/synthetics/api/v3",
+var BaseURLs = map[region.Region]string{
+	region.US:      "https://synthetics.newrelic.com/synthetics/api/v3",
+	region.EU:      "https://synthetics.eu.newrelic.com/synthetics/api/v3",
+	region.Staging: "https://staging-synthetics.newrelic.com/synthetics/api/v3",
 }
 
 // Synthetics is used to communicate with the New Relic Synthetics product.
@@ -59,7 +60,7 @@ func (e *ErrorResponse) Error() string {
 func New(config config.Config) Synthetics {
 
 	if config.BaseURL == "" {
-		config.BaseURL = BaseURLs[config.Region]
+		config.BaseURL = BaseURLs[region.Parse(config.Region)]
 	}
 
 	client := http.NewClient(config)


Test patch:
diff --git a/internal/http/client_test.go b/internal/http/client_test.go
index a592346..d8aaf13 100644
--- a/internal/http/client_test.go
+++ b/internal/http/client_test.go
@@ -8,9 +8,11 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
+	"github.com/newrelic/newrelic-client-go/internal/region"
 	"github.com/newrelic/newrelic-client-go/pkg/config"
 	"github.com/newrelic/newrelic-client-go/pkg/errors"
-	"github.com/stretchr/testify/assert"
 )
 
 func TestConfig(t *testing.T) {
@@ -38,7 +40,7 @@ func TestConfigDefaults(t *testing.T) {
 		APIKey: testAPIKey,
 	})
 
-	assert.Equal(t, config.DefaultBaseURLs[c.Config.Region], c.Config.BaseURL)
+	assert.Equal(t, region.DefaultBaseURLs[region.Parse(c.Config.Region)], c.Config.BaseURL)
 	assert.Contains(t, c.Config.UserAgent, "newrelic/newrelic-client-go/")
 }
 
diff --git a/internal/region/region_test.go b/internal/region/region_test.go
new file mode 100644
index 0000000..30d957b
--- /dev/null
+++ b/internal/region/region_test.go
@@ -0,0 +1,55 @@
+// +build unit
+
+package region
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestParse(t *testing.T) {
+	t.Parallel()
+
+	pairs := map[string]Region{
+		"us":      US,
+		"Us":      US,
+		"uS":      US,
+		"US":      US,
+		"eu":      EU,
+		"Eu":      EU,
+		"eU":      EU,
+		"EU":      EU,
+		"staging": Staging,
+		"Staging": Staging,
+	}
+
+	for k, v := range pairs {
+		result := Parse(k)
+		assert.Equal(t, result, v)
+	}
+
+	// Default is US
+	result := Parse("")
+	assert.Equal(t, result, US)
+}
+
+func TestString(t *testing.T) {
+	t.Parallel()
+
+	pairs := map[Region]string{
+		US:      "US",
+		EU:      "EU",
+		Staging: "Staging",
+	}
+
+	for k, v := range pairs {
+		result := k.String()
+		assert.Equal(t, result, v)
+	}
+
+	// Verify that an uninitialized Region (should be 0) isn't known
+	var unk Region
+	result := unk.String()
+	assert.Equal(t, result, "(Unknown)")
+}
diff --git a/newrelic/newrelic_test.go b/newrelic/newrelic_test.go
index 2e5caa2..fefc936 100644
--- a/newrelic/newrelic_test.go
+++ b/newrelic/newrelic_test.go
@@ -3,11 +3,101 @@
 package newrelic
 
 import (
+	"errors"
+	"net/http"
 	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
 
 	"github.com/newrelic/newrelic-client-go/pkg/config"
 )
 
-func TestNew(t *testing.T) {
-	New(config.Config{})
+var testAPIkey = "asdf1234"
+
+func TestNew_invalid(t *testing.T) {
+	t.Parallel()
+
+	nr, err := New("")
+
+	assert.Nil(t, nr)
+	assert.Error(t, errors.New("apiKey required"), err)
+}
+
+func TestNew_basic(t *testing.T) {
+	t.Parallel()
+
+	nr, err := New(testAPIkey)
+
+	assert.NotNil(t, nr)
+	assert.NoError(t, err)
+}
+
+func TestNew_configOptionError(t *testing.T) {
+	t.Parallel()
+
+	badOption := func(cfg *config.Config) error { return errors.New("option with error") }
+	nr, err := New(testAPIkey, badOption)
+
+	assert.Nil(t, nr)
+	assert.Error(t, errors.New("option with error"), err)
+}
+
+func TestNew_setRegion(t *testing.T) {
+	t.Parallel()
+
+	nr, err := New(testAPIkey, ConfigRegion("US"))
+
+	assert.NotNil(t, nr)
+	assert.NoError(t, err)
+}
+
+func TestNew_optionTimeout(t *testing.T) {
+	t.Parallel()
+
+	timeout := time.Second * 30
+	nr, err := New(testAPIkey, ConfigHTTPTimeout(timeout))
+
+	assert.NotNil(t, nr)
+	assert.NoError(t, err)
+}
+
+func TestNew_optionTransport(t *testing.T) {
+	t.Parallel()
+
+	nr, err := New(testAPIkey, ConfigHTTPTransport(nil))
+	assert.Nil(t, nr)
+	assert.Error(t, errors.New("HTTP Transport can not be nil"), err)
+
+	transport := http.DefaultTransport
+	nr, err = New(testAPIkey, ConfigHTTPTransport(&transport))
+
+	assert.NotNil(t, nr)
+	assert.NoError(t, err)
+}
+
+func TestNew_optionUserAgent(t *testing.T) {
+	t.Parallel()
+
+	nr, err := New(testAPIkey, ConfigUserAgent(""))
+	assert.Nil(t, nr)
+	assert.Error(t, errors.New("user-agent can not be empty"), err)
+
+	nr, err = New(testAPIkey, ConfigUserAgent("my-user-agent"))
+
+	assert.NotNil(t, nr)
+	assert.NoError(t, err)
+}
+
+func TestNew_optionBaseURL(t *testing.T) {
+	t.Parallel()
+
+	nr, err := New(testAPIkey, ConfigBaseURL(""))
+	assert.Nil(t, nr)
+	assert.Error(t, errors.New("base URL can not be empty"), err)
+
+	nr, err = New(testAPIkey, ConfigBaseURL("http://localhost/"))
+
+	assert.NotNil(t, nr)
+	assert.NoError(t, err)
 }
diff --git a/pkg/errors/errors_test.go b/pkg/errors/errors_test.go
new file mode 100644
index 0000000..f4912b1
--- /dev/null
+++ b/pkg/errors/errors_test.go
@@ -0,0 +1,28 @@
+// +build unit
+
+package errors
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestErrorNotFound(t *testing.T) {
+	t.Parallel()
+
+	var e ErrorNotFound
+
+	assert.Equal(t, "404 not found", e.Error())
+}
+
+func TestErrorUnexpectedStatusCode(t *testing.T) {
+	t.Parallel()
+
+	e := ErrorUnexpectedStatusCode{
+		StatusCode: 99,
+		Err:        "wat",
+	}
+
+	assert.Equal(t, "99 response returned: wat", e.Error())
+}
diff --git a/pkg/synthetics/synthetics_test.go b/pkg/synthetics/synthetics_test.go
index bc45031..f0a01d0 100644
--- a/pkg/synthetics/synthetics_test.go
+++ b/pkg/synthetics/synthetics_test.go
@@ -7,45 +7,59 @@ import (
 	"net/http/httptest"
 	"testing"
 
-	"github.com/newrelic/newrelic-client-go/pkg/config"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/newrelic/newrelic-client-go/internal/region"
+	"github.com/newrelic/newrelic-client-go/pkg/config"
 )
 
-func TestDefaultEnvironment(t *testing.T) {
+func TestBaseURLs(t *testing.T) {
 	t.Parallel()
-	a := New(config.Config{})
 
-	actual := a.client.Config.BaseURL
-	expected := "https://synthetics.newrelic.com/synthetics/api/v3"
-	if actual != expected {
-		t.Errorf("expected baseURL: %s, received: %s", expected, actual)
+	pairs := map[region.Region]string{
+		region.US:      "https://synthetics.newrelic.com/synthetics/api/v3",
+		region.EU:      "https://synthetics.eu.newrelic.com/synthetics/api/v3",
+		region.Staging: "https://staging-synthetics.newrelic.com/synthetics/api/v3",
 	}
-}
 
-func TestEUEnvironment(t *testing.T) {
-	t.Parallel()
-	a := New(config.Config{
-		Region: config.EU,
-	})
+	// Default should be region.US
+	a := New(config.Config{})
+	assert.Equal(t, pairs[region.US], a.client.Config.BaseURL)
+
+	for k, v := range pairs {
+		b := New(config.Config{
+			Region: k.String(),
+		})
 
-	actual := a.client.Config.BaseURL
-	expected := "https://synthetics.eu.newrelic.com/synthetics/api/v3"
-	if actual != expected {
-		t.Errorf("expected baseURL: %s, received: %s", expected, actual)
+		assert.Equal(t, v, b.client.Config.BaseURL)
 	}
 }
 
-func TestStagingEnvironment(t *testing.T) {
+// TestError checks that messages are reported in the correct
+// order by going through priorities backwards
+func TestError(t *testing.T) {
 	t.Parallel()
-	a := New(config.Config{
-		Region: config.Staging,
-	})
 
-	actual := a.client.Config.BaseURL
-	expected := "https://staging-synthetics.newrelic.com/synthetics/api/v3"
-	if actual != expected {
-		t.Errorf("expected baseURL: %s, received: %s", expected, actual)
+	// Default empty
+	e := ErrorResponse{}
+	assert.Equal(t, "", e.Error())
+
+	// 3rd Messages concat
+	e.Messages = []ErrorDetail{
+		{Message: "detail message"},
+		{Message: "another detail"},
 	}
+	assert.Equal(t, "detail message, another detail", e.Error())
+
+	// 2nd Message
+	e.Message = "message"
+	assert.Equal(t, "message", e.Error())
+
+	// 1st Server Error Message
+	e.ServerErrorMessage = "server message"
+	assert.Equal(t, "server message", e.Error())
+
 }
 
 // nolint