feat(tools): HTTP Response 503 Service unavailable #2023
Conversation
Add HTTP response for 503 Service unavailable. This will be used for a new api-unavailable plugin that will send a 503 error. See #1279.
kong/tools/responses.lua
Outdated
| @@ -55,7 +56,8 @@ local _M = { | |||
| HTTP_METHOD_NOT_ALLOWED = 405, | |||
| HTTP_CONFLICT = 409, | |||
| HTTP_UNSUPPORTED_MEDIA_TYPE = 415, | |||
| HTTP_INTERNAL_SERVER_ERROR = 500 | |||
| HTTP_INTERNAL_SERVER_ERROR = 500, | |||
| HTTP_SERVICE_UNAVAILABLE = 503 | |||
There was a problem hiding this comment.
We need to add a trailing comma here so that future diffs on that table will be more pertinent.
There was a problem hiding this comment.
24 years of java programming has me not putting the trailing comma. I like the consistent diff idea where languages support it
kong/tools/responses.lua
Outdated
| @@ -96,7 +102,7 @@ local function send_response(status_code) | |||
| -- @param content (Optional) The content to send as a response. | |||
| -- @return ngx.exit (Exit current context) | |||
| return function(content, headers) | |||
| if status_code >= _M.status_codes.HTTP_INTERNAL_SERVER_ERROR then | |||
| if status_code >= _M.status_codes.HTTP_INTERNAL_SERVER_ERROR and status_code ~= _M.status_codes.HTTP_SERVICE_UNAVAILABLE then | |||
There was a problem hiding this comment.
style: let's not go over the 80 characters limit per line
There was a problem hiding this comment.
Additionally, I wonder if this behavior should actually be changed to only log when we send 500, instead of 5xx.
Added , to end of list Changed to only log 500 errors
|
Will do
Is there an easy way to just to run that test file?
…On Thu, Jan 26, 2017 at 23:06 Thibault Charbonnier ***@***.***> wrote:
***@***.**** requested changes on this pull request.
THanks for taking care of it.
Would you now mind adding the necessary tests to
spec/01-unit/10-responses_spec.lua? Namely, that requests are not logged
on all 5xx status codes and that the new "Service Unavailable" one is
working as expected?
Thanks
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2023 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAbMYdjCnd0I7qkU5UinGzU5-PTTor8Kks5rWSbhgaJpZM4LvKEy>
.
|
|
|
kong/tools/responses.lua
Outdated
| @@ -83,6 +86,9 @@ local response_default_content = { | |||
| end, | |||
| [_M.status_codes.HTTP_METHOD_NOT_ALLOWED] = function(content) | |||
| return "Method not allowed" | |||
| end, | |||
| [_M.status_codes.HTTP_SERVICE_UNAVAILABLE] = function(content) | |||
| return content or "Service Unavailable" | |||
There was a problem hiding this comment.
nitpick; align capitalization, single capital. eg. "Service unavailable"
thibaultcha
added
the
pr/ready (but hold merge)
|
Let's wait for the 0.10.0 to be final before merging, to reduce conflicts. And to refactor some other 503 references in |
|
This is why it hasn't been merged (although it could without much trouble).
… On Jan 31, 2017, at 6:38 AM, Thijs Schreijer ***@***.***> wrote:
Let's wait for the 0.10.0 to be final before merging, to reduce conflicts. And to refactor some other 503 references in /kong/core/balancer.lua (in the new release)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Do you want me to update this branch to change the existing send(503) to use the new send method? |
|
@pauldaustin only after the 0.10.0 release branch has been merged (that's where those references are). |
|
merged through #2328 closing this. @pauldaustin Thank you! |
Summary
Add HTTP response for 503 Service unavailable. This will be used for a
new api-unavailable plugin that will send a 503 error. See #1279.
Full changelog
Issues resolved
N/A