search

openservicebrokerapi / servicebroker

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

Rename ServiceInstanceProvision to ServiceInstance #672

Closed leonwanghui closed 5 years ago

leonwanghui commented 5 years ago

What is the problem this PR solves? When I use the OpenAPITools tool to auto-generate Golang code from swagger.yaml (and openapi.yaml), it seems that the ServiceInstanceProvision defined is conflict with the generated ServiceInstanceProvision method. So this PR is proposed to rename ServiceInstanceProvision to ServiceInstanceProvisionResponse so as to avoid the name confusion. Besides, to keep all name consistent, ServiceBinding field would also been renamed to ServiceBindingResponse.

Checklist:

cfdreddbot commented 5 years ago

:white_check_mark: Hey leonwanghui! The commit authors and yourself have already signed the CLA.

Samze commented 5 years ago

I notice we already have:

    ServiceInstanceResource:
      type: object
      properties:
        service_id:
          type: string
        plan_id:
          type: string
        dashboard_url:
          type: string
        parameters:
          $ref: '#/components/schemas/Object'

I think that doing this rename may be confusing since we would have both a ServiceInstance object and a ServiceInstanceResource object. Really the ServiceInstance schema is specific to the provision operation.

However I notice that there is a ServiceBinding and ServiceBindingResource.

I would be in favour for fixing both to be more explicit. Something like ServiceInstanceProvisionResponse.

leonwanghui commented 5 years ago

@Samze I agree with your point, we need to make them all consistent.

leonwanghui commented 5 years ago

@Samze @mattmcneeney @fmui PTAL, thanks!

tinygrasshopper commented 5 years ago

I think the change makes sense, but it's a 'breaking' change for folks who are generating clients using the swagger spec. Do we need to add the breaking nature of this change to the readme

mattmcneeney commented 5 years ago

Good point @tinygrasshopper. My guess is that there are very few folks doing that, but maybe we should try to advertise this change (emailing the mailing list?) just in case.