redirect: allow to specify redirect response code#217
Conversation
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
api/rds.proto
Outdated
| } | ||
| // The HTTP status code to use in the redirect response. The default response | ||
| // code is MOVED_PERMANENTLY (301). | ||
| HTTPResponseCode response_code = 3; |
There was a problem hiding this comment.
IIRC proto will default this to 0, not 301. @htuch would this be covered by the validator?
There was a problem hiding this comment.
The tests are complaining about the enum needing to start at 0. api/rds.proto: The first enum value must be zero in proto3. I'll change the enums to start at 0.
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM. Let's wait till tomorrow to see if anyone else has any comments.
| // Moved Permanently HTTP Status Code - 301. | ||
| MOVED_PERMANENTLY = 0; | ||
| // Found HTTP Status Code - 302. | ||
| FOUND = 1; |
There was a problem hiding this comment.
May be add 304 as well? From this (https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_Redirection), 304 seems to be the one that might be useful in future.
There was a problem hiding this comment.
Added 303 and 304 to keep the Status codes in order.
There was a problem hiding this comment.
Added 303 and 304 to keep the order.
There was a problem hiding this comment.
304 Not Modified definitely shouldn't be one this list, could you please remove it?
On the other hand, 307 Temporary Redirect and 308 Permanent Redirect are definitely worth adding.
There was a problem hiding this comment.
Yes good catch, I missed this. @ccaraman please fix.
| NONE = 0; | ||
| // External requests must use TLS. If a request is external and it is not | ||
| // using TLS, a 302 redirect will be sent telling the client to use HTTPS. | ||
| // using TLS, a 301 redirect will be sent telling the client to use HTTPS. |
rshriram
left a comment
rshriram
left a comment
There was a problem hiding this comment.
add 304 to list of options if possible.
api/rds.proto
Outdated
| // The path portion of the URL will be swapped with this value. | ||
| string path_redirect = 2; | ||
|
|
||
| enum HTTPResponseCode { |
There was a problem hiding this comment.
From the perspective of documentation generation, this is going to appear a bit confusing on the webpage. Suggest renaming to RedirectResponseCode or something to that effect, so as to disambiguate.
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
5 participants
Description: