Use status code 204 No Content for empty responses #403

Merged
lx merged 2 commits from tobikris/garage:http-no-content into main 2022-10-18 14:20:44 +00:00
Contributor

I picked the starter issue #376 to get familiar with the repo.
I successfully ran the smoke-tests (except winscp because I am running on Linux).

When replacing one occurence of an empty body and status 200 with status 204 the minio tests fail - I don't understand why this is the case but left it at 200 therefore.

I picked the starter issue #376 to get familiar with the repo. I successfully ran the smoke-tests (except winscp because I am running on Linux). When replacing [one occurence](https://git.deuxfleurs.fr/tobikris/garage/src/commit/2c3dab77168b72883296431a2bab37448851ffd7/src/api/s3/get.rs#L174) of an empty body and status 200 with status 204 the minio tests fail - I don't understand why this is the case but left it at 200 therefore.
tobikris force-pushed http-no-content from 2c3dab7716 to 9b57d5b4c2 2022-10-16 22:44:20 +00:00 Compare
Owner

Sorry, I think the issue description was quite misleading, but in fact S3 API endpoints are NOT concerned by this issue and should NOT be changed.

Issue #376 concerns only the Admin API, which is an API that we designed ourselves, and where we can decide freely what HTTP return codes we use. For the S3 API we have no freedom, as we have to mimic exactly what AWS does. As you saw, changing the return codes on S3 endpoints causes tests to fail, so they should stay as they are because that's the correct behavior to conform to the S3 spec.

Sorry, I think the issue description was quite misleading, but in fact S3 API endpoints are NOT concerned by this issue and should NOT be changed. Issue #376 concerns only the Admin API, which is an API that we designed ourselves, and where we can decide freely what HTTP return codes we use. For the S3 API we have no freedom, as we have to mimic exactly what AWS does. As you saw, changing the return codes on S3 endpoints causes tests to fail, so they should stay as they are because that's the correct behavior to conform to the S3 spec.
tobikris force-pushed http-no-content from 9b57d5b4c2 to 1e3369820e 2022-10-17 08:51:23 +00:00 Compare
tobikris force-pushed http-no-content from 1e3369820e to 12b058e073 2022-10-17 08:54:36 +00:00 Compare
tobikris force-pushed http-no-content from 12b058e073 to 7865003323 2022-10-17 08:56:58 +00:00 Compare
Author
Contributor

Ok, I changed the PR to only touch the admin and k2v API. However, the CI seems to be broken currently :(

Ok, I changed the PR to only touch the admin and k2v API. However, the CI seems to be broken currently :(
Owner

Ci is now fixed, pesky issue with a runner not having access to Gitea

Ci is now fixed, pesky issue with a runner not having access to Gitea
Owner

Thanks for the PR, LGTM.

Do you think you could also update the documentation for the admin api and K2V api to indicate the new return status codes? They are respectively in doc/book/reference-manual/admin-api.md and doc/drafts/k2v-spec.md

Thanks for the PR, LGTM. Do you think you could also update the documentation for the admin api and K2V api to indicate the new return status codes? They are respectively in `doc/book/reference-manual/admin-api.md` and `doc/drafts/k2v-spec.md`
tobikris added 1 commit 2022-10-18 11:51:07 +00:00
update k2v docs for status 204 changes
All checks were successful
continuous-integration/drone/pr Build is passing
f1c96d108c
Author
Contributor

I have added the documentation changes about k2v. The admin-api docs however do not contain references to specific return codes. Is it therefore ok like this?

I have added the documentation changes about k2v. The admin-api docs however do not contain references to specific return codes. Is it therefore ok like this?
Owner

Indeed, there is nothing to update in the admin API documentation. Thanks for the work!

Fix #376

ping @quentin: your openapi specs will need updating

Indeed, there is nothing to update in the admin API documentation. Thanks for the work! Fix #376 ping @quentin: your openapi specs will need updating
lx merged commit 5670599372 into main 2022-10-18 14:20:44 +00:00
tobikris deleted branch http-no-content 2022-10-18 16:51:13 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/garage#403
No description provided.