search

heroku / docker-registry-client

A Go API client for the v2 Docker Registry API
BSD 3-Clause "New" or "Revised" License
297 stars 225 forks source link

Follow Link header in tags/list responses #11

Closed squaremo closed 8 years ago

squaremo commented 8 years ago

In the Docker registry API v2 specification, the response to a list tags call may be paginated by the server. It does this using an RFC5988 Link header, with rel=next; no header means no more pages.

At least quay.io uses this mechanism to return pages of 50 tags at a time, though it gets the format slightly wrong (by missing the < and > around the URL).

This PR makes the registry.Tags method deal with pagination, by following the Link header when it's present. I've used a regexp to parse the Link header; it's not exactly correct, but probably good enough in practice.

squaremo commented 8 years ago

Now uses a sentinel error to indicate that there are no more pages to request.

dmathieu commented 8 years ago

Thank you. Two additional notes:

squaremo commented 8 years ago

Could you add tests?

How do I say this .. there were no tests present, so I didn't add any. I'm happy to add tests, given some scaffolding, but I'm less enthused about creating the scaffolding. Is that fair?

I merged repositories earlier today: d61a28e. I'm thinking this endpoint should get the same pagination.

OK.

dmathieu commented 8 years ago

How do I say this .. there were no tests present, so I didn't add any. I'm happy to add tests, given some scaffolding, but I'm less enthused about creating the scaffolding. Is that fair?

Argh my bad. I didn't write this project, and assumed there were tests in other places. No, please don't setup any scaffolding.

squaremo commented 8 years ago

Catalog responses also now follow the pagination.

squaremo commented 8 years ago

.. and some basic tests.

squaremo commented 8 years ago

I believe it'd be better to introduce this without tests

I'm not clear whether you want the tests or not, since you've reviewed them ...

dmathieu commented 8 years ago

Yes, I've reviewed the tests. But to speed up merging this PR, and since there isn't any tests setup anywhere else in that project, I believe shipping this without tests is fine.

squaremo commented 8 years ago

OKdokes. I think I've fixed the things pointed out in the review. This PR no longer includes the tests (they are in a branch squaremo/test-paginated-results; I've not acted on the review comments for that code).

dmathieu commented 8 years ago

Thank you! 🌷 I've just left a minor comment. I'm going to merge this and fix it.

squaremo commented 8 years ago

Great! Thanks for your patient reviewing :)

dmathieu commented 8 years ago

Thank you for your contribution :)