Use status code 204 No Content for empty responses

tobikris commented

2022-10-16 22:42:58 +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

lx commented

2022-10-17 07:37:31 +00:00

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

tobikris commented

2022-10-17 09:00:29 +00:00

Author

Contributor

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

maximilien commented

2022-10-17 20:39:04 +00:00

Owner

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

lx commented

2022-10-18 08:30:06 +00:00

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

continuous-integration/drone/pr Build is passing Details

f1c96d108c

update k2v docs for status 204 changes

tobikris commented

2022-10-18 11:52:34 +00:00

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?