-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass headers to custom error backend #275
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one nit about variable name
["X-Code"] = status or "404", | ||
["X-Format"] = ngx.var.httpAccept or "html", | ||
} | ||
headers = headers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the variable names are a little confusing, can you name one of the "headers" something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Changed.
local random_backend = get_destination() | ||
local res, err = httpc:request_uri(random_backend, { | ||
path = "/", | ||
method = "GET", | ||
headers = { | ||
["X-Code"] = status or "404", | ||
["X-Format"] = ngx.var.httpAccept or "html", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for my own education, can you clarify how ngx.var.httpAccept was inadequate in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing. The idea of this PR is to pass all the headers from the original request to the custom error backend and use the original Accept header.
78333f8
to
3551525
Compare
LGTM pending travis, feel free to merge once that's done |
Coverage decreased (-0.04%) to 45.963% when pulling 3551525e1cf30826c722b31f96a88474d2594cfc on aledbf:pass-headers into 1f70373 on kubernetes:master. |
Coverage remained the same at 46.002% when pulling 3551525e1cf30826c722b31f96a88474d2594cfc on aledbf:pass-headers into 1f70373 on kubernetes:master. |
@bprashanth ok, I am still testing :) |
Coverage remained the same at 46.002% when pulling 4245d2a58618fca7b6ca206ea504d4e50191b216 on aledbf:pass-headers into 1f70373 on kubernetes:master. |
fixes #273