Simplify gitlab source client by not recording http status

8 files changed, 43 insertions, 61 deletions

diff --git a/internal/source/gitlab/api/lookup.go b/internal/source/gitlab/api/lookup.go
index 5b3b3d39..73a3ce43 100644
--- a/internal/source/gitlab/api/lookup.go
+++ b/internal/source/gitlab/api/lookup.go
@@ -3,7 +3,6 @@ package api
 // Lookup defines an API lookup action with a response that GitLab sends
 type Lookup struct {
 	Name   string
-	Domain VirtualDomain
-	Status int
 	Error  error
+	Domain *VirtualDomain
 }
diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index f140c479..cd6e6f06 100644
--- a/internal/source/gitlab/cache/cache_test.go
+++ b/internal/source/gitlab/cache/cache_test.go
@@ -19,7 +19,6 @@ type client struct {
 	bootup      chan uint64
 	domain      chan string
 	failure     error
-	status      int
 }
 
 func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
@@ -28,10 +27,6 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
 	c.bootup <- atomic.AddUint64(&c.started, 1)
 	defer atomic.AddUint64(&c.resolutions, 1)
 
-	if c.status == 0 {
-		lookup.Status = 200
-	}
-
 	if c.failure == nil {
 		lookup.Name = <-c.domain
 	} else {
@@ -71,7 +66,7 @@ func (cache *Cache) withTestEntry(config entryConfig, block func(*Entry)) {
 	entry := cache.store.LoadOrCreate(domain)
 
 	if config.retrieved {
-		entry.setResponse(api.Lookup{Name: domain, Status: 200})
+		entry.setResponse(api.Lookup{Name: domain})
 	}
 
 	if config.expired {
@@ -100,7 +95,6 @@ func TestResolve(t *testing.T) {
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
 			assert.NoError(t, lookup.Error)
-			assert.Equal(t, 200, lookup.Status)
 			assert.Equal(t, "my.gitlab.com", lookup.Name)
 			assert.Equal(t, uint64(1), resolver.resolutions)
 		})
diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go
index d0634763..45a89db5 100644
--- a/internal/source/gitlab/cache/entry.go
+++ b/internal/source/gitlab/cache/entry.go
@@ -73,7 +73,7 @@ func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lo
 
 	select {
 	case <-newctx.Done():
-		lookup = &api.Lookup{Name: e.domain, Status: 502, Error: errors.New("context done")}
+		lookup = &api.Lookup{Name: e.domain, Error: errors.New("context done")}
 	case <-e.retrieved:
 		lookup = e.Lookup()
 	}
diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go
index e0d04255..65d8249b 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -30,7 +30,7 @@ func (r *Retriever) Retrieve(domain string) api.Lookup {
 	select {
 	case <-ctx.Done():
 		fmt.Println("retrieval context done") // TODO logme
-		lookup = api.Lookup{Status: 502, Error: errors.New("retrieval context done")}
+		lookup = api.Lookup{Error: errors.New("retrieval context done")}
 	case lookup = <-r.resolveWithBackoff(ctx, domain):
 		fmt.Println("retrieval response sent") // TODO logme
 	}
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 628e2091..04cdf91c 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -4,6 +4,9 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
+	"io"
+	"io/ioutil"
 	"net/http"
 	"net/url"
 	"time"
@@ -21,12 +24,6 @@ type Client struct {
 	httpClient *http.Client
 }
 
-var (
-	errUnknown      = errors.New("Unknown")
-	errUnauthorized = errors.New("Unauthorized")
-	errNotFound     = errors.New("Not Found")
-)
-
 // TODO make these values configurable https://gitlab.com/gitlab-org/gitlab-pages/issues/274
 var tokenTimeout = 30 * time.Second
 var connectionTimeout = 10 * time.Second
@@ -61,64 +58,58 @@ func NewFromConfig(config Config) (*Client, error) {
 // GetLookup returns a VirtualDomain configuration wrap into a Lookup for a
 // given host
 func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
-	lookup := api.Lookup{Name: host}
-
 	params := url.Values{}
 	params.Set("host", host)
 
-	resp, status, err := gc.get(ctx, "/api/v4/internal/pages", params)
-	if resp != nil {
-		defer resp.Body.Close()
-	} else {
-		err = errors.New("empty response returned")
+	resp, err := gc.get(ctx, "/api/v4/internal/pages", params)
+	if err != nil {
+		return api.Lookup{Name: host, Error: err}
 	}
 
-	lookup.Status = status
-	lookup.Error = err
-
-	if err != nil || status == http.StatusNoContent {
-		return lookup
+	if resp == nil {
+		return api.Lookup{Name: host}
 	}
 
-	err = json.NewDecoder(resp.Body).Decode(&lookup.Domain)
-	if err != nil {
-		lookup.Error = err
-		return lookup
-	}
+	lookup := api.Lookup{Name: host}
+	lookup.Error = json.NewDecoder(resp.Body).Decode(&lookup.Domain)
 
 	return lookup
 }
 
-func (gc *Client) get(ctx context.Context, path string, params url.Values) (*http.Response, int, error) {
+func (gc *Client) get(ctx context.Context, path string, params url.Values) (*http.Response, error) {
 	endpoint, err := gc.endpoint(path, params)
 	if err != nil {
-		return nil, 0, err
+		return nil, err
 	}
 
 	req, err := gc.request(ctx, "GET", endpoint)
 	if err != nil {
-		return nil, 0, err
+		return nil, err
 	}
 
 	resp, err := gc.httpClient.Do(req)
 	if err != nil {
-		return nil, 0, err
+		return nil, err
+	}
+
+	if resp == nil {
+		return nil, errors.New("unknown response")
 	}
 
-	switch {
 	// StatusOK means we should return the API response
-	case resp.StatusCode == http.StatusOK:
-		return resp, resp.StatusCode, nil
+	if resp.StatusCode == http.StatusOK {
+		return resp, nil
+	}
+
+	io.Copy(ioutil.Discard, resp.Body)
+	resp.Body.Close()
+
 	// StatusNoContent means that a domain does not exist, it is not an error
-	case resp.StatusCode == http.StatusNoContent:
-		return resp, resp.StatusCode, nil
-	case resp.StatusCode == http.StatusUnauthorized:
-		return resp, resp.StatusCode, errUnauthorized
-	case resp.StatusCode == http.StatusNotFound:
-		return resp, resp.StatusCode, errNotFound
-	default:
-		return resp, resp.StatusCode, errUnknown
+	if resp.StatusCode == http.StatusNoContent {
+		return nil, nil
 	}
+
+	return nil, fmt.Errorf("HTTP status: %d", resp.StatusCode)
 }
 
 func (gc *Client) endpoint(path string, params url.Values) (*url.URL, error) {
diff --git a/internal/source/gitlab/client/client_stub.go b/internal/source/gitlab/client/client_stub.go
index 801039f1..70c00511 100644
--- a/internal/source/gitlab/client/client_stub.go
+++ b/internal/source/gitlab/client/client_stub.go
@@ -15,7 +15,7 @@ type StubClient struct {
 
 // GetLookup reads a test fixture and unmarshalls it
 func (c StubClient) GetLookup(ctx context.Context, host string) api.Lookup {
-	lookup := api.Lookup{Name: host, Status: 200}
+	lookup := api.Lookup{Name: host}
 
 	f, err := os.Open(c.File)
 	defer f.Close()
diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go
index 9b7632bd..5f6be9ca 100644
--- a/internal/source/gitlab/client/client_test.go
+++ b/internal/source/gitlab/client/client_test.go
@@ -10,8 +10,6 @@ import (
 
 	jwt "github.com/dgrijalva/jwt-go"
 	"github.com/stretchr/testify/require"
-
-	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
 
 var (
@@ -48,8 +46,8 @@ func TestNewInvalidBaseURL(t *testing.T) {
 
 func TestLookupForErrorResponses(t *testing.T) {
 	tests := map[int]string{
-		http.StatusUnauthorized: "Unauthorized",
-		http.StatusNotFound:     "Not Found",
+		http.StatusUnauthorized: "HTTP status: 401",
+		http.StatusNotFound:     "HTTP status: 404",
 	}
 
 	for statusCode, expectedError := range tests {
@@ -70,7 +68,7 @@ func TestLookupForErrorResponses(t *testing.T) {
 			lookup := client.GetLookup(context.Background(), "group.gitlab.io")
 
 			require.EqualError(t, lookup.Error, expectedError)
-			require.Equal(t, lookup.Domain, api.VirtualDomain{})
+			require.Nil(t, lookup.Domain)
 		})
 	}
 }
@@ -91,7 +89,7 @@ func TestMissingDomain(t *testing.T) {
 	lookup := client.GetLookup(context.Background(), "group.gitlab.io")
 
 	require.NoError(t, lookup.Error)
-	require.Equal(t, lookup.Domain, api.VirtualDomain{})
+	require.Nil(t, lookup.Domain)
 }
 
 func TestGetVirtualDomainAuthenticatedRequest(t *testing.T) {
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index 88b4004b..0221fd11 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -36,15 +36,15 @@ func New(config client.Config) (*Gitlab, error) {
 func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) {
 	lookup := g.client.GetLookup(context.Background(), name)
 
-	// NoContent response means that a domain does not exist
-	if lookup.Status == http.StatusNoContent {
-		return nil, nil
-	}
-
 	if lookup.Error != nil {
 		return nil, lookup.Error
 	}
 
+	// Domain does not exist
+	if lookup.Domain == nil {
+		return nil, nil
+	}
+
 	domain := domain.Domain{
 		Name:            name,
 		CertificateCert: lookup.Domain.Certificate,