-
Notifications
You must be signed in to change notification settings - Fork 434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Service dependencies specification #488
Conversation
Hey fmui! 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. |
"service_name": "mysql", | ||
"plan_id": "1b367d58-6f17-4cb3-9ef8-3a69e752b826", | ||
"plan_name": "small", | ||
"credentials": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this contains a binding, why not call the key binding
? The word credentials
doesn't communicate that other things besides credentials can exist in the object (as we see in this example), which I suspect is why the word binding
was chosen for the API.
* Instance dependencies declare dependencies to a specific instance either | ||
by a service instance ID or by a service instance alias. | ||
|
||
Dependencies are injected as parameters when a service instance is provisioned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
injected
this makes it sound like there is magic happening, these are not just provided in the provision request?
Dependencies are declared in the catalog at the plan level. | ||
There are three types of dependencies. | ||
|
||
* Open dependencies are arbitrary dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an "open dependency" sounds like it should be an unfulfilled dependency, ie: the service requests a dependency of some type. This is really a platform defined dependency or something along that line.
|
||
| Field | Type | Description | | ||
| --- | --- | --- | | ||
| open\_dependencies\_parameter | string | A parameter name used for an open list of dependencies. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "open_dependencies_field" to mean the field name the broker expects this list to be in.
Also: if it is a list, make it a list?
|
||
| Field | Type | Description | | ||
| --- | --- | --- | | ||
| parameter* | string | Name of the parameter. MUST be a non-empty string and MUST be unique within all dependency parameters. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here again, field vs parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the field that the broker is expecting to look in for the service dependecies? Suppose the broker wanted to accept it in a nested object, I think it might need to be a path to support something like "parameters.dependencies.service"
| parameter* | string | Name of the parameter. MUST be a non-empty string and MUST be unique within all dependency parameters. | | ||
| description* | string | A short description of the dependency. MUST be a non-empty string. | | ||
| optional | boolean | Whether the dependency is optional or not. Defaults to false. | | ||
| service_id | string | Service ID of the instance. MUST be a non-empty string. MUST NOT be used in conjunction with the fields `plan_ids` and `service_name`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not have service_id be service_ids and allow a list?
|
||
``` | ||
{ | ||
"parameters": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend pulling out the dependency fields into their own field next to parameters, like:
{
"dependencies" {
"service_id": "71ef3933-8520-4f26-b808-f483d23f94a6",
"service_name": "mysql",
"plan_id": "1b367d58-6f17-4cb3-9ef8-3a69e752b826",
"plan_name": "small",
},
"parameters": {
"database": {
"credentials": {
"uri": "mysql://mysqluser:pass@mysqlhost:3306/dbname",
"username": "mysqluser",
"password": "pass",
"host": "mysqlhost",
"port": 3306,
"database": "dbname"
}
}
}
}
|
||
\* Fields with an asterisk are REQUIRED. | ||
|
||
For bindable services, the value MUST contain be the response of a binding request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For bindable services, the value MUST contain be the response of a binding request.
I am having a really hard time trying to figure out what this means. The values [in the above table>] must be contained inside the request for binding? What is required about response? I am confused because only requests have "parameters" but this is mixing the request/response demands.
Instances of non-bindable services can only be provided to the same broker. | ||
The platform MUST set the instance ID into the parameter value object. | ||
|
||
| Field | Type | Description | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a table of allowed additive fields for non-bindable services?
the structures above. Additionally, the platform MAY add a dependency name, | ||
set by the user to each object. | ||
|
||
| Field | Type | Description | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a table of allowed additive fields for "open deps"?
It took me a very long time to understand this was what this was attempting to say. Perhaps one table with restrictions on the fields?
``` | ||
{ | ||
"parameters": { | ||
"some_dependencies ": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is suppose to be additive to the OSBAPI, I would recommend not mix instance parameters with broker dependencies. It is really confusing and I suspect this would be difficult to implement cleanly in broker.
Overall I think this change does not do enough to allow the ecosystem to play nice together. I understand the use-case but I think this implementation is too opinionated about the solution it recommends brokers to implement. I don't see an easy way for broker to broker dependencies, such as the use case where I want to bring my broker that provisions my application, and that application depends on a database of some kind, it would be real nice to be able to say "in my application broker, please tie in a database that speaks SQL". I think the customizable parameter targets are confusing. And in the very least they need to be paths inside the json object relative to the root of the object. It would be interesting to use the concept of I think more detail needs to be added around what the platform is expected to do when it provisions something and the catalog can change with more items, or deprovision and items disappear. I wonder if there is also a chance to add a concept of a service depending on a binding. At the moment we make bindings but we do not get to say who they are for. But a binding could be shared as well so each binding could have n parties using it. I am not sure on that point... |
I agree with @n3wscott 's summary. It would be helpful to focus more on illustrating the big picture and how the key use cases are fulfilled. Reading this proposal, I found it somewhat difficult to keep track of where things fit in to a high-level workflow, and who-or-what is the actor performing each step. It may help to focus more on addressing these general questions:
|
Adding an opinion here that I expressed at the f2f. I believe that solving "adheres to" is a necessary pre-requisite for some of this. |
Refresh my memory - what's the status of this? Awaiting an updated PR? |
See issue #427.
First draft of the service dependencies specification.