chore(#8179) Add json error messages to haproxy - #8481
Conversation
|
Targeting branch in PR #8446 as it builds on changes from there. Will update target if that PR gets merged first. Please let me know if you have another preferred workflow for PRs that builds on other PRs |
That process is fine. I did get a bit confused for a second but worked it out. |
garethbowen
left a comment
garethbowen
left a comment
There was a problem hiding this comment.
Great work, thanks! Some thoughts inline.
| # nginx_error_messages | ||
|
|
||
| This directory contains error pages for haproxy. Do not update the error pages | ||
| manually, instead look in `generate.sh` and the `template.json` file |
There was a problem hiding this comment.
In general I prefer generated code to not be checked in to the repo, and instead generated in CI and published. This means someone doesn't accidentally manually update the output and saves on duplication. What do you think? Is there an advantage to checking in the build output?
There was a problem hiding this comment.
I can see arguments on both sides. The main reason I went with the more explicit pre-generated files is that I expect these will change at most 1-2 times per year and I didn't want to add complexity to the general build process. I imagine debugging an issue with the generated files without having been involved in this PR be more difficult if they're generated on build/start.
What if I add a test to ensure the current files are equal to how they will look when running generate.sh? I prefer that over build step as it doesn't complicate the build process and can be run independently from the build. I added a suggestion to what that could look like in this PR in the file haproxy/tests/errors_generate.bats feel free to review that one.
| timeout http-keep-alive 5m | ||
|
|
||
| errorfiles json | ||
|
|
There was a problem hiding this comment.
If I understand this correctly, this will always respond with JSON regardless of the Accepts header?
There was a problem hiding this comment.
Correct, since HAProxy is not being called directly by browsers or mobile apps adding a conditional can be considered unnecessary logic (feel free to discuss further in #8179)
|
|
||
| echo " errorfile $status_code /usr/local/etc/haproxy/errors/${status_code}-json.http" | ||
|
|
||
| done |
garethbowen
left a comment
garethbowen
left a comment
There was a problem hiding this comment.
Looks great! Thanks.
2 participants
Description
For the HAProxy part of #8179 - nginx and haproxy should respect
Acceptheaders when responding with error templatesCode review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.