Service Broker Create Service Instance Schema Validation #847
Conversation
[#145580733] Signed-off-by: Alex Blease <[email protected]>
Signed-off-by: Sam Gunaratne <[email protected]>
[#145580731] Signed-off-by: Alex Blease <[email protected]>
Signed-off-by: Alex Blease <[email protected]>
[#145580731] Signed-off-by: Sam Gunaratne <[email protected]>
[#146276747] Signed-off-by: Sam Gunaratne <[email protected]>
[#146276815] Signed-off-by: Alex Blease <[email protected]>
[#146276747] Signed-off-by: Luis Urraca <[email protected]>
[#146276747] Signed-off-by: Luis Urraca <[email protected]>
|
Hey ablease! 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. |
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/148656211 The labels on this github issue will be updated when the story is started. |
|
Hey @ablease, we've prioritized this story and will take a look soon. |
There was a problem hiding this comment.
Disclaimer: We understand that the patterns we're requesting may not be consistent with some of the services code (such as CatalogPlan where we seem to have written another validation framework)...But we'd ideally like to modernize that part of the codebase as well eventually (PRs welcome).
We'd like to hear your thoughts on this before we merge.
- Aakash & @tcdowney
| @errors = VCAP::Services::ValidationErrors.new | ||
| validate_and_populate_create_instance(attrs) | ||
| validate_and_populate_create_instance(schema) |
There was a problem hiding this comment.
(This comment doesn't necessarily reflect changes form this specific PR, but the set of PRs)
We were surprised to see validations happening in the constructor. We are more familiar with the ActiveModel pattern of not explicitly implementing valid?, but rather including validate statements.
With this approach, we'd be able to convert the basic validations (data types, presence checks, length) with the validations provided by ActiveModel, and separate those from the content validations.
We're asking for this change to make the code consistent with recent V3 code where validations are important. Examples are in the messages/ folder
| validate_schema(create_instance_path, create_instance_schema) | ||
| return unless errors.empty? | ||
|
|
||
| @create_instance = create_instance_schema |
There was a problem hiding this comment.
(This comment doesn't necessarily reflect changes form this specific PR, but the set of PRs)
We were also surprised to see the object being created conditionally through the validations. We'd prefer that the object's validity rely on valid?'s return value, rather than presence of an attribute.
| end | ||
| end | ||
|
|
||
| def schema_validations |
There was a problem hiding this comment.
This method is a prime example of being close to how ActiveModel validations are represented. See ActiveModel::Validations Custom Methods
|
Thanks for the feedback @aashah & @tcdowney. I agree that the Since submitting this we have added additional schema support which we are looking to raise as a PR in the next few days. This means that this branch and our head have diverged quite a lot. To save us from having to modify this branch, merging, then fixing all the commits from our newer branch would you be willing to accept this PR and have us do the refactoring you suggest as part of our next PR Update Service Instance Schemas? Let us know what you think. Sam & @lurraca |
|
Seems good to us! Feel free to close this PR.
|
|
Just kidding, let's just merge this PR right now. An upcoming PR can address some of the concerns above. Whoever picks this up, feel free to just ignore the review (maybe don't dismiss it so it can stay as a reference) |
As a Service Broker Author, when I register a plan with a create service instance schema, I get fast feedback when my schema is invalid.
#146276815, #146276747.
This PR follows from a previous PR.
Why
The Open Service Broker API is proposing allowing brokers to define JSON schema for their configuration parameters. This will allow tooling to validate parameters and UIs to auto generate forms.
What
This PR improves support for create instance schemas. Schemas are parsed during registration, and validated against the following criteria:
"type": "object"This PR address the 3 notes that were not implemented in the previous PR.
Notable things that are not in this PR but are addressed in future stories are:
Feedback appreciated!
I have viewed signed and have submitted the Contributor License Agreement
I have made this pull request to the
masterbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests on bosh lite