-
Notifications
You must be signed in to change notification settings - Fork 385
Add option to automatically deploy the Broker resource with the sample broker app #980
Conversation
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 fine with this, one ask
charts/ups-broker/values.yaml
Outdated
@@ -3,3 +3,5 @@ | |||
image: quay.io/kubernetes-service-catalog/user-broker:v0.0.11 | |||
# ImagePullPolicy; valid values are "IfNotPresent", "Never", and "Always" | |||
imagePullPolicy: Always | |||
# Specifies whether to deploy the Broker service-catalog object for this Deployment | |||
deployBroker: true |
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.
Can we call this deployBrokerResource
instead?
@pmorie Fixed, PTAL |
ping @pmorie |
name: ups-broker | ||
spec: | ||
url: http://{{ template "fullname" . }}.{{ .Release.Namespace }}.svc.cluster.local | ||
{{ end }} |
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 am all for this, but it won't work unless the service-catalog API server is run behind the aggregator. Can you document that please?
I'm all for this, but it depends on the API server being run behind the aggregator. I'd like to hold this until the aggregator is setup by default for the default path through the walkthrough. |
@pmorie Ok now when the aggregator stuff merged? |
charts/ups-broker/README.md
Outdated
@@ -36,6 +36,7 @@ Service Broker | |||
|-----------|-------------|---------| | |||
| `image` | Image to use | `quay.io/kubernetes-service-catalog/user-broker:v0.0.11` | | |||
| `imagePullPolicy` | `imagePullPolicy` for the ups-broker | `Always` | | |||
| `deployBrokerResource` | Whether the `Broker` Service Catalog resource should be automatically created or not | `true` | |
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.
Could we change the Broker -> User Provided Service Broker
or UPS Broker.
Broker resource is very generic.
charts/ups-broker/values.yaml
Outdated
@@ -3,3 +3,5 @@ | |||
image: quay.io/kubernetes-service-catalog/user-broker:v0.0.11 | |||
# ImagePullPolicy; valid values are "IfNotPresent", "Never", and "Always" | |||
imagePullPolicy: Always | |||
# Specifies whether to deploy the Broker service-catalog object for this Deployment | |||
deployBrokerResource: true |
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.
Can we change this to deployUPSBroker
I personally think it would be better to drop the Resource from the name, but the biggest thing missing is the fact that it's UPS broker specifically.
@luxas this is getting stale - will you rebase it or close it? Thanks ps: happy birthday |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
c348d24
to
6aa7476
Compare
@pmorie Cool. Now got back to this (finally!) |
@@ -36,6 +36,7 @@ Service Broker | |||
|-----------|-------------|---------| | |||
| `image` | Image to use | `quay.io/kubernetes-service-catalog/user-broker:v0.0.23` | | |||
| `imagePullPolicy` | `imagePullPolicy` for the ups-broker | `Always` | | |||
| `deployUPSBroker` | Whether the `User Provided Service Broker` Service Catalog resource should be automatically created or not. Requires running behind the aggregator. | `true` | |
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 values.yaml
defaults to false
, but this says the default is true
.
@luxas do you want to address the @kibbles-n-bytes's review comment and finally merge it? |
Sorry don't have bandwidth to rebase again. If you want this patch, pls carry it for me. |
No-op.
Just makes the chart automatically create the Broker object; convenient since the Chart knows how to talk to the ups-broker service.
Currently this is in an example, and assumes you've installed things in a certain fashion.
With this it's easy to just assume that when you install the broker using the chart, you get the Broker registration for free