Catalog content discovery #2782
Conversation
eusebiu-constantin-petu-dbk
requested review from
rchincha and
andaaron
as code owners
eusebiu-constantin-petu-dbk
force-pushed
the
catalog_content_discovery
branch
3 times, most recently
from
e08544f to
6619b7b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2782 +/- ##
==========================================
- Coverage 91.93% 91.88% -0.06%
==========================================
Files 170 170
Lines 30126 30282 +156
==========================================
+ Hits 27697 27824 +127
- Misses 1806 1829 +23
- Partials 623 629 +6 ☔ View full report in Codecov by Sentry. |
|
I gave it a try and found some problem I think. I updated the original issue: #2715 (comment) and will take a deeper look. |
|
Also be mindful about access control - one may not have read access to all repos. |
eusebiu-constantin-petu-dbk
force-pushed
the
catalog_content_discovery
branch
from
6619b7b to
590a8ce
Compare
|
@uvegla Thank you very much for your work on this! |
eusebiu-constantin-petu-dbk
force-pushed
the
catalog_content_discovery
branch
8 times, most recently
from
5ae5b92 to
9302fc6
Compare
feat(api): added /v2/_catalog pagination, fixes project-zot#2715 Signed-off-by: Eusebiu Petu <[email protected]>
eusebiu-constantin-petu-dbk
force-pushed
the
catalog_content_discovery
branch
from
9302fc6 to
7afee3d
Compare
| sort.Strings(subPaths) | ||
|
|
||
| storePath := rh.c.StoreController.GetStorePath(lastEntry) | ||
| if storePath == storage.DefaultStorePath { |
There was a problem hiding this comment.
So the entries are not sorted by names? The default store entries are always first?
|
|
||
| subStore := rh.c.StoreController.SubStore | ||
|
|
||
| subPaths := make([]string, 0) |
There was a problem hiding this comment.
Can you add more comments? It's a bit hard to follow the logic of this function.
Are there assumptions being made about the order of the substores relative to the default store?
This spec https://github.com/openshift/docker-distribution/blob/master/docs/spec/api.md#pagination mentions "The catalog result set is represented abstractly as a lexically sorted list,"
How would the logic behave if I have a substore for folder /b/ and another for folder /d/?
Repos /a/ are supposed to be first from the default store followed from repos /b/ from the 1st substore, followed by repos /c/ from the default store, followed by repos /d/ from the 2nd substore, followed by repos /e/ from the default strore...
Of course you could have cases such as /aa/, /ab/, and other folders.
We may agree that in out zot implementation we don't care about lexical sorting, since _catalog is not part or the oci dist spec, but in this case let's make that clear in the code/comments/docs?
| img := CreateRandomImage() | ||
|
|
||
| repoNames := []string{ | ||
| "alpine1", "alpine2", "alpine3", |
There was a problem hiding this comment.
Can we mix them so the results from the default store and substores are not in alphabetical order?
Let's say substores with paths /a and /c, and images "alpine1", "alpine2", "alpine3", "a/alpine4", "a/alpine5", "a/alpine6", "b/alpine7", "b/alpine8", "b/alpine9", "c/alpine10", "c/alpine11", "c/alpine12", "d/alpine10", "d/alpine11", "d/alpine12". Also some entries not having a folder in path.
|
Tested locally, looks good to me now! 🙂 Thank you for working on this! 👍🏻 |
|
@eusebiu-constantin-petu-dbk can you take a look at @andaaron 's comments? |
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.