Support pagination in list catalog #201
Conversation
rmalpani-uber
commented
Aug 14, 2019
rmalpani-uber
commented
- Tagserver now sends list and listRepositories in paginated format
- TagClient support list and listRepositories with pagination
- registryoverride support pagination for catalog listing
- Test cases
build-index/tagserver/server.go
Outdated
| @@ -484,29 +470,29 @@ func buildPaginationOptions(u *url.URL) ([]backend.ListOption, error) { | |||
| func buildPaginationResponse(u *url.URL, continuationToken string, | |||
| result []string) (interface{}, error) { | |||
There was a problem hiding this comment.
Returning interface{} is dangerous and cause runtime issues. Only do it when you absolutely have to and comment on the return types.
There was a problem hiding this comment.
Initial, buildPaginationResponse was sending different responses based on argument, not anymore. Make sense to not send interface now.
build-index/tagclient/client.go
Outdated
| @@ -54,6 +58,21 @@ type singleClient struct { | |||
| tls *tls.Config | |||
| } | |||
|
|
|||
| // Request for ListWithPagination. | |||
| type ListRequest struct { | |||
There was a problem hiding this comment.
can we move ListRequest to tagmodels too?
There was a problem hiding this comment.
This is request from consumers of tagclient. Models are interaction between tagserver and tagclient.
build-index/tagclient/client.go
Outdated
| } | ||
|
|
||
| // Response from ListWithPagination. | ||
| type ListResponse struct { |
There was a problem hiding this comment.
is it possible to remove this struct and just use tagsmodels.ListResponse? Having two similar structs can be confusing.
There was a problem hiding this comment.
tagclient.ListResposne is internal struct to transfer data between different modules. tagmodels.ListResponse is http response though and contains field that are not relevant to tagclient.ListResponse. Let me rename tagclient.ListResponse to avoid confusion.
There was a problem hiding this comment.
imo having two similar structs is more confusing then a struct containing irrelevant fields. All you need is GetNames, GetOffset and GetNextURL ?
There was a problem hiding this comment.
Response from tagserver contains next in url format
e.g "/list/[a-z][A-Z][0-9]*?limt=&offset="
compared tagclient.ListWithPagination that contains offset(read next) as token-string. The reason is to hide it from the caller of tagclient.ListWithPagination. I can consolidate the two struct with Next field being interpreted as 2 different things based on the context?
|
It would be nice to document the behaviors of |
yep |
|
|
||
| // List Response with pagination. Models tagserver reponse to list and | ||
| // listRepository. | ||
| type ListResponse struct { |
There was a problem hiding this comment.
i'm still leaning towards unifying the two response structs. Also, looks like we parse the responses twice here to get offset and next. Is it possible to have ListResponse.GetOffset() and ListResponse.GetNext()?
There was a problem hiding this comment.
We are parsing twice for the reason above.
|
actually this test is failing |
+ Tagserver now sends list and listRepositories in paginated format + TagClient support list and listRepositories with pagination + registryoverride support pagination for catalog listing + Test cases
