diff options
author | Nick Thomas <nick@gitlab.com> | 2019-12-04 18:59:40 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2019-12-04 18:59:40 +0300 |
commit | 7f35a7b7c1dde36f695fd7f1627fa77d9d8d2be0 (patch) | |
tree | 3c2cb02406644a0b233ec90fe95aaaab4b93847a /internal | |
parent | b535811a888f52fc44925cc7abd3295b0e44903e (diff) | |
parent | 6b76b603ec5296666ec48d2e736cc66648f29504 (diff) |
Merge branch 'fix/gb/improve-error-reporting-in-client' into 'master'
Improve error handing when creating new domains config source
Closes #275
See merge request gitlab-org/gitlab-pages!205
Diffstat (limited to 'internal')
-rw-r--r-- | internal/source/domains.go | 19 | ||||
-rw-r--r-- | internal/source/domains_test.go | 50 | ||||
-rw-r--r-- | internal/source/gitlab/client/client.go | 14 | ||||
-rw-r--r-- | internal/source/gitlab/client/client_test.go | 34 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab.go | 9 |
5 files changed, 108 insertions, 18 deletions
diff --git a/internal/source/domains.go b/internal/source/domains.go index f47884ef..85d06152 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -36,11 +36,20 @@ type Domains struct { // NewDomains is a factory method for domains initializing a mutex. It should // not initialize `dm` as we later check the readiness by comparing it with a // nil value. -func NewDomains(config Config) *Domains { +func NewDomains(config Config) (*Domains, error) { + if len(config.GitlabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 { + return &Domains{disk: disk.New()}, nil + } + + gitlab, err := gitlab.New(config) + if err != nil { + return nil, err + } + return &Domains{ + gitlab: gitlab, disk: disk.New(), - gitlab: gitlab.New(config), - } + }, nil } // GetDomain retrieves a domain information from a source. We are using two @@ -69,6 +78,10 @@ func (d *Domains) IsReady() bool { } func (d *Domains) source(domain string) Source { + if d.gitlab == nil { + return d.disk + } + for _, name := range newSourceDomains { if domain == name { return d.gitlab diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 6854c359..378f8c89 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -3,14 +3,44 @@ package source import ( "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" ) -func TestHasDomain(t *testing.T) { +type sourceConfig struct { + api string + secret string +} + +func (c sourceConfig) GitlabServerURL() string { + return c.api +} + +func (c sourceConfig) GitlabAPISecret() []byte { + return []byte(c.secret) +} + +func TestDomainSources(t *testing.T) { + t.Run("when GitLab API URL has been provided", func(t *testing.T) { + domains, err := NewDomains(sourceConfig{api: "https://gitlab.com", secret: "abc"}) + require.NoError(t, err) + + require.NotNil(t, domains.gitlab) + require.NotNil(t, domains.disk) + }) + + t.Run("when GitLab API has not been provided", func(t *testing.T) { + domains, err := NewDomains(sourceConfig{}) + require.NoError(t, err) + + require.Nil(t, domains.gitlab) + require.NotNil(t, domains.disk) + }) +} + +func TestGetDomain(t *testing.T) { newSourceDomains = []string{"new-source-test.gitlab.io"} brokenSourceDomain = "pages-broken-poc.gitlab.io" @@ -43,7 +73,7 @@ func TestHasDomain(t *testing.T) { domain, err := domains.GetDomain("domain.test.io") require.NoError(t, err) - assert.Nil(t, domain) + require.Nil(t, domain) }) t.Run("when requesting a broken test domain", func(t *testing.T) { @@ -57,7 +87,17 @@ func TestHasDomain(t *testing.T) { domain, err := domains.GetDomain("pages-broken-poc.gitlab.io") - assert.Nil(t, domain) - assert.EqualError(t, err, "broken test domain used") + require.Nil(t, domain) + require.EqualError(t, err, "broken test domain used") + }) + + t.Run("when requesting a test domain in case of the source not being fully configured", func(t *testing.T) { + domains, err := NewDomains(sourceConfig{}) + require.NoError(t, err) + + domain, err := domains.GetDomain("new-source-test.gitlab.io") + + require.Nil(t, domain) + require.NoError(t, err) }) } diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index 6c9327dc..1c3024b7 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -9,8 +9,6 @@ import ( jwt "github.com/dgrijalva/jwt-go" - "gitlab.com/gitlab-org/labkit/log" - "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" ) @@ -35,10 +33,14 @@ var connectionTimeout = 10 * time.Second // NewClient initializes and returns new Client baseUrl is // appConfig.GitLabServer secretKey is appConfig.GitLabAPISecretKey -func NewClient(baseURL string, secretKey []byte) *Client { +func NewClient(baseURL string, secretKey []byte) (*Client, error) { + if len(baseURL) == 0 || len(secretKey) == 0 { + return nil, errors.New("GitLab API URL or API secret has not been provided") + } + url, err := url.Parse(baseURL) if err != nil { - log.WithError(err).Fatal("could not parse GitLab server URL") + return nil, err } return &Client{ @@ -48,11 +50,11 @@ func NewClient(baseURL string, secretKey []byte) *Client { Timeout: connectionTimeout, Transport: httptransport.Transport, }, - } + }, nil } // NewFromConfig creates a new client from Config struct -func NewFromConfig(config Config) *Client { +func NewFromConfig(config Config) (*Client, error) { return NewClient(config.GitlabServerURL(), config.GitlabAPISecret()) } diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go index f3ce4fa6..d056f2bd 100644 --- a/internal/source/gitlab/client/client_test.go +++ b/internal/source/gitlab/client/client_test.go @@ -16,6 +16,34 @@ var ( encodedSecret = "e41rcFh7XBA7sNABWVCe2AZvxMsy6QDtJ8S9Ql1UiN8=" // 32 bytes, base64 encoded ) +func TestNewValidBaseURL(t *testing.T) { + _, err := NewClient("https://gitlab.com", secretKey()) + require.NoError(t, err) +} + +func TestNewInvalidBaseURL(t *testing.T) { + t.Run("when API URL is not valid", func(t *testing.T) { + client, err := NewClient("%", secretKey()) + + require.Error(t, err) + require.Nil(t, client) + }) + + t.Run("when API URL is empty", func(t *testing.T) { + client, err := NewClient("", secretKey()) + + require.Nil(t, client) + require.EqualError(t, err, "GitLab API URL or API secret has not been provided") + }) + + t.Run("when API secret is empty", func(t *testing.T) { + client, err := NewClient("https://gitlab.com", []byte{}) + + require.Nil(t, client) + require.EqualError(t, err, "GitLab API URL or API secret has not been provided") + }) +} + func TestGetVirtualDomainForErrorResponses(t *testing.T) { tests := map[int]string{ http.StatusNoContent: "No Content", @@ -35,7 +63,8 @@ func TestGetVirtualDomainForErrorResponses(t *testing.T) { server := httptest.NewServer(mux) defer server.Close() - client := NewClient(server.URL, secretKey()) + client, err := NewClient(server.URL, secretKey()) + require.NoError(t, err) actual, err := client.GetVirtualDomain("group.gitlab.io") @@ -78,7 +107,8 @@ func TestGetVirtualDomainAuthenticatedRequest(t *testing.T) { server := httptest.NewServer(mux) defer server.Close() - client := NewClient(server.URL, secretKey()) + client, err := NewClient(server.URL, secretKey()) + require.NoError(t, err) actual, err := client.GetVirtualDomain("group.gitlab.io") require.NoError(t, err) diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 7cb88e42..a2911e6b 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -20,8 +20,13 @@ type Gitlab struct { } // New returns a new instance of gitlab domain source. -func New(config client.Config) *Gitlab { - return &Gitlab{client: client.NewFromConfig(config), cache: cache.New()} +func New(config client.Config) (*Gitlab, error) { + client, err := client.NewFromConfig(config) + if err != nil { + return nil, err + } + + return &Gitlab{client: client, cache: cache.New()}, nil } // GetDomain return a representation of a domain that we have fetched from |