split plans off of service classes #1106

MHBauer · 2017-08-07T17:07:44Z

a start on #454

need to fix the integration tests, probably e2es as well

MHBauer · 2017-08-07T17:22:25Z

this is wicked gnarly to rebase and keep everything working.

arschles · 2017-08-07T17:23:22Z

@MHBauer can you rename the title to something more descriptive please? Specifically, I'd like to know which part of #454 this addresses

pmorie

Overall comment: I don't think information about plans should live in two places - it will get out of sync otherwise.

I might be able to be convinced on this eventually, but I think so far we only had consensus on splitting the resources.

Suggest that for now, we should just split the resources, and talk through whether there should be high-level information about plans on the service class later.

pmorie · 2017-08-07T17:53:02Z

build/update-client-gen.sh

@@ -19,6 +19,7 @@
 set -o errexit
 set -o nounset
 set -o pipefail
+set -o xtrace


Did you mean to commit this?

Yes, I was a little weirded out seeing the other codegen run and not the client-generator.

pmorie · 2017-08-07T17:53:51Z

pkg/apis/servicecatalog/types.go

@@ -191,7 +191,7 @@ type ServiceClass struct {

 	// Plans is the list of ServicePlans for this ServiceClass.  All
 	// ServiceClasses have at least one ServicePlan.
-	Plans []ServicePlan
+	Plans []Plan


Probably less bikeshed-accretion risk here to just keep the current name.

In a pure split, what is this made up of?

Should this now have a slice of v1.LocalObjectReference?

If we're just splitting, plans should just reference the service class and we should remove this array field of ServiceClass.

Two-way references are very difficult to maintain.

Plan -> ServiceClass
not
ServiceClass -> Plan.

Isn't this an inversion of the current arrangement? Is there any particular justification for either direction?

we'll need to be able to traverse from the serviceclass to the plan quickly - can the controller (or is it "owner"?) property on serviceclass help here?

@duglin
I don't know what that is. Do you mean the BrokerName? Shouldn't that be some kind of ObjectReferece?

It sounds like you are disagreeing with the reference direction proposed.

pmorie · 2017-08-07T17:54:33Z

pkg/apis/servicecatalog/types.go

@@ -226,8 +226,41 @@ type ServiceClass struct {
 	AlphaRequires []string
 }

+type Plan struct {
+	// PlanRef is a reference to the full plan details.
+	PlanRef v1.LocalObjectReference `json:"planRef"`


I'm not sure what Plan is vs. ServicePlan - I had thought we agreed to just split them rather than keeping some information in two places.

I don't think it's going to be profitable to store this information in two places - they will get out of sync.

I think this might be related something I don't think we resolved... when I GET a serviceclass how much of the Plan is pulled back with it? If there are 300 plans do we want to force the UI/client to do 300 GETs too? See #454 (comment)

pmorie · 2017-08-07T19:20:06Z

Just a thought: it might be worth holding off on this until we can get some clarity on what the kubernetes names of these entities should be.

MHBauer · 2017-08-08T17:50:17Z

@pmorie where can we go push on the naming board or api board or whatever?

MHBauer · 2017-08-08T20:49:07Z

I don't think renaming blocks this.

Do we need a separate design?

What are thoughts on the references?
Plan -> ServiceClass
OR
ServiceClass -> Plan

duglin · 2017-08-08T21:55:09Z

I think talking about this during one of our design sessions would be good.

It might also be good to try to split the discussion into smaller pieces, I think there are at least two independent topics:
1 - which properties are in Service vs Plan and which (if any) are duplicated?
2 - how to manage the references? S->P or P->S

If you could come up with a concrete proposal for each, to help focus the discussion, then we can discuss them on an upcoming call.

pmorie · 2017-08-13T16:13:40Z

@MHBauer

I don't think renaming blocks this.

Yeah, I agree with you. I had misread your PR when I made this comment.

MHBauer · 2017-08-28T18:24:57Z

note: don't forget to add rbac for plans

MHBauer · 2017-08-29T02:18:25Z

This should build and finish through the integration tests.

I don't think I took out anything of the integration tests, but I also didn't add anything new.

MHBauer · 2017-08-29T03:03:36Z

We can probably turn some of the test verbosity down now...

MHBauer · 2017-08-29T17:30:17Z

Already seen some flakes I would have guessed existed. I'm not sure how tenable the maintenance of this approach is. I'll try and get it passing again, but I think this concept needs another think.

I may be over thinking this.

MHBauer · 2017-08-29T21:37:23Z

Seems only TPR was failing. Have attempted a fix.

pmorie

Excellent start, found a few nits or apparent TODOs. Only thing I think we should talk about is the type of the service class reference.

pmorie · 2017-08-30T01:37:40Z

contrib/hack/test-apiserver.sh

@@ -44,16 +44,19 @@ NO_TTY=1 kubectl config set-cluster service-catalog-cluster --server=https://${D
 set -x
 NO_TTY=1 kubectl create -f contrib/examples/apiserver/broker.yaml
 NO_TTY=1 kubectl create -f contrib/examples/apiserver/serviceclass.yaml
+NO_TTY=1 kubectl create -f contrib/examples/apiserver/serviceplan.yaml -v 9


is the -v argument intentional here?

now that it appears to be working, can we remove the -v ?

yes. whoops.

pmorie · 2017-08-30T01:40:13Z

pkg/apis/servicecatalog/types.go

+
+	// ServiceClassRef is a reference to the service class that
+	// owns this plan.
+	ServiceClassRef v1.LocalObjectReference


I think that we can use ServiceClassName here. LocalObjectReference is advantageous when you anticipate needing to make a local reference into a namespaced reference in the future. I don't think we anticipate that here.

What is ServiceClassName?

I think he means just use string and put the service class's name in there. However, at some point we may need to have namespaced classes/plans at some point - so would keeping LocalObjectRef be safer? In CF-land they have the notion of making certain services available only to certain orgs (they are not globally available to everyone), so it seems very possible we'll need that too at some point.

On a related question, if we allow for serviceclasses to be namespaced in the future will that be a backward incompatible change for our APIs? I think its just a matter of adding a namespace property when needed, right?

And... can a resource type be both namespaced and not namespaced? Meaning, can I have one instance of ServiceClass be namespaced and nother one be un-namespaced?

This seems the more kube-y way to do that to me. LocalObjectRef is a Name.

pmorie · 2017-08-30T01:40:35Z

pkg/apis/servicecatalog/v1alpha1/types.go

+
+	// ServiceClassRef is a reference to the service class that
+	// owns this plan.
+	ServiceClassRef v1.LocalObjectReference `json:"serviceClassRef"`


Same comment about typing.

pmorie · 2017-08-30T01:45:38Z

pkg/controller/controller.go

-			ExternalID:  plans[i].ID,
-			Free:        (plans[i].Free != nil && *plans[i].Free),
-			Description: plans[i].Description,
+func convertServicePlans(plans []osb.Plan, serviceClassName string) ([]*v1alpha1.ServicePlan, error) {


I believe that we will need to prefix the name of the service plan with the name of the service class. Plan names are only unique within a service, and not globally. I don't think this yields the greatest names, but we can fix that with porcelain.

Totally open to talking through this in a meeting, and I realize it is an intersection with an unresolved issue (#802).

Fix it later.

we should discuss this before beta. Morgan can you open an issue and put it in the 0.1.0 milestone so we don't forget.

Circling back in this group of comments for posterity:

We decided in a SIG meeting for now to use $name-$id as the kubernetes names of services to spur ourselves to finish figuring out what to do with the kubernetes names of these objects.

pmorie · 2017-08-30T01:46:39Z

pkg/controller/controller_binding.go

 	}
-
+	/*


Looks like an old comment

yea, removing.

pmorie · 2017-08-30T01:51:00Z

pkg/controller/controller_test.go

@@ -823,7 +885,7 @@ func TestCatalogConversionServicePlanBindable(t *testing.T) {
 		t.Fatalf("Failed to unmarshal test catalog: %v", err)
 	}

-	actual, err := convertCatalog(catalog)
+	actual, _, err := convertCatalog(catalog)


Looks like a TODO

pmorie · 2017-08-30T01:51:49Z

pkg/registry/servicecatalog/serviceplan/storage.go

+}
+
+// GetAttrs returns labels and fields of a given object for filtering purposes.
+func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, bool, error) {


We might want to make service class name a selectable field (in a follow-up)

I don't know what this does.

pmorie · 2017-08-30T01:52:52Z

pkg/registry/servicecatalog/serviceplan/storage.go

+
+// NewSingular returns a new shell of a service servicePlan, according to the given namespace and
+// name
+func NewSingular(ns, name string) runtime.Object {


Is this applicable, since ServicePlan is root-scoped?

No idea, copied serviceclass.

it's used in TPR storage

and in CRD as well 😉

pmorie · 2017-08-30T01:54:21Z

test/integration.sh

@@ -25,7 +25,8 @@ source "${KUBE_ROOT}/hack/lib/init.sh"
 runTests() {
  kube::etcd::start

-  go test -race -v github.com/kubernetes-incubator/service-catalog/test/integration/... --args -v 10 -logtostderr
+  go test -race -v -i github.com/kubernetes-incubator/service-catalog/test/integration/... -c \


is checking in -c intentional

I'm curious too

pmorie · 2017-08-30T01:54:39Z

test/integration/clientset_test.go

@@ -113,8 +113,12 @@ func TestEtcdHealthCheckerSuccess(t *testing.T) {
 	}
 	c := &http.Client{Transport: tr}
 	resp, err := c.Get(clientconfig.Host + "/healthz")
-	if nil != err || http.StatusOK != resp.StatusCode {
-		t.Fatal("health check endpoint should not have failed")
+	if nil != err {


separate PR plz

Debugging improvement. Why split it out?

You can split it out if you want it to land sooner. Your choice.

MHBauer · 2017-08-30T22:06:52Z

https://travis-ci.org/MHBauer/service-catalog/builds/270170017 works if you don't want to wait for cri-o builds to finish.

arschles

overall this looks good. thanks @MHBauer! also, I am ok with the local object reference.

LGTM

arschles · 2017-08-30T22:18:08Z

pkg/registry/servicecatalog/serviceplan/storage.go

+
+// NewSingular returns a new shell of a service servicePlan, according to the given namespace and
+// name
+func NewSingular(ns, name string) runtime.Object {


it's used in TPR storage

MHBauer · 2017-08-31T00:40:24Z

Everything is green, so I'll squish down into two.

duglin · 2017-08-31T01:27:25Z

This still has ServiceInstanceSpec.PlanName as a string, which is kind of interesting. Technically it should be an ObjectReference since Plans are now independent resources. However, keeping it a string means that we now manually go find the right Plan object by rather than just follow a ref.

We have an issue right now with the names of Plans because per the OSB API spec they only need to be unique within a service, but in Kube they need to be unique for the entire cluster.

Now, @pmorie suggested that we concat the serviceName and the planName and use that as the planName on the Plan resource. That doesn't yield the greatest UX. However, since ServiceInstanceSpec.PlanName isn't an objectRef and we need to do a manual lookup we can get away with ServiceInstanceSpec.PlanName only being the plan name itself and not the concatenation of ServiceName and PlanName - which means a nicer UX, at least for creating ServiceInstances. Its not great if you ever need to retrieve a Plan by itself because you then need to twiddle names.

I'd like to discuss this issue and the names of Plans in general on tomorrow's design call before we merge this. I'm hoping that the net result of this will be to just twiddling of these split resources but in case it ends up requiring a lot more surgery I think having one more day/talk would be good and not delaying it too much.

duglin · 2017-08-31T01:36:46Z

pkg/controller/controller.go

+// a brokerclient to use for a given Instance.
+func (c *controller) getServicePlan(name string) (*v1alpha1.ServicePlan, error) {
+	// use the ref to get the real plan
+	return c.servicePlanLister.Get(name)


Note - at some point this will need to be scoped to just the service. But its all based on how we deal with names of plans, etc....

It would be useful to have a TODO here for that

Not sure I agree with the scope of that. This does exactly and only what it should. TODO should be an issue, maybe?

duglin · 2017-08-31T01:41:36Z

pkg/controller/controller_binding.go

@@ -171,7 +171,8 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic

 	serviceClass, servicePlan, brokerName, brokerClient, err := c.getServiceClassPlanAndServiceBrokerForServiceInstanceCredential(instance, binding)
 	if err != nil {
-		return err
+		glog.Warning("couldn't get the things")


ok this warning isn't very useful. Can we add more info to it?

I can put it back to not having any log message at all.

I like the idea of the msg, but I want it to be meaningful

duglin · 2017-08-31T01:42:38Z

pkg/controller/controller_broker.go

@@ -214,7 +218,8 @@ func (c *controller) reconcileServiceBroker(broker *v1alpha1.ServiceBroker) erro
 		glog.V(5).Infof("Successfully fetched %v catalog entries for ServiceBroker %v", len(brokerCatalog.Services), broker.Name)

 		glog.V(4).Infof("Converting catalog response for ServiceBroker %v into service-catalog API", broker.Name)
-		catalog, err := convertCatalog(brokerCatalog)
+		glog.V(4).Infof("Converting catalog response for Broker %v into service-catalog API", broker.Name)


s/%v/%q/ so its quoted

dup line anyway

MHBauer · 2017-09-09T06:44:16Z

@kibbles-n-bytes Jenkins had a "failure to set github status". Is there a retry in our code when that tries to happen, or is it part of a jenkins plugin we probably can't touch?

MHBauer · 2017-09-09T07:08:39Z

edited, rebased, all green.

pmorie · 2017-09-09T17:25:21Z

I'm taking off LGTM1 to re-review because this is a huge API change.

kibbles-n-bytes · 2017-09-11T17:42:13Z

@MHBauer Huh, I've never seen the GitHub status setting fail. Sounds like GitHub was just down for a brief moment. That portion is touchable and I could add a retry if necessary, but I think it's such a rare occurrence that it can wait as a "nice-to-have" feature for the time being.

nilebox

I like the direction of this PR, mostly just nits.

nilebox · 2017-09-14T06:03:52Z

pkg/apis/servicecatalog/validation/serviceplan.go

+func ValidateServicePlanUpdate(new *sc.ServicePlan, old *sc.ServicePlan) field.ErrorList {
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, ValidateServicePlan(new)...)
+	allErrs = append(allErrs, ValidateServicePlan(old)...)


As far as I can tell, the old one is being sometimes implicitly validated because we need some conversion of types, comparing fields etc.
For example, see https://github.com/kubernetes-incubator/service-catalog/blob/master/vendor/k8s.io/apimachinery/pkg/api/validation/objectmeta.go#L243
There could be errors produced as a result of invalid old object, true, but that was not the primary goal here - it was a side-effect of the need to convert types.

I haven't found any example of validating the old one just for the sake of "best practice".
Seems redundant here to me (could remove in other places in follow-up PRs as well, if we have similar code already).

nilebox · 2017-09-14T06:10:58Z

cmd/apiserver/app/server/run_server_test.go

-	if getAction != 4 {
-		t.Errorf("Expected 4 'get' action, got %d", getAction)
+	if getAction != 5 {
+		t.Errorf("Expected 5 'get' actions, got %d", getAction)


Shall we start checking resource types, or at least make sure that all get/create were for different types?

nilebox · 2017-09-14T06:13:43Z

docs/proposals/implemented/2017-08-split-plans-from-serviceclass.md

+### ServiceClass API Changes
+
+Instead of having an array of ServicePlans defined inline, we have an array of
+Reference to ServicePlan.


Isn't that reverse, ServicePlan having a reference to a ServiceClass? Do we still have an array for some reason?

Good catch. I will edit this and review the whole thing to make sure it is correct.

brain fart on my part. I will update and review the whole document.

nilebox · 2017-09-14T06:14:31Z

docs/proposals/implemented/2017-08-split-plans-from-serviceclass.md

+}
+```
+
+Classes are not namespaced, so plans are not namespaced. Possibility


nit: such resources are called "cluster scoped" (as opposed to "namespace scoped") in Kubernetes

okay, will edit.

nilebox · 2017-09-14T06:19:53Z

docs/proposals/implemented/2017-08-split-plans-from-serviceclass.md

+
+Classes are not namespaced, so plans are not namespaced. Possibility
+of collision of two ServicePlans from different ServiceClass based on
+Name. This will be addressed by concatinating the ServiceClass


I wonder if we have the same issue for ServiceClass already. If so, how did we resolve this?
According to the spec, service name:

MUST be unique across all service objects returned in this response

so it's not globally unique either.

This is an open issue upstream as well.

nilebox · 2017-09-14T06:26:15Z

pkg/apis/servicecatalog/validation/serviceplan.go

+//var validateServicePlanName = apivalidation.NameIsDNSSubdomain
+
+const servicePlanNameFmt string = `[-a-z0-9]+`
+const servicePlanNameMaxLength int = 63


Where did you get this limit from?

This comes from upstream when the serviceplan name was used as a label key, it was failing.
#1047

I should get rid of that comment above.

nilebox · 2017-09-14T06:26:49Z

pkg/apis/servicecatalog/validation/serviceplan.go

+		errs = append(errs, utilvalidation.MaxLenError(servicePlanNameMaxLength))
+	}
+	if !servicePlanNameRegexp.MatchString(value) {
+		errs = append(errs, utilvalidation.RegexError(servicePlanNameFmt, "plan-name-0000"))


this is a bad example I guess :) should provide a legit example with UUID suffix instead.

nilebox · 2017-09-14T06:30:17Z

pkg/controller/controller.go

@@ -358,7 +372,15 @@ func (c *controller) getServiceClassPlanAndServiceBrokerForServiceInstanceCreden
 }

 // ServiceBroker utility methods - move?


This comment seems to be misplaced now.

oops, thanks.

nilebox · 2017-09-14T06:32:59Z

pkg/controller/controller.go

+		serviceClasses[i].SetName(svc.Name)
+
+		// set up the plans using the ServiceClass Name
+		plans, err := convertServicePlans(svc.Plans, svc.Name)


nit: pass serviceClasses[i].Name instead of svc.Name for cleanliness.

I do not follow, what do you mean?

2 lines above you take a value from svc.Name and set it to serviceClass.
convertServicePlans generates a plan which has a reference to serviceClass, right? So I would prefer to explicitly pass serviceClasses[i].Name here instead of svc.Name (even though the value is the same).

Imagine that you would change the code 2 lines-above to something like

serviceClasses[i].SetName(svc.Name + generateUuid())

in this case the name you pass into convertServicePlans would be out of sync. With my change it wouldn't.

nilebox · 2017-09-14T06:39:40Z

plugin/pkg/admission/serviceplan/defaultserviceplan/admission.go

@@ -77,6 +79,9 @@ func (d *defaultServicePlan) Admit(a admission.Attributes) error {
 		return nil
 	}

+	// check and default


I think this was more of a top-level 'what are we doing here' type comment, but in context it is pretty useless. I'll remove it, but I think we should try and get a group together to review the default service plan admission controller.

jboyd01 · 2017-09-14T13:32:47Z

I hate to add more work here Morgan, but #1210 was merged yesterday that added a new admission controller that blocks changes to an instance's service plan if the plan is not updatable.

MHBauer · 2017-09-16T01:03:37Z

I'll get started on the refresh Monday morning.

MHBauer · 2017-09-18T23:07:09Z

Green Jenkins, phew.

pmorie · 2017-09-19T01:09:32Z

plugin/pkg/admission/serviceplan/changevalidator/admission.go

@@ -75,7 +76,7 @@ func (d *denyPlanChangeIfNotUpdatable) Admit(a admission.Attributes) error {
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			glog.V(5).Infof("Could not locate service class %v, can not determine if UpdateablePlan.", instance.Spec.ServiceClassName)
-			return nil
+			return nil // should this be `return err`? why would we allow the instance in if we cannot determine it is updatable?


Seems like there's a follow-up issue here - do you want to create one?

If memory serves me, I tried this with returning an error, it broke unit tests, so I figured this was the desired behavior.

I suppose this particular thing is not concerned with whether the instance refers to a valid service class. We can let it through on the recognition that it will be caught later in the actual validation stages.

Weird, but I think that is the correct analysis. Thoughts?

pmorie · 2017-09-19T13:52:04Z

I'm concerned that the walkthrough e2es may not be testing the correct version of the code. When I try to run the walkthrough locally, the controller can't create serviceplans for the ups-broker:

apiVersion: servicecatalog.k8s.io/v1alpha1
kind: ServiceBroker
metadata:
  creationTimestamp: 2017-09-19T13:39:46Z
  finalizers:
  - kubernetes-incubator/service-catalog
  generation: 1
  name: ups-broker
  resourceVersion: "3"
  selfLink: /apis/servicecatalog.k8s.io/v1alpha1/servicebrokers/ups-broker
  uid: 00183ac8-9d40-11e7-a37c-0242ac110005
spec:
  url: http://ups-broker-ups-broker.ups-broker.svc.cluster.local
status:
  conditions:
  - lastTransitionTime: 2017-09-19T13:39:46Z
    message: 'Error syncing catalog from ServiceBroker. Error reconciling servicePlan
      "default-86064792-7ea2-467b-af93-ac9694d96d52" (broker "ups-broker"): serviceplans.servicecatalog.k8s.io
      is forbidden: User "system:serviceaccount:catalog:service-catalog-controller-manager"
      cannot create serviceplans.servicecatalog.k8s.io at the cluster scope'
    reason: ErrorSyncingCatalog
    status: "False"
    type: Ready
  reconciledGeneration: 0

Going to try the e2e next.

pmorie · 2017-09-19T14:08:45Z

So, I am not sure at all how e2e could be passing, and I found a bug in the rbac setup for which I needed to create https://github.com/MHBauer/service-catalog/pull/3.

I think we should ensure that we understand why the tests showed as green before we merge this.

pmorie · 2017-09-19T14:09:31Z

Adding do-not-merge for now to avoid an accidental merge until we can figure out why the tests appeared to pass.

kibbles-n-bytes · 2017-09-19T17:00:07Z

@pmorie @MHBauer Looking into it now.

MHBauer · 2017-09-19T19:45:50Z

Oh ya, missed my own reminder. whoops.
#1106 (comment)

MHBauer · 2017-09-19T21:00:48Z

something weird is going on with lint.

- logic for separate plan lookups - make the tests keep working - remove plan references from class - nuke plans - defining the serviceplan storage - controller adjustments - remove json unnecessary annotations - getting the storage for apiserver hooked up - prevent service plan ExeternalID changes on update - random test case improvements to assist in debugging - the secret is defining whether or not an object is namespaced in nineteen different places - de-verbose - quotes for doug - more debug information in tpr test

MHBauer · 2017-09-19T23:16:57Z

Green again.

vaikas · 2017-09-20T16:38:14Z

@pmorie verified RBAC rules as per discussion on the slack channel. Merging.

ash2k · 2017-09-21T00:46:52Z

pkg/apis/servicecatalog/types.go

+
+	// ServiceClassRef is a reference to the service class that
+	// owns this plan.
+	ServiceClassRef v1.LocalObjectReference


This could be just an Owner Reference with Controller=true?

It's merged, so we can do followups now.

What does controller=true mean?

@MHBauer Kubernetes object could have multiple OwnerReferences, but only one of them could be a controller ("true owner").
IMO a dedicated ServiceClassRef makes sense there, and provides a better more transparent UX.

That said, I agree with @ash2k that we should also set OwnerReference to make sure there is no clash between different classes having plans with same UUIDs/names.
It will also prevent deletion of ServiceClass if it has existing ServicePlan referencing it.

See pull request #979 for the example of how I set OwnerReference for secret owned by binding.

See item 11 https://github.com/kubernetes/community/blob/master/contributors/devel/controllers.md
and https://github.com/kubernetes/community/blob/master/contributors/design-proposals/api-machinery/controller-ref.md for details.

MHBauer added the in-progress label Aug 7, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 7, 2017

MHBauer changed the title ~~Do454~~ split plans off of service classes Aug 7, 2017

pmorie reviewed Aug 7, 2017

View reviewed changes

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 28, 2017

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 28, 2017

MHBauer removed the in-progress label Aug 29, 2017

pmorie reviewed Aug 30, 2017

View reviewed changes

arschles approved these changes Aug 30, 2017

View reviewed changes

arschles added the LGTM1 label Aug 30, 2017

MHBauer added the broker-api-spec-compliance label Aug 31, 2017

duglin reviewed Aug 31, 2017

View reviewed changes

pmorie removed the LGTM1 label Sep 9, 2017

pmorie mentioned this pull request Sep 11, 2017

ServiceClass and ServicePlan should each have spec and status #1214

Closed

nilebox reviewed Sep 14, 2017

View reviewed changes

pmorie reviewed Sep 19, 2017

View reviewed changes

pmorie added the do-not-merge label Sep 19, 2017

MHBauer added 6 commits September 19, 2017 14:01

Add Proposal Text

46207e9

plan name tweaker for @duglin

7682752

rebase service instance update validator

6d23e38

update tests to add split plans

141466e

generated code for service plans

6b7c142

vaikas added LGTM1 and removed do-not-merge labels Sep 20, 2017

vaikas merged commit 170aab5 into kubernetes-retired:master Sep 20, 2017

ash2k reviewed Sep 21, 2017

View reviewed changes

		@@ -358,7 +372,15 @@ func (c *controller) getServiceClassPlanAndServiceBrokerForServiceInstanceCreden
		}

		// ServiceBroker utility methods - move?

split plans off of service classes #1106

split plans off of service classes #1106

Conversation

MHBauer commented Aug 7, 2017

MHBauer commented Aug 7, 2017

arschles commented Aug 7, 2017

pmorie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie commented Aug 7, 2017

MHBauer commented Aug 8, 2017

MHBauer commented Aug 8, 2017

duglin commented Aug 8, 2017

pmorie commented Aug 13, 2017 • edited Loading

MHBauer commented Aug 28, 2017

MHBauer commented Aug 29, 2017

MHBauer commented Aug 29, 2017

MHBauer commented Aug 29, 2017

MHBauer commented Aug 29, 2017

pmorie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Aug 30, 2017

arschles left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Aug 31, 2017

duglin commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Sep 9, 2017

pmorie left a comment •

edited

Loading

pmorie commented Aug 13, 2017 •

edited

Loading

arschles left a comment •

edited

Loading

kibbles-n-bytes commented Sep 11, 2017 •

edited

Loading