Semantic Status Code - modify on_request response from 200 to 204 in plugin/in_http #2640
Conversation
Signed-off-by: Chin <[email protected]> 1. Modify response status code in in_http 2. Add test_response_without_empty_img in test_in_http 3. Fix typo of test_response_with_empty_img in test_in_http
There was a problem hiding this comment.
https://tools.ietf.org/html/rfc7231#section-6.3.1
a 200 response always has a payload,
though an origin server MAY generate a payload body of zero length.
If no payload is desired, an origin server ought to send 204 (No
Content) instead
Right. According to RFC, 204 seems better than 200 when sending no payload. LGTM.
But this is breaking change, so it may be better just changing a success message to the not empty payload(e.g. "200 OK") to avoid breaking change.
@repeatedly What do you think of this?
|
Yeah, we can't judge this change is safe for existing users.
|
Signed-off-by: Chin <[email protected]> 1. Add use_204_response flag to avoid breaking change happened in in_http 2. Modify test to keep testing the edge
|
Thanks for review. |
lib/fluent/plugin/in_http.rb
Outdated
| @@ -140,7 +140,7 @@ def close | |||
| super | |||
| end | |||
|
|
|||
| def on_request(path_info, params) | |||
| def on_request(path_info, params, use_204_response=false) | |||
There was a problem hiding this comment.
How do users set use_204_response?
There was a problem hiding this comment.
Oops, I think that is my misunderstanding.
I fixed it and add config_param in in_http.
This is my first time to PR with Ruby.
let me know if anything not suitable.
Thanks for review.
Signed-off-by: chin <[email protected]> 1.Add config_param use_204_response in in_http 2.Add test_response_use_204_response in test_in_http
|
Thanks! |
Semantic Status Code - modify on_request response from 200 to 204 in plugin/in_http Signed-off-by: Masahiro Nakagawa <[email protected]>
The reason about this PR:
I used fluentd service and figured that 200 response with empty string is a little bit not precise.
According to rfc7231, 204 is more proper status code.
What this PR does / why we need it: