Dashboard: API Endpoints must return 204 when they return no content on success

[ncskier]

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:

if (response.ok) {
  return response.status === 204 ? {} : response.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.

[vtereso]

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?

[ncskier]

Thanks @vtereso, I wasn't aware of the routes_test.go; those tests will have to be updated under this issue.
To clarify, I was making a generalization above, that the front-end requires a 204 status any time there is no content. So, if PUT requests already adhere to this, then that is great. However, I think the main problem is the POST operations that create resources and returning 201 when successful. The problem is that these POST operations return no content with a 201 status, so the front-end complains with an error.

[vtereso]

I believe https://github.com/tektoncd/dashboard/issues/18 outlined the best practice to have POST requests return 201 and since PUTs modify resources in place (we don't allow create on PUTs) there isn't really a necessity to have a content-location header so 204 followed by 200 seems the best practice at least according to the spec. Not sure if we want to have the front end match the current state of backend or? @mnuttall @a-roberts

[ncskier]

@AlanGreene what are your thoughts? We talked offline about using status code 204's, but maybe we should have an open discussion about this?

[AlanGreene]

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.

[ncskier]

Ok. That will also affect issues https://github.com/tektoncd/dashboard/issues/108 and https://github.com/tektoncd/dashboard/issues/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 https://github.com/tektoncd/dashboard/issues/75?

[AlanGreene]

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.

[jessm12]

I can create a PR with just that small change now 🙂

[jessm12]

https://github.com/tektoncd/dashboard/pull/153 PR here, though seeing what Alan mentioned here https://tektoncd.slack.com/archives/CJ62C1555/p1558022103266500 on running the tests

[ncskier]

@jessm12 are we able to close this issue now that the PR's merged? 👍

[jessm12]

/close

[tekton-robot]

@jessm12: Closing this issue.

In response to this:

/close

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.