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
[Proposal] Plan support #294
[Proposal] Plan support #294
Conversation
89ef012
to
e9a6c95
Compare
e9a6c95
to
ca2bf88
Compare
49cfe16
to
9645fff
Compare
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.
A typo and requesting another example without parameter reference. Otherwise, +1
docs/specs/apb.yml-plan-support.md
Outdated
| --- | ||
|
|
||
| 3) Move any example of a `[]map[string]T` to `[]T`. The former introduced an entirely | ||
| unecessary map layer. They started to get nested after moving params onto plans and |
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.
TYPO: unecessary -> unnecessary
docs/specs/apb.yml-plan-support.md
Outdated
|
|
||
| 3) Move any example of a `[]map[string]T` to `[]T`. The former introduced an entirely | ||
| unecessary map layer. They started to get nested after moving params onto plans and | ||
| things got very ugly because of it. Reintroduce `name` onto `T`s. |
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.
+1 to reintroducing name.
| title: Mediawiki Site Name | ||
| ``` | ||
|
|
||
| Should use this pattern for any additional changes moving forward. |
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.
+1 this make it more explicit and consistent. It also makes parsing it broker side that much easier.
| ## Questions | ||
|
|
||
| * Obviously this is a large, breaking change to **all** existing APBs and their `apb.yml` files. | ||
| How do we want to make across the board so things get rolled out as smoothly as possible? |
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 say we do the following:
- update all the apbs in apb-examples with the new plan stuff
- update the apb tooling so that it generates a default plan
- rebuild all apbs after merging the above examples changes
- merge broker and rebuild broker image
- test it all
| How do we want to make across the board so things get rolled out as smoothly as possible? | ||
| * Is this a good opportunity to have the broker start respecting the version label found on | ||
| apbs? I think it makes sense to have that in lock setup with broker release versions. Is | ||
| broker master considered `0.10` since it's `release-0.9++`? |
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.
The version in the ansible-service-broker rpm spec is currently 1.0.0 in master. We have not built an rpm from this spec yet so we can change it if we want to go do 0.10.0
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'm happy with 1.0.0 since that's likely what we will release as under GA, right?
| longDescription: This plan provides a single non-HA PostgreSQL server with persistent storage | ||
| cost: $5.99 monthly | ||
| parameters: *_p | ||
| ``` |
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.
Would be nice to see a simple example of not using the references so folks know you can also put the parameters inline with the plan.
docs/specs/apb.yml-plan-support.md
Outdated
| displayName: PostgreSQL (APB) | ||
| console.openshift.io/iconClass: icon-postgresql | ||
| dependencies: | ||
| - postgresql_version |
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 a typo, dependencies has been moved up to the metadata.
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.
Updated
|
@jmrodri Updated based on your feedback, +1 re: single param set example. |
No description provided.