-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Improve search method #99
base: master
Are you sure you want to change the base?
Conversation
2733a7d
to
4746097
Compare
Heh, didn't you add the spec in? 😁 |
The prior behavior fetched them all? It looks like this adds not just an option to limit, but a default limit that did not exist before? Is that a breaking change? |
the default is 100 entries per page. We could for sure make the "n" optional and if nothing gets passed it gets the /_catalog as before. I will add a spec too to make sure that the n influences the number of returned entries per page Reference: https://distribution.github.io/distribution/spec/api/ |
So you are saying that the default is 100. This PR just makes it explicit, but that is what happens anyways? I approved it, just waiting for the spec you said you wanted to add. |
Exactly, the default page size for the docker official registry is 100. I will add a spec for that. Maybe we should keep the query to the "_catalog" in case people are using a different registry backend and are expecting other value there.
spec will come 👍 |
Hi @deitch While running the local registry with Anyway with the commands:
I was able to convert the manifest from v1 to v2 |
I created some images locally to test the "/v2/_catalog" with the following script (that I will add to the PR)
Then to make sure that we have more than 100 images in the catalog:
Lets check the default of calling
So like that I'm sure the default are 100 results. I will now add the spec to make sure that we can control this value, i.e having 3 images named |
361507c
to
65717e0
Compare
65717e0
to
260adcc
Compare
@@ -11,8 +11,8 @@ def self.connect(uri = 'https://registry.hub.docker.com', opts = {}) | |||
@reg = DockerRegistry2::Registry.new(uri, opts) | |||
end | |||
|
|||
def self.search(query = '') | |||
@reg.search(query) | |||
def self.search(query = '', records = 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a bit strange that here the params are query
and records
, but in reg.search()
they are query
and record_count
? Should we not be consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i will align them 👍 .. any preferences? records
or records_count
?
@@ -0,0 +1,21 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice utility, but I don't think it belongs in the root, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, i will move it to a better place
Hello @deitch
I was working in a different project, where we tune the query to the "_catalog" endpoint to get more or less records per page. So I then imagined that we could have the same in the docker_registry2 gem.
So the default is 100 records per page, so thats what i set on the
search
method. I "precompiled" the regex used in the search method, matching the query, outside of the each loop, since it doesnt change.Lets see if it works 👍
P.S: Ops it looks like I have to regenerate the VCR files. Nice CI pipeline you have here 😊