-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
admin: fix warnings and status code in /load API #7267
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
base: master
Are you sure you want to change the base?
admin: fix warnings and status code in /load API #7267
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.
Ah, thank you. Although, we should still probably return caddy.APIError
if possible?
I agree. I'll update it so warnings also return caddy.APIError, to make sure error responses follow the same format. |
I moved Warning into a separate package so it can be reused in |
This breaks every single config adapter in the ecosystem. That's more than 1. We can't break that many. I wonder if Go type aliases may be helpful in this instance though. |
b7f1368
to
106f1ec
Compare
04ac46e
to
39ace45
Compare
Did you mean to do that?? |
@mohammed90 Thanks for the suggestion, it was really helpful. I updated the code accordingly. |
I think it would make sense to have another response type for success responses with warnings. check these lines: I can add this as well if that’s okay. |
Description
This PR fixes #7246
where the Admin API /load endpoint returned 200 OK instead of 400 Bad Request when an invalid Caddyfile also produced adapter warnings.
The problem happened because the warnings were written to the response first, which made http.ResponseWriter default to 200 OK. As a result, the response contained two separate JSON objects (warnings + error) stuck together, which was invalid JSON.
Changes
400 Bad Request
) is returned for invalid configs.Behavior Before
Behavior After