Support pagination for tagserver #190
Conversation
build-index/tagserver/server.go
Outdated
| return nil, handler.Errorf( | ||
| "invalid query %s:%s", k, v).Status(http.StatusBadRequest) | ||
| } | ||
| if k == limitQ { |
There was a problem hiding this comment.
Just 3 case, same perf and readable. Let me know if you think switch is more readable?
There was a problem hiding this comment.
i think it would be slightly more readable.
build-index/tagserver/server.go
Outdated
| "invalid query %s", k).Status(http.StatusBadRequest) | ||
| } | ||
| } | ||
| if len(q) > 0 { |
There was a problem hiding this comment.
len(q) > 0 does not really map to // Enable pagination if either or both of the query param exists.. Maybe it is better to define an explicit variable like enablePagination.
There was a problem hiding this comment.
If query exists, we are validating it above. If its validate, it would be related to pagination as of current implementation. But I think enablePagination would make it more readable too. Will fix it.
There was a problem hiding this comment.
Maybe len(opts) would be good check.
build-index/tagserver/server.go
Outdated
| opts = append(opts, backend.ListWithPagination()) | ||
| } | ||
|
|
||
| return &opts, nil |
There was a problem hiding this comment.
why returning reference?
There was a problem hiding this comment.
Avoiding slice header allocation.
There was a problem hiding this comment.
hmm i dont think we really worry about slice allocation here...I wouldnt return a pointer to a list unless it is really necessary. Also, if you return just []backend.ListOption, then you don't need to check nil in the caller above^.
build-index/tagserver/server.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if opts == nil { |
There was a problem hiding this comment.
i wonder if client.List(path.Join(repo, "_manifests/tags"), opts...) should handle the case when opts is empty...
There was a problem hiding this comment.
build-index/tagserver/server.go
Outdated
| return nil, handler.Errorf( | ||
| "invalid query %s:%s", k, v).Status(http.StatusBadRequest) | ||
| } | ||
| if k == limitQ { |
There was a problem hiding this comment.
i think it would be slightly more readable.
build-index/tagserver/server.go
Outdated
| opts = append(opts, backend.ListWithPagination()) | ||
| } | ||
|
|
||
| return &opts, nil |
There was a problem hiding this comment.
hmm i dont think we really worry about slice allocation here...I wouldnt return a pointer to a list unless it is really necessary. Also, if you return just []backend.ListOption, then you don't need to check nil in the caller above^.
build-index/tagserver/server.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if opts == nil { | ||
| return handler.Errorf("internal error") |
There was a problem hiding this comment.
return something more meaningful here?
+ Support pagination in list and listRespositories handler
+ Fix bug in gcsbackend pagination
+ Response for list with pagination
type ListResponse struct {
Links struct {
Next string `json:"next"`
Self string `json:"self"`
}
Size int `json:"size"`
Result []string `json:"result"`
}
type ListResponse struct {
Links struct {
Next string
json:"next"Self string
json:"self"}
Size int
json:"size"Result []string
json:"result"}
{
"Links": {
"next": "/list/?limit=3&offset=ClEvdGVzdC1idWNrZXQva3Jha2VuL2RlZmF1bHQvZG9ja2VyL3JlZ2lzdHJ5L3YyL2Jsb2JzL3NoYTI1Ni9hbC9hbHBpbmU6bGF0ZXN0L2RhdGE%3D",
"self": "/list/?limit=3&offset=CoQBL3Rlc3QtYnVja2V0L2tyYWtlbi9kZWZhdWx0L2RvY2tlci9yZWdpc3RyeS92Mi9ibG9icy9zaGEyNTYvNTcvNTczMzRjNTA5NTlmMjZjZTFlZTAyNWQwOGYxMzZjMjI5MmMxMjhmODRlN2IyMjlkMWIwZGE1ZGFjODllOTg2Ni9kYXRh"
},
"size": 3,
"result": [
"/test-bucket/kraken/default/docker/registry/v2/blobs/sha256/92/921b31ab772b38172fd9f942a40fae6db24decbd6706f67836260d47a72baab5/data",
"/test-bucket/kraken/default/docker/registry/v2/blobs/sha256/97/97a042bf09f1bf78c8cf3dcebef94614f2b95fa2f988a5c07314031bc2570c7a/data",
"/test-bucket/kraken/default/docker/registry/v2/blobs/sha256/al/alpine:latest/data"
]
}