openservicebrokerapi / servicebroker

Open Service Broker API Specification
https://openservicebrokerapi.org/
Apache License 2.0
1.19k stars 436 forks source link

Add 4xx responses from spec to openapi.yaml and swagger.yaml #703

Closed treznick closed 4 years ago

treznick commented 4 years ago

What is the problem this PR solves? This Pull Request adds 404 and 422 as permitted responses to the openapi.yaml and swagger.yaml where contextually appropriate.

Specifically, it adds 422 as an acceptable response to unbinding requests in the openapi.yaml, as documented in the spec: https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#unbinding and as provided in the swagger.yaml: https://github.com/openservicebrokerapi/servicebroker/blob/master/swagger.yaml#L371-L372

Furthermore, it adds 404 as an acceptable response to the last_operation requests on service instances and service bindings. While not explicitly documented in the spec, 404 is an acceptable response for the parent URIs in question: /v2/service_instances/:instance_id => https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-4 /v2/service_instances/:instance_id/service_bindings/:binding_id => https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-7

Which suggests that these should also be permitted responses in the case of child responses.

Given that 9de42fd introduces a slight behavioral change in the API, I am happy to revert it if that is deemed appropriate.

Finally, it adds 401 Unauthorized as an acceptable response in the openapi.yaml. This is a permitted response per the spec and the swagger.yaml.

Checklist:

linux-foundation-easycla[bot] commented 4 years ago

CLA Check
The committers are authorized under a signed CLA.

mattmcneeney commented 4 years ago

Thanks for this @treznick!

If we're adding 404s as an acceptable response for last operation calls in the openapi docs, we should probably add them to spec.md too in the appropriate response tables (here and here). I personally don't have any problems with adding 404 responses to those endpoints - @fmui @tinygrasshopper any thoughts?

Let me know if you have any trouble signing the CLA @treznick

treznick commented 4 years ago

@mattmcneeney You're very welcome!

I added 404s as acceptable responses to the tables you mentioned in this commit: 409d097

As for the CLA I am working with legal at my employer to figure out the best way to sign (and whether I can sign as an individual contributor). I will keep you all apprised.

mattmcneeney commented 4 years ago

I can confirm that this change won’t break anything in CF, although the error message may suggest that the broker has responded with an invalid code unless we add support for this.

mattmcneeney commented 4 years ago

Closing and reopening to retrigger Travis

lyubo1 commented 4 years ago

@mattmcneeney CLA should be signed, can we re-run the checks here? /cc @treznick

mattmcneeney commented 4 years ago

Hey @lyubo1 - It still doesn't seem to be working. I tried closing and reopening which usually triggers the check but it still says it's missing

treznick commented 4 years ago

@mattmcneeney @lyubo1 I have signed the CLA. Please let me know if you need anything further.

mattmcneeney commented 4 years ago

Perfect, thanks @treznick I can see it's all green now. Time for some reviews! Ping @openservicebrokerapi/osbapi-pmc

mattmcneeney commented 4 years ago

ping @fmui