openservicebrokerapi / servicebroker

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

Add updateable field to plan object #595

Closed jberkhahn closed 6 years ago

jberkhahn commented 6 years ago

per #582

cfdreddbot commented 6 years ago

Hey jberkhahn!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

Samze commented 6 years ago

What is the semantic relationship between the service level updatable field and plan level updatable field? Does a service updatable effect the plan updatable. For example is it valid to have service updatable false and plan updatable true?

duglin commented 6 years ago

I'm leaning towards saying that the plan can be "false" if the service is "true", but the plan can not be "true" if the service is "false". ie. a plan can be more restrictive than the service, but not less.

duglin commented 6 years ago

I take that back... Russell just pointed out to me that "bindable" for services and plans allows for a plan to override the service regardless of the service's value. So we should probably be consistent with that.

jberkhahn commented 6 years ago

So leave it as is then? Independent of the service offering's field?

rboykin commented 6 years ago

@jberkhahn @duglin I suggest adding in the default/precedence clauses like are done in the services.bindable and the plan.bindable fields. For services.bindable:

This specifies the default for all plans of this service. Plans can override this field (see Plan Object).

For plan.bindable (changing bindable to plan_updateable):

This field is OPTIONAL. If specified, this takes precedence over the `bindable` attribute of the service. If not specified, the default is derived from the service. 
jberkhahn commented 6 years ago

borrowed the wording from bindable per Russell's suggestion

jberkhahn commented 6 years ago

Fixed

Samze commented 6 years ago

I wonder if having plan_updateable on the service offering and updateable on the service plan is confusing?

Perhaps we should just use plan_updateable in both places for consistency?

tinygrasshopper commented 6 years ago

There is some text in the Updating a Service Instance section which talks about enabling support for updating the service instance(To enable support for the update of the plan, a Service Broker MUST declare support per service by including "plan_updateable": true in its catalog endpoint.) I think that needs to be enhanced with the implications of the updateable flag on the plan.

mattmcneeney commented 6 years ago

@jberkhahn to address comments and then we will re-review (hopefully asynchronously before next week).

jberkhahn commented 6 years ago

Updated

duglin commented 6 years ago

LGTM

Approved with PullApprove

jberkhahn commented 6 years ago

Added the link.

duglin commented 6 years ago

LGTM

Approved with PullApprove