Skip to content
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

enable search using pagination #964

Merged
merged 1 commit into from
Jun 26, 2020
Merged

Conversation

QiWang19
Copy link
Collaborator

Enable search registry specifying the number of results rather than using default API 100 limit.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1827794

Signed-off-by: Qi Wang [email protected]

Path: "/v2/_catalog",
}
q := u.Query()
q.Set("n", strconv.Itoa(limit))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does limit get set to 100 if not specified by the user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The limit parameter is supposed to be a limit on the number of results, not on the number of values that will be checked for matches.
  • The linked bug asks for support of pagination, not just for controlling the number of results.

@QiWang19
Copy link
Collaborator Author

The linked bug expected to receive the total result if the total number of results when there are over 100.
To support the pagination, the podman will provide a page size argument and with the current limit for total number of results. These enable to podman search to return the paginated result and this is the functionality that should be provided. Did I get it right?

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 18, 2020

No; “pagination” means that a single HTTP request only returns up to N results (a “page”), and the client must make another HTTP request for another page, and yet another for the third page, and so on.

See https://github.com/docker/distribution/blob/master/docs/spec/api.md#pagination , and GetRepositoryTags for an example.

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 18, 2020

(The page size is ultimately decided by the server, which just won’t return larger pages than some internally-decided number. It doesn’t need to be a number specified by Podman, and especially users should not have to care.)

@rhatdan
Copy link
Member

rhatdan commented Jun 19, 2020

If the user cares about page size then they can pipe it to less.

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 19, 2020

The “page” size from the API is completely unrelated to the end-user UI, first of all because the “pages” apply to the full catalog but we only return results of the search in that catalog.

@QiWang19 QiWang19 changed the title enable search using results limit enable search using pagination Jun 23, 2020
@QiWang19 QiWang19 force-pushed the search-limit branch 2 times, most recently from b521ffe to 5f809f9 Compare June 23, 2020 21:44
@QiWang19
Copy link
Collaborator Author

@mtrmac @vrothberg @TomSweeneyRedHat @rhatdan PTAL. This uses the pagination until the search result reaches the limit.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one nit.

docker/docker_client.go Outdated Show resolved Hide resolved
docker/docker_client.go Show resolved Hide resolved
docker/docker_client.go Show resolved Hide resolved
Enable search registry uses the pagination until the search result reaches the limit, instead of returning default 100 limit from registry API.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1827794

Signed-off-by: Qi Wang <[email protected]>
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mtrmac mtrmac merged commit 4c0f8a7 into containers:master Jun 26, 2020
QiWang19 added a commit to QiWang19/image that referenced this pull request Aug 11, 2020
QiWang19 added a commit to QiWang19/image that referenced this pull request Aug 12, 2020
Backports containers#964 to v5.1
Enable search registry uses the pagination until the search result reaches the limit, instead of returning default 100 limit from registry API.
BZ:  https://bugzilla.redhat.com/show_bug.cgi?id=1866153

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Aug 12, 2020
Backports containers#964 to v5.5.1
Enable search registry uses the pagination until the search result reaches the limit, instead of returning default 100 limit from registry API.
BZ:  https://bugzilla.redhat.com/show_bug.cgi?id=1866153

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Aug 13, 2020
Backports pagination fix containers#964 to release-5.5
Enable search registry uses the pagination until the search result reaches the limit, instead of returning default 100 limit from registry API.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1866153

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Aug 13, 2020
Backports pagination fix containers#964 to release-5.5
Use skopeo branch release-1.1
Enable search registry uses the pagination until the search result reaches the limit, instead of returning default 100 limit from registry API.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1866153

Signed-off-by: Qi Wang <[email protected]>
mtrmac pushed a commit to QiWang19/image that referenced this pull request Aug 17, 2020
Backports pagination fix containers#964 to release-5.5
Use skopeo branch release-1.1
Enable search registry uses the pagination until the search result reaches the limit, instead of returning default 100 limit from registry API.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1866153

Signed-off-by: Qi Wang <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants