-
Notifications
You must be signed in to change notification settings - Fork 260
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
API Endpoints must return 204 when they return no content on success #146
Comments
Currently, there is https://github.com/tektoncd/dashboard/blob/master/pkg/endpoints/routes_test.go#L122, that ensures all registered PUT routes return 204. These are the only routes that should return 204 atm IIRC. Is there another case that is not covered? |
Thanks @vtereso, I wasn't aware of the |
I believe #18 outlined the best practice to have POST requests return 201 and since |
@AlanGreene what are your thoughts? We talked offline about using status code 204's, but maybe we should have an open discussion about this? |
I discussed this with @jessm12 yesterday and provided her with an approach to handle these correctly in the front end code. I believe she's implementing that change to handle the 201 response from the POST APIs, including providing access to the response headers. 200 / 204 will continue to be handled as they are already. |
Ok. That will also affect issues #108 and #67 for front-end paneling that involves POST requests with 201 return codes (FYI @carlos-logro). @AlanGreene can we close this issue, and we will use @jessm12's fix which will be introduced with her PR for issue #75? |
If @jessm12 wants to break off the change for the response handling into a separate PR I'd be more than happy to merge that to unblock anyone working on other tasks consuming POST APIs. |
I can create a PR with just that small change now 🙂 |
#153 PR here, though seeing what Alan mentioned here https://tektoncd.slack.com/archives/CJ62C1555/p1558022103266500 on running the tests |
@jessm12 are we able to close this issue now that the PR's merged? 👍 |
/close |
@jessm12: Closing this issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The front-end expects that if it receives any success HTTP code other than 204, then the response has content. If a response has no content and a code other than 204, the front-end will throw an error trying to parse the response as a JSON:
(https://github.com/tektoncd/dashboard/blob/master/src/api/comms.js#L28)
So, we must update all API endpoints to return code 204 when they succeed and return no content.
The text was updated successfully, but these errors were encountered: