eruvanos / openbrokerapi

A python package for the V2 CF Service Broker API
https://openbrokerapi.readthedocs.io/
MIT License
35 stars 19 forks source link

Remove validation for organization_guid and space_guid #115

Closed rajahaidar closed 4 years ago

rajahaidar commented 4 years ago

Describe the bug https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#body-3 As per the above spec, the organization and space GUIDs will be deprecated in favor of context, this will prevent users that would like to use the new specification from from using it To Reproduce Steps to reproduce the behavior: Implement a broker and attempt to implement a broker with this library, and attempt to create a service instance without specifying an organization and space GUID. It will throw the error

Organization and space guid are required.

Expected behavior To conform to the spec, it should not throw that error and make those fields optional

Additional context Using Flask to develop a python application

redorff commented 4 years ago

As the two fields _organizationguid and _spaceguid have the asterisk, and as the documentation states that Fields with an asterisk are REQUIRED, my understanding is that they are still required even though there is a warning that they will be deprecated in a later version.

Another loosening option could be to still accept provision requests with those fields within the context field. This is probably a good idea.

rajahaidar commented 4 years ago

It is confusing, the documentation also mentions

in the interim both SHOULD be used to ensure interoperability with old and new implementations.
redorff commented 4 years ago

IMHO, the broker should support both but raise an error if neither are supplied...

rajahaidar commented 4 years ago

The specification doesn't mention anything about requiring the space_guid and organization_guid in the context, it says it will be deprecated in favor of context. This might be useful in applications that use openservicebroker outside of cloudfoundry

eruvanos commented 4 years ago

I agree, that it looks like the new version would not require the org_guid and space_guid in the context. With the current version the spec still marks the org_guid and space_guid as required.

I would be fine with a feature flag to disable the check. Like the implementation for the version check. https://github.com/eruvanos/openbrokerapi/blob/c0f862ee6232e98b5167d155861bc3db8fd40873/openbrokerapi/api.py#L85

Do you want to adjust the PR?

rajahaidar commented 4 years ago

Sure working on it now

rajahaidar commented 4 years ago

@eruvanos Added this change to the PR, I couldn't figure out a good way to test it and I didnt see another example in the code where tests were added for modifying environment variables. Do you suggest anything?

rajahaidar commented 4 years ago

I used monkeypatch to change that variable that is set by the environment variable into True, hope that is okay with you

eruvanos commented 4 years ago

Thank you for the input. PR is merged.