thoth-station / thamos

A CLI tool and library for communicating with Thoth
http://thoth-station.ninja
GNU General Public License v3.0
15 stars 17 forks source link

`thamos images` errors #1184

Closed goern closed 1 year ago

goern commented 2 years ago

thamos images throws an error:

/kind bug

codificat commented 2 years ago

thamos advise

is it thamos advise or thamos images (mentioned in the title)?

An example command invocation (and config?) that reproduces the problem would help

throws an error:

which error?

Can we have some more context?

codificat commented 2 years ago

/triage needs-information

goern commented 2 years ago

it is images:

2022-10-10 13:57:00,994 [1289268] ERROR    thamos: (404)
Reason: NOT FOUND
HTTP response headers: HTTPHeaderDict({'server': 'gunicorn', 'date': 'Mon, 10 Oct 2022 11:57:00 GMT', 'content-type': 'application/json', 'content-length': '314', 'x-thoth-version': '0.35.8', 'x-user-api-service-version': '0.35.8+messaging.0.16.2.storages.0.73.3.common.0.36.4.python.0.16.10', 'x-thoth-search-ui-url': 'https://thoth-station.ninja/search/', 'access-control-allow-origin': '*', 'set-cookie': '99770cb82864be05282857f803e02327=66c8c60f3ea5d397db93d125852ede8c; path=/; HttpOnly; Secure; SameSite=None', 'cache-control': 'private'})
HTTP response body: b'{\n  "error": "No Image found",\n  "parameters": {\n    "cuda_version": null,\n    "image_name": null,\n    "library_name": null,\n    "os_name": null,\n    "os_version": null,\n    "package_name": null,\n    "page": 3,\n    "per_page": 25,\n    "python_version": null,\n    "rpm_package_name": null,\n    "symbol": null\n  }\n}\n'
codificat commented 2 years ago

The bug is thamos not handling pagination and 404/NotFound gracefully enough.

Note the request is for page 3 when there is no page 3:

$ http --headers 'https://khemenu.thoth-station.ninja/api/v1/container-images?page=0&per_page=25' | grep count
page_count: 2
entries_count: 41

/transfer thamos

The user API started returning a 404 in this situation starting in v0.35.6 after this change: https://github.com/thoth-station/user-api/commit/ab32cad162d93e61454e5629c5b6ee2efd417510#diff-0c1d9c73b678717b9143917cfc52d1fccbfcdf488adb2375865c8adca9a27a6eR376-R379

/remove-triage needs-information /triage accepted /priority critical-urgent /sig user-experience

codificat commented 2 years ago

/sig user-experience

codificat commented 2 years ago

The user API started returning a 404 in this situation starting in v0.35.6

By the way, I am not sure if a 404 is appropriate here: maybe the API should just return a 200/OK with a total of 0 results, no?

Still, it sounds like thamos should handle pagination better by looking at the page count in the response headers

codificat commented 2 years ago

Related: #1109

goern commented 1 year ago

delivering a 404 sounds reasonable when no images are available, or when requesting page 5 with per_page 10 and a total of 8 images available. 404 is the server signaling, that the requrested data could not be found, giving 200 with an empty list if a a incorrect behavior, isnt it?

goern commented 1 year ago

I tried to create an integration test covering this, see https://github.com/thoth-station/integration-tests/pull/351

codificat commented 1 year ago

delivering a 404 sounds reasonable when no images are available

Maybe. But:

or when requesting page 5 with per_page 10 and a total of 8 images available. 404 is the server signaling, that the requrested data could not be found,

That might better be answered by a 400 (bad request) instead? Or even a 200 with empty results (meaning "this is what I found: nothing").

Note that the API response includes a total item count and a page count in the response headers; these complement an empty 200 response nicely I think.

giving 200 with an empty list if a a incorrect behavior, isnt it?

IMO I think it is a better behavior when appropriate... but I can buy the 404 too, no too-hard feelings.

Gkrumbach07 commented 1 year ago

Acceptance Criteria

Gkrumbach07 commented 1 year ago

This is not a thamos issue but a user-api issue. The correct response code is 200 for an empty list. 404 would indicate that the collection of images does not exist.