diff options
-rw-r--r-- | internal/source/gitlab/api/client.go | 2 | ||||
-rw-r--r-- | internal/source/gitlab/cache/cache.go | 9 | ||||
-rw-r--r-- | internal/source/gitlab/cache/cache_test.go | 32 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry.go | 4 | ||||
-rw-r--r-- | internal/source/gitlab/cache/retriever.go | 19 | ||||
-rw-r--r-- | internal/source/gitlab/client/client.go | 8 | ||||
-rw-r--r-- | internal/source/gitlab/client/client_stub.go | 6 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab.go | 3 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab_test.go | 7 |
9 files changed, 41 insertions, 49 deletions
diff --git a/internal/source/gitlab/api/client.go b/internal/source/gitlab/api/client.go index 7206e25a..5d575cc6 100644 --- a/internal/source/gitlab/api/client.go +++ b/internal/source/gitlab/api/client.go @@ -7,5 +7,5 @@ import ( // Client represents an interface we use to retrieve information from GitLab type Client interface { // GetLookup retrives an VirtualDomain from GitLab API and wraps it into Lookup - GetLookup(ctx context.Context, domain string) Lookup + GetLookup(ctx context.Context, domain string) *Lookup } diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go index 85f44e11..7bf755a3 100644 --- a/internal/source/gitlab/cache/cache.go +++ b/internal/source/gitlab/cache/cache.go @@ -20,7 +20,7 @@ func NewCache(client api.Client) *Cache { } } -// Resolve is going to return a Lookup based on a domain name. The caching +// GetLookup is going to return a lookup based on a domain name. The caching // algorithm works as follows: // - We first check if the cache entry exists, and if it is up-to-date. If it // is fresh we return the lookup entry from cache and it is a cache hit. @@ -69,7 +69,7 @@ func NewCache(client api.Client) *Cache { // - we create a lookup that contains information about an error // - we cache this response // - we pass this lookup upstream to all the clients -func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup { +func (c *Cache) GetLookup(ctx context.Context, domain string) *api.Lookup { entry := c.store.LoadOrCreate(domain) if entry.IsUpToDate() { @@ -84,8 +84,3 @@ func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup { return entry.Retrieve(ctx, c.client) } - -// New creates a new instance of Cache and sets default expiration -func New() *Cache { - return &Cache{} -} diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go index ac7c6726..27c11b6d 100644 --- a/internal/source/gitlab/cache/cache_test.go +++ b/internal/source/gitlab/cache/cache_test.go @@ -21,7 +21,7 @@ type client struct { failure error } -func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup { +func (c *client) GetLookup(ctx context.Context, _ string) *api.Lookup { var lookup api.Lookup // TODO This might not work on some architectures @@ -42,7 +42,7 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup { lookup.Error = c.failure } - return lookup + return &lookup } func withTestCache(config resolverConfig, block func(*Cache, *client)) { @@ -75,7 +75,7 @@ func (cache *Cache) withTestEntry(config entryConfig, block func(*Entry)) { entry := cache.store.LoadOrCreate(domain) if config.retrieved { - entry.setResponse(api.Lookup{Name: domain}) + entry.setResponse(&api.Lookup{Name: domain}) } if config.expired { @@ -96,12 +96,12 @@ type entryConfig struct { retrieved bool } -func TestResolve(t *testing.T) { +func TestGetLookup(t *testing.T) { t.Run("when item is not cached", func(t *testing.T) { withTestCache(resolverConfig{buffered: true}, func(cache *Cache, resolver *client) { resolver.domain <- "my.gitlab.com" - lookup := cache.Resolve(context.Background(), "my.gitlab.com") + lookup := cache.GetLookup(context.Background(), "my.gitlab.com") require.NoError(t, lookup.Error) require.Equal(t, "my.gitlab.com", lookup.Name) @@ -116,7 +116,7 @@ func TestResolve(t *testing.T) { receiver := func() { defer wg.Done() - cache.Resolve(ctx, "my.gitlab.com") + cache.GetLookup(ctx, "my.gitlab.com") } wg.Add(3) @@ -136,7 +136,7 @@ func TestResolve(t *testing.T) { t.Run("when item is in short cache", func(t *testing.T) { withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: false, retrieved: true}, func(*Entry) { - lookup := cache.Resolve(context.Background(), "my.gitlab.com") + lookup := cache.GetLookup(context.Background(), "my.gitlab.com") require.Equal(t, "my.gitlab.com", lookup.Name) require.Equal(t, uint64(0), resolver.lookups) @@ -150,7 +150,7 @@ func TestResolve(t *testing.T) { lookup := make(chan *api.Lookup, 1) go func() { - lookup <- cache.Resolve(context.Background(), "my.gitlab.com") + lookup <- cache.GetLookup(context.Background(), "my.gitlab.com") }() <-resolver.bootup @@ -170,7 +170,7 @@ func TestResolve(t *testing.T) { t.Run("when item is in long cache only", func(t *testing.T) { withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) { - lookup := cache.Resolve(context.Background(), "my.gitlab.com") + lookup := cache.GetLookup(context.Background(), "my.gitlab.com") require.Equal(t, "my.gitlab.com", lookup.Name) require.Equal(t, uint64(0), resolver.lookups) @@ -184,9 +184,9 @@ func TestResolve(t *testing.T) { t.Run("when item in long cache is requested multiple times", func(t *testing.T) { withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) { - cache.Resolve(context.Background(), "my.gitlab.com") - cache.Resolve(context.Background(), "my.gitlab.com") - cache.Resolve(context.Background(), "my.gitlab.com") + cache.GetLookup(context.Background(), "my.gitlab.com") + cache.GetLookup(context.Background(), "my.gitlab.com") + cache.GetLookup(context.Background(), "my.gitlab.com") require.Equal(t, uint64(0), resolver.lookups) @@ -200,7 +200,7 @@ func TestResolve(t *testing.T) { withTestCache(resolverConfig{failure: errors.New("500 err")}, func(cache *Cache, resolver *client) { maxRetrievalInterval = 0 - lookup := cache.Resolve(context.Background(), "my.gitlab.com") + lookup := cache.GetLookup(context.Background(), "my.gitlab.com") require.Equal(t, uint64(3), resolver.lookups) require.EqualError(t, lookup.Error, "500 err") @@ -212,7 +212,7 @@ func TestResolve(t *testing.T) { cancel() withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { - lookup := cache.Resolve(ctx, "my.gitlab.com") + lookup := cache.GetLookup(ctx, "my.gitlab.com") require.Equal(t, uint64(0), resolver.lookups) require.EqualError(t, lookup.Error, "context done") @@ -224,7 +224,7 @@ func TestResolve(t *testing.T) { defer func() { retrievalTimeout = 5 * time.Second }() withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { - lookup := cache.Resolve(context.Background(), "my.gitlab.com") + lookup := cache.GetLookup(context.Background(), "my.gitlab.com") require.Equal(t, uint64(0), resolver.lookups) require.EqualError(t, lookup.Error, "retrieval context done") @@ -237,7 +237,7 @@ func TestResolve(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) response := make(chan *api.Lookup, 1) - go func() { response <- cache.Resolve(ctx, "my.gitlab.com") }() + go func() { response <- cache.GetLookup(ctx, "my.gitlab.com") }() cancel() diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index d91bb331..0bf5c3a8 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -92,11 +92,11 @@ func (e *Entry) retrieveWithClient(client api.Client) { e.setResponse(retriever.Retrieve(e.domain)) } -func (e *Entry) setResponse(lookup api.Lookup) { +func (e *Entry) setResponse(lookup *api.Lookup) { e.mux.Lock() defer e.mux.Unlock() - e.response = &lookup + e.response = lookup close(e.retrieved) } diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go index 1284b021..ca5421df 100644 --- a/internal/source/gitlab/cache/retriever.go +++ b/internal/source/gitlab/cache/retriever.go @@ -3,9 +3,10 @@ package cache import ( "context" "errors" - "fmt" "time" + log "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" ) @@ -17,28 +18,26 @@ type Retriever struct { // Retrieve retrieves a lookup response from external source with timeout and // backoff. It has its own context with timeout. -func (r *Retriever) Retrieve(domain string) api.Lookup { +func (r *Retriever) Retrieve(domain string) (lookup *api.Lookup) { ctx, cancel := context.WithTimeout(context.Background(), retrievalTimeout) defer cancel() - var lookup api.Lookup - select { case <-ctx.Done(): - fmt.Println("retrieval context done") // TODO logme - lookup = api.Lookup{Error: errors.New("retrieval context done")} + log.Debug("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 + log.Debug("retrieval response sent") } return lookup } -func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-chan api.Lookup { - response := make(chan api.Lookup) +func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-chan *api.Lookup { + response := make(chan *api.Lookup) go func() { - var lookup api.Lookup + var lookup *api.Lookup for i := 1; i <= maxRetrievalRetries; i++ { lookup = r.client.GetLookup(ctx, domain) diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index 9bf4bf32..e66ef51c 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -57,23 +57,23 @@ 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 { +func (gc *Client) GetLookup(ctx context.Context, host string) *api.Lookup { params := url.Values{} params.Set("host", host) resp, err := gc.get(ctx, "/api/v4/internal/pages", params) if err != nil { - return api.Lookup{Name: host, Error: err} + return &api.Lookup{Name: host, Error: err} } if resp == nil { - return api.Lookup{Name: host} + return &api.Lookup{Name: host} } lookup := api.Lookup{Name: host} lookup.Error = json.NewDecoder(resp.Body).Decode(&lookup.Domain) - return lookup + return &lookup } func (gc *Client) get(ctx context.Context, path string, params url.Values) (*http.Response, error) { diff --git a/internal/source/gitlab/client/client_stub.go b/internal/source/gitlab/client/client_stub.go index 00a689d8..9c8cf19a 100644 --- a/internal/source/gitlab/client/client_stub.go +++ b/internal/source/gitlab/client/client_stub.go @@ -14,17 +14,17 @@ type StubClient struct { } // GetLookup reads a test fixture and unmarshalls it -func (c StubClient) GetLookup(ctx context.Context, host string) api.Lookup { +func (c StubClient) GetLookup(ctx context.Context, host string) *api.Lookup { lookup := api.Lookup{Name: host} f, err := os.Open(c.File) if err != nil { lookup.Error = err - return lookup + return &lookup } defer f.Close() lookup.Error = json.NewDecoder(f).Decode(&lookup.Domain) - return lookup + return &lookup } diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 7977e35a..18798b4f 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -19,7 +19,6 @@ import ( // information about domains from GitLab instance. type Gitlab struct { client api.Client - cache *cache.Cache // WIP } // New returns a new instance of gitlab domain source. @@ -29,7 +28,7 @@ func New(config client.Config) (*Gitlab, error) { return nil, err } - return &Gitlab{client: client, cache: cache.New()}, nil + return &Gitlab{client: cache.NewCache(client)}, nil } // GetDomain return a representation of a domain that we have fetched from diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go index ad5144f5..d6a81269 100644 --- a/internal/source/gitlab/gitlab_test.go +++ b/internal/source/gitlab/gitlab_test.go @@ -6,14 +6,13 @@ import ( "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/cache" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client" ) func TestGetDomain(t *testing.T) { t.Run("when the response if correct", func(t *testing.T) { client := client.StubClient{File: "client/testdata/test.gitlab.io.json"} - source := Gitlab{client: client, cache: cache.New()} + source := Gitlab{client: client} domain, err := source.GetDomain("test.gitlab.io") require.NoError(t, err) @@ -23,7 +22,7 @@ func TestGetDomain(t *testing.T) { t.Run("when the response is not valid", func(t *testing.T) { client := client.StubClient{File: "/dev/null"} - source := Gitlab{client: client, cache: cache.New()} + source := Gitlab{client: client} domain, err := source.GetDomain("test.gitlab.io") @@ -34,7 +33,7 @@ func TestGetDomain(t *testing.T) { func TestResolve(t *testing.T) { client := client.StubClient{File: "client/testdata/test.gitlab.io.json"} - source := Gitlab{client: client, cache: cache.New()} + source := Gitlab{client: client} t.Run("when requesting a nested group project", func(t *testing.T) { target := "https://test.gitlab.io:443/my/pages/project/path/index.html" |