Broker Relist #1183

eriknelson · 2017-08-31T18:14:54Z

Initial fields for #1086

pmorie

Looks like a promising start - you need to add a validation and do a small amount of refactoring. Also, since the CLI argument becomes a dummy, you should hide it. (see options.go under controller-manager).

pmorie · 2017-09-08T00:43:06Z

pkg/apis/servicecatalog/types.go

@@ -60,8 +60,29 @@ type ServiceBrokerSpec struct {
 	// CABundle is a PEM encoded CA bundle which will be used to validate a Broker's serving certificate.
 	// +optional
 	CABundle []byte
+
+	// RelistBehavior specifies the type of relist behavior the catalog should exhibit


We need to add validations for these fields. See pkg/apis/servicecatalog/validation/broker.go

pmorie · 2017-09-08T00:45:57Z

pkg/apis/servicecatalog/v1alpha1/types.go

+
+	// RelistBehavior specifies the type of relist behavior the catalog should exhibit
+	// when relisting ServiceClasses available from a broker
+	RelistBehavior ServiceBrokerRelistBehavior


fields in versioned APIs need to have json tags associated with them. For this field, it would be:

RelistBehavior ServiceBrokerRelistBehavior `json:"relistBehavior"`

You'll need to add these to the fields in this file.

pmorie · 2017-09-08T00:46:15Z

pkg/controller/controller_broker.go

@@ -113,8 +113,8 @@ const (
 // shouldReconcileServiceBroker determines whether a broker should be reconciled; it
 // returns true unless the broker has a ready condition with status true and
 // the controller's broker relist interval has not elapsed since the broker's
-// ready condition became true.
-func shouldReconcileServiceBroker(broker *v1alpha1.ServiceBroker, now time.Time, relistInterval time.Duration) bool {
+// ready condition became true, or if the broker's RelistBehavior is set to Manual


missing period

pmorie · 2017-09-08T00:50:19Z

pkg/controller/controller_broker.go

-// ready condition became true.
-func shouldReconcileServiceBroker(broker *v1alpha1.ServiceBroker, now time.Time, relistInterval time.Duration) bool {
+// ready condition became true, or if the broker's RelistBehavior is set to Manual
+func shouldReconcileServiceBroker(broker *v1alpha1.ServiceBroker, now time.Time, relistInterval metav1.Duration) bool {


Technically you do not need to pass in the relistInterval parameter here, since you're now reading it from the broker.

Oh, I didn't realize you were still reading it as a parameter. You already have the broker here - you can read the value in this method and delete the relistInterval parameter. There needs to ve a validation that ensures that the duration field is set when the mode is 'duration'.

pmorie · 2017-09-08T00:54:34Z

pkg/controller/controller_broker.go

@@ -168,14 +185,16 @@ func (c *controller) reconcileServiceBrokerKey(key string) error {
 // processed and should be resubmitted at a later time.
 func (c *controller) reconcileServiceBroker(broker *v1alpha1.ServiceBroker) error {
 	glog.V(4).Infof("Processing ServiceBroker %v", broker.Name)
+	if broker.Spec.RelistDuration == nil {


I think this TODO is fixed :)

You should be defensive in programming, but some assumptions that you have validated for already are okay. So for instance, you should ensure that you don't dereference a nil pointer for an optional field. However, if that optional field must be set based on a validation, a state where that validation fails in code should be handled like an error.

pmorie · 2017-09-08T00:54:52Z

pkg/controller/controller_broker.go

-			"Not processing ServiceBroker %v because relist interval has not elapsed since the broker became ready",
-			broker.Name,
-		)
+	if !shouldReconcileServiceBroker(broker, time.Now(), *broker.Spec.RelistDuration) {


Your TODO appears to be handled now in shouldReconcileServiceBroker.

pmorie · 2017-09-08T00:59:08Z

pkg/controller/controller_broker_test.go

@@ -553,6 +554,8 @@ func testReconcileServiceBrokerWithAuth(t *testing.T, authInfo *v1alpha1.Service
 	}
 }

+// ERIK:TODO: Write up another test here to confirm an error is bubbled when
+// RelistDuration is nil, similar to this?


RelistDuration being nil should be handled in shouldReconcileBroker.

pmorie · 2017-09-08T01:00:35Z

pkg/storage/tpr/storage_interface_test.go

@@ -140,7 +141,9 @@ func TestCreateWithNoNamespace(t *testing.T) {
 	inputServiceBroker := &sc.ServiceBroker{
 		ObjectMeta: metav1.ObjectMeta{Name: name},
 		Spec: sc.ServiceBrokerSpec{
-			URL: "http://my-awesome-broker.io",
+			URL:            "http://my-awesome-broker.io",


I am not qualified to review TPR stuff. @arschles @nilebox

pmorie

Looks like validations need to be tweaked a little bit. Left a couple small asks and nits.

Let's do the subresource and porcelain command in a follow-up.

pmorie · 2017-09-12T01:16:07Z

pkg/apis/servicecatalog/testing/fuzzer.go

@@ -192,6 +193,11 @@ func FuzzerFor(t *testing.T, version schema.GroupVersion, src rand.Source) *fuzz
 			// Set the bytes field on the RawExtension
 			r.Raw = bytes
 		},
+		func(bs *servicecatalog.ServiceBrokerSpec, c fuzz.Continue) {


I'm surprised that the default fuzzer didn't work for this field. Do you try it?

Think I did, but I'll pull it and try again.

pmorie · 2017-09-12T01:41:23Z

pkg/apis/servicecatalog/v1alpha1/defaults.go

+		spec.RelistBehavior = ServiceBrokerRelistBehaviorDuration
+	}
+
+	if spec.RelistDuration == nil {


You should only default relist duration if the relist behavior is "Duration". It should not be defaulted for manual behavior.

pmorie · 2017-09-12T01:41:57Z

pkg/apis/servicecatalog/v1alpha1/defaults_test.go

@@ -68,6 +68,22 @@ func roundTrip(t *testing.T, obj runtime.Object) runtime.Object {
 	return obj3
 }

+func TestSetDefaultServiceBroker(t *testing.T) {


Tests here need to change according to the logic as I commented above

There should be test cases for:

Neither duration or behavior set

Behavior set to manual

Behavior set to duration but no duration provided

Still need test cases for these defaults. You need to ensure that after an object starting in each of the states in the above comment is defaults, that the object is in the expected state.

pmorie · 2017-09-12T01:43:19Z

pkg/apis/servicecatalog/v1alpha1/types.go

+
+	// RelistBehavior specifies the type of relist behavior the catalog should exhibit
+	// when relisting ServiceClasses available from a broker
+	RelistBehavior ServiceBrokerRelistBehavior `json:"relistBehavior"`


newline separate these to keep it consistent with the rest of the file, same for the other types.go

pmorie · 2017-09-12T01:44:40Z

pkg/apis/servicecatalog/validation/broker.go

@@ -99,6 +99,12 @@ func validateServiceBrokerSpec(spec *sc.ServiceBrokerSpec, fldPath *field.Path)
 			)
 		}

+		if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorDuration && spec.RelistDuration == nil {


Question: do you think it is valid for duration to be set if the behavior is manual? It might be. If you switch from duration to manual temporarily, for example, you might not expect to have to unset the duration. Consider what the 'least surprise' is here.

Also, I don't see unit tests for the validations

do you think it is valid for duration to be set if the behavior is manual? It might be. If you switch from duration to manual temporarily, for example, you might not expect to have to unset the duration.

Agreed. I would be surprised if I had to explicitly unset duration, and instead would expect the duration to simply be ignored.

pmorie · 2017-09-12T01:46:33Z

pkg/controller/controller_broker_test.go

 	// reconcile: whether or not the reconciler should run, the return of
 	// shouldReconcileServiceBroker
+	// ERIK:TODO: Test new Relist codepaths


Looks like this TODO still needs to be done, accurate?

Have a local commit with a bunch of error handling I wanted to run by you. It tests the RelistDuration == nil branch, but I want to make sure I hit the rest of them.

pmorie

Looks like the validations and defaults still need to be tested.

Also, validation needed to ensure that the behavior field is set.

pmorie · 2017-09-13T14:30:26Z

pkg/apis/servicecatalog/types.go

+	// when relisting ServiceClasses available from a broker
+	RelistBehavior ServiceBrokerRelistBehavior
+
+	//RelistDuration is the frequency by which a controller will relist the broker


There should be spaces after the // and all godoc comments for fields should end in periods.

pmorie · 2017-09-13T14:31:42Z

pkg/apis/servicecatalog/types.go

+type ServiceBrokerRelistBehavior string
+
+const (
+	// ServiceBrokerRelistBehaviorDuration will allow the RelistDuration frequency to be configured


// ServiceBrokerRelistBehaviorDuration indicates that the broker will be relisted automatically after the specified duration has passed.

pmorie · 2017-09-13T14:32:10Z

pkg/apis/servicecatalog/types.go

+const (
+	// ServiceBrokerRelistBehaviorDuration will allow the RelistDuration frequency to be configured
+	ServiceBrokerRelistBehaviorDuration ServiceBrokerRelistBehavior = "Duration"
+	// ServiceBrokerRelistBehaviorManual will configure the catalog to only perform future


// ServiceBrokerRelistBehaviorManual indicates that the broker is only relisted when the spec of the broker changes.

pmorie · 2017-09-13T14:34:57Z

pkg/apis/servicecatalog/v1alpha1/defaults_test.go

@@ -68,6 +68,22 @@ func roundTrip(t *testing.T, obj runtime.Object) runtime.Object {
 	return obj3
 }

+func TestSetDefaultServiceBroker(t *testing.T) {


Still need test cases for these defaults. You need to ensure that after an object starting in each of the states in the above comment is defaults, that the object is in the expected state.

pmorie · 2017-09-13T14:35:26Z

pkg/apis/servicecatalog/validation/broker.go

@@ -99,6 +99,12 @@ func validateServiceBrokerSpec(spec *sc.ServiceBrokerSpec, fldPath *field.Path)
 			)
 		}



Need a validation here to ensure that the relist behavior is set.

Still need to update the unit tests for this validation.

pmorie · 2017-09-13T14:36:10Z

pkg/controller/controller_broker.go

+				}
+
+				if broker.Spec.RelistDuration == nil {
+					glog.V(10).Infof(


This should be logged at Error

jboyd01 · 2017-09-13T19:00:12Z

pkg/apis/servicecatalog/validation/broker_test.go

+			valid: true,
+		},
+		{
+			name: "valid broker - manual behavior with behavior",


nit: name should be manual behavior with duration instead of manual behavior with beavior

jboyd01

lgtm

staebler

Some nits

staebler

Generally, LGTM. Some nits.

staebler · 2017-09-13T19:16:45Z

pkg/apis/servicecatalog/validation/broker.go

 	}

 	if spec.InsecureSkipTLSVerify && len(spec.CABundle) > 0 {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("caBundle"), spec.CABundle, "caBundle cannot be used when insecureSkipTLSVerify is true"))
 	}

+	if "" == spec.RelistBehavior {
+		allErrs = append(allErrs,
+			field.Required(fldPath.Child("relistDuration"),


The path here should be relistBehavior and the comment should indicate that the broker must have a relistBehavior.

staebler · 2017-09-13T19:18:28Z

pkg/apis/servicecatalog/validation/broker.go

+	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorDuration && spec.RelistDuration == nil {
+		allErrs = append(
+			allErrs,
+			field.Required(fldPath.Child("relistDuration"), "relistDuration must be set if behavior is set to Duration"),


This should probably be "relistDuration must be set if relistBehavior is set to Duration", so that it is clear which field is of importance.

Looks like this still needs to be done

staebler · 2017-09-13T19:20:26Z

pkg/apis/servicecatalog/validation/broker.go

+			field.Required(fldPath.Child("relistDuration"), "relistDuration must be set if behavior is set to Duration"),
+		)
+	}
+


Do we want to enforce that relistRequests be non-negative.

I had a similar thought. Should the field be a uint? I that normally acceptable?

uint is discouraged. All you need to do is add a validation check to ensure it is non-negative. And indicate that restriction in the comment.

@staebler pushed an update to the comments indicating as such. Validation asserts greater than zero.

staebler · 2017-09-13T19:24:19Z

pkg/apis/servicecatalog/v1alpha1/defaults_test.go

+			)
+		}
+
+		if tc.duration == nil && actualSpec.RelistDuration == nil {


This check does not verify that the actual RelistDuration was nil for the "behavior set to manual" test.

There is an open question as to whether or not RelistDuration MUST BE nil given a RelistBehavior of Manual. Is this something that needs to be asserted? CC @pmorie

@eriknelson yes, it should be enforced to nil (and checked at the validation stage). similar to union types, where only one branch can be initialized at a time.

staebler · 2017-09-13T19:32:42Z

pkg/apis/servicecatalog/validation/broker.go

+				"brokers must have a relistDuration"))
+	}
+
+	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorDuration && spec.RelistDuration == nil {


We need to validate that the RelistDuration is above a reasonable minimum, at least above 0.

staebler · 2017-09-13T19:34:03Z

pkg/apis/servicecatalog/validation/broker_test.go

+				},
+			},
+			valid: false,
+		},


We also need tests to verify the behavior when RelistBehavior is invalid and empty.

staebler · 2017-09-13T19:40:55Z

pkg/controller/controller_broker.go

+					return false
+				}
+
+				if broker.Spec.RelistDuration == nil {


Is this check necessary given that we have already performed validation on the Broker Spec to ensure that the RelistDuration is not nil when the RelistBehavior is not Manual?

I've been back and forth on this. Are folks generally comfortable with trusting and dereffing a pointer value given a validation?

You should always program defensively, IMO.

staebler · 2017-09-13T19:45:07Z

pkg/apis/servicecatalog/types.go

+	// when the RelistBehavior is set to ServiceBrokerRelistBehaviorDuration
+	RelistDuration *metav1.Duration
+
+	// RelistRequests is a strictly increasing integer counter that can be manually incremented


If we are claiming that this is a strictly increasing integer, then we should add a check in the ValidateUpdate function of the brokerRESTStrategy that validates that the new value is not less than the old value.

staebler · 2017-09-13T19:51:48Z

pkg/controller/controller_broker_test.go

 			reconcile: true,
 		},
+		{
+			name: "ready, duration behavior, nil duration",


We shouldn't need to test this. This should be caught by the Broker validator.

If it's being asserted in shouldReconcileBroker, I think it's worth keeping as a test case. Happy to remove if folks feel strongly otherwise.

staebler · 2017-09-13T19:54:06Z

pkg/controller/controller_broker_test.go

@@ -553,6 +585,7 @@ func testReconcileServiceBrokerWithAuth(t *testing.T, authInfo *v1alpha1.Service
 	}
 }

+// RelistDuration is nil, similar to this?


Is this a stray comment that got placed here accidentally?

eriknelson · 2017-09-13T21:01:59Z

@staebler responded to feedback, thank you!

pmorie

Found a couple little nits. Super close to LGTM.

pmorie · 2017-09-14T00:08:54Z

pkg/apis/servicecatalog/validation/broker.go

+	if "" == spec.RelistBehavior {
+		allErrs = append(allErrs,
+			field.Required(fldPath.Child("relistBehavior"),
+				"brokers must have a relistBehavior"))


"relist behavior is required"

pmorie · 2017-09-14T00:09:27Z

pkg/apis/servicecatalog/validation/broker.go

+	isValidRelistBehavior := spec.RelistBehavior == "Duration" ||
+		spec.RelistBehavior == "Manual"
+	if !isValidRelistBehavior {
+		errMsg := fmt.Sprintf("unknown relistBehavior found: %v", spec.RelistBehavior)


"relist behavior must be "Manual" or "Duration""

Or use field.NotSupported which takes a []string of valid values.

pmorie · 2017-09-14T00:09:37Z

pkg/apis/servicecatalog/validation/broker.go

+				"brokers must have a relistBehavior"))
+	}
+
+	isValidRelistBehavior := spec.RelistBehavior == "Duration" ||


use the constants from API types

pmorie · 2017-09-14T00:10:20Z

pkg/apis/servicecatalog/validation/broker.go

+				field.Required(fldPath.Child("relistDuration"), "relistDuration must be set if behavior is set to Duration"),
+			)
+		}
+		if spec.RelistRequests < 0 {


I think you always want to apply this validation, even if the behavior is manual

pmorie · 2017-09-14T00:10:51Z

pkg/apis/servicecatalog/validation/broker.go

+	}
+
+	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorDuration {
+		if spec.RelistDuration == nil {


looks like this is a duplicate of :124, and looks like this can be removed now, see comment about non-negative validation

pmorie · 2017-09-14T00:11:40Z

pkg/apis/servicecatalog/validation/broker_test.go

+				},
+			},
+			valid: false,
+		},


need test for negative relistRequests

pmorie · 2017-09-14T00:12:12Z

pkg/controller/controller_broker_test.go

 			reconcile: true,
 		},
+		{
+			name: "ready, duration behavior, nil duration",


nilebox

Generally looks good, but needs code cleanup, more tests and minor fixes (see comments).

nilebox · 2017-09-14T05:12:54Z

pkg/apis/servicecatalog/v1alpha1/defaults.go

 )

 func addDefaultingFuncs(scheme *runtime.Scheme) error {
 	return RegisterDefaults(scheme)
 }

+func SetDefaults_ServiceBrokerSpec(spec *ServiceBrokerSpec) {
+	isValidRelistBehavior := spec.RelistBehavior == "Duration" ||


Reference ServiceBrokerRelistBehaviorDuration and ServiceBrokerRelistBehaviorManual constants instead of inline strings?

This code shouldn't be there at all, see below.

nilebox · 2017-09-14T05:14:21Z

pkg/apis/servicecatalog/v1alpha1/defaults.go

+	isValidRelistBehavior := spec.RelistBehavior == "Duration" ||
+		spec.RelistBehavior == "Manual"
+
+	if !isValidRelistBehavior {


Defaults shouldn't handle invalid values, they handle missing values.
Hence, instead of the check above you should check for empty string here.

nilebox · 2017-09-14T05:24:28Z

pkg/apis/servicecatalog/v1alpha1/defaults_test.go

+			)
+		}
+
+		if tc.duration == nil && actualSpec.RelistDuration == nil {


@eriknelson yes, it should be enforced to nil (and checked at the validation stage). similar to union types, where only one branch can be initialized at a time.

nilebox · 2017-09-14T05:26:05Z

pkg/apis/servicecatalog/validation/broker.go

+		)
+	}
+
+	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorDuration && spec.RelistDuration == nil {


as mentioned in the other comments thread, you should also check that RelistDuration is nil when RelistBehavior != Duration

nilebox · 2017-09-14T05:26:44Z

pkg/apis/servicecatalog/validation/broker_test.go

@@ -40,7 +41,9 @@ func TestValidateServiceBroker(t *testing.T) {
 					Name: "test-broker",
 				},
 				Spec: servicecatalog.ServiceBrokerSpec{
-					URL: "http://example.com",
+					URL:            "http://example.com",
+					RelistBehavior: "Duration",


use constant from types (here and below).

nilebox · 2017-09-14T05:30:21Z

pkg/apis/servicecatalog/validation/broker_test.go

-					URL: "http://example.com",
+					URL:            "http://example.com",
+					RelistBehavior: "Duration",
+					RelistDuration: &metav1.Duration{15 & time.Minute},


should be * instead of &, you accidentally perform binary AND operation. here and below.

Ah, thank you. Not at all what I intended.

nilebox · 2017-09-14T05:33:53Z

pkg/storage/tpr/storage_interface_test.go

@@ -140,7 +141,9 @@ func TestCreateWithNoNamespace(t *testing.T) {
 	inputServiceBroker := &sc.ServiceBroker{
 		ObjectMeta: metav1.ObjectMeta{Name: name},
 		Spec: sc.ServiceBrokerSpec{
-			URL: "http://my-awesome-broker.io",
+			URL:            "http://my-awesome-broker.io",
+			RelistBehavior: "Duration",


use constant

nilebox · 2017-09-14T05:35:20Z

pkg/storage/tpr/storage_interface_test.go

-			URL: "http://my-awesome-broker.io",
+			URL:            "http://my-awesome-broker.io",
+			RelistBehavior: "Duration",
+			RelistDuration: &metav1.Duration{15 * time.Minute},


What are we trying to check there by setting those fields? If we want to make sure that serialization and persistence works correctly, you should set values different from default.

* NOTE: May not be correct, but was required for passing the tests

As written, was always setting Duration to 15m0s when Behavior was set to Duration

* Log error and return false from shouldReconcileServiceBroker when RelistDuration is set to nil despite a Duration behavior

* Set default RelistBehavior only in the absence of a value * Error messages tweaks * Use correct Behavior constants from types rather than inline strings * Validate nil RelistDuration when RelistBehavior is set to Manual * Remove redundant validation * Fix accidental binary AND * Test negative value for RelistRequests fails validation

eriknelson · 2017-09-14T18:41:28Z

@nilebox @pmorie think I should have addressed your comments in the last couple commits, also rebased and repaired conflicts. Let me know if I missed something, thank you for the reviews!

nilebox

@eriknelson see a couple more nits about constants. close to LGTM.

nilebox · 2017-09-15T00:01:12Z

pkg/controller/controller_broker_test.go

+			name: "ready, duration behavior, nil duration",
+			broker: func() *v1alpha1.ServiceBroker {
+				broker := getTestServiceBrokerWithStatus(v1alpha1.ConditionTrue)
+				broker.Spec.RelistBehavior = "Duration"


nilebox · 2017-09-15T00:01:22Z

pkg/apis/servicecatalog/testing/fuzzer.go

@@ -192,6 +193,11 @@ func FuzzerFor(t *testing.T, version schema.GroupVersion, src rand.Source) *fuzz
 			// Set the bytes field on the RawExtension
 			r.Raw = bytes
 		},
+		func(bs *servicecatalog.ServiceBrokerSpec, c fuzz.Continue) {
+			c.FuzzNoCustom(bs)
+			bs.RelistBehavior = "Duration"


nilebox · 2017-09-15T00:02:05Z

pkg/controller/controller_broker_test.go

+			name: "ready, manual behavior",
+			broker: func() *v1alpha1.ServiceBroker {
+				broker := getTestServiceBrokerWithStatus(v1alpha1.ConditionTrue)
+				broker.Spec.RelistBehavior = "Manual"


eriknelson · 2017-09-15T13:50:48Z

@nilebox think I got the last of them. For my edification, I had someone suggest it might be preferred to not use the consts from types in tests in case the values change? I'm probably misunderstanding that, but are there any cases where an inline value might be preferred?

nilebox · 2017-09-15T21:53:14Z

@eriknelson if you want to ensure that the constant hasn't changed, I would write a specific test for it. Might be something as trivial as

if ServiceBrokerRelistBehaviorDuration != "Duration" {
  t.Fail();
}

RelistBehavior is effectively a poor man's enum type (Go lacks support for enumeration types), so I don't see any other case where inline string is preferable - even if you want to avoid extra dependency I would still copy all the constants and used them.

Unfortunately, Kubernetes code is full of inline hardcoded strings, and it makes it hard to catch all cases while refactoring. So I personally prefer using constants where it makes sense.

pmorie

Just a couple nits, and also:

make sure godoc comments end in a period
make sure godoc comments are wrapped at 80 cols

pmorie · 2017-09-18T14:12:36Z

pkg/apis/servicecatalog/validation/broker.go

+	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorManual && spec.RelistDuration != nil {
+		allErrs = append(
+			allErrs,
+			field.Required(fldPath.Child("relistDuration"), "relistDuration must not be set if behavior is set to Manual"),


s/behavior/relistBehavior/

pmorie · 2017-09-18T14:12:47Z

pkg/apis/servicecatalog/validation/broker.go

+	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorDuration && spec.RelistDuration == nil {
+		allErrs = append(
+			allErrs,
+			field.Required(fldPath.Child("relistDuration"), "relistDuration must be set if behavior is set to Duration"),


Looks like this still needs to be done

pmorie · 2017-09-18T19:50:22Z

/test

pmorie · 2017-09-19T14:21:24Z

LGTM

* Base commit * Test updates for passing * storage_interface_test updates * NOTE: May not be correct, but was required for passing the tests * Set default Duration in fuzzer * RelistDuration + defaults and tests * Generated code * Initial controller-manager changes * Fix bug in the defaults with Duration As written, was always setting Duration to 15m0s when Behavior was set to Duration * PR feedback items + updated tests * Generated feedback * Newline separate fields * Defualt RelistDuration only when set to Duration * Handle RelistDuration nil when set to Duration * Log error and return false from shouldReconcileServiceBroker when RelistDuration is set to nil despite a Duration behavior * Comment updates, spacing and content * Validation and default tests * Bump nil RelistDuration log to error * Add controller_broker tests to trigger new paths * Remove todos * Fix test names * Minor spacing * Additional tests and feedback response * Add comment for non-negative RelistRequests * PR feedback items * Set default RelistBehavior only in the absence of a value * Error messages tweaks * Use correct Behavior constants from types rather than inline strings * Validate nil RelistDuration when RelistBehavior is set to Manual * Remove redundant validation * Fix accidental binary AND * Test negative value for RelistRequests fails validation * Use non-default vals to test storage serialization * Add updated types.generated.go * Fix linter complaints * Prune last of the inline str behaviors * Error message update based on feedback * Format API field godocs appropriately

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 31, 2017

eriknelson force-pushed the relist branch 2 times, most recently from eb4a4ce to 1b96007 Compare September 6, 2017 17:07

pmorie reviewed Sep 8, 2017

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2017

pmorie reviewed Sep 12, 2017

View reviewed changes

pmorie reviewed Sep 13, 2017

View reviewed changes

eriknelson changed the title ~~[WIP] broker relist~~ Broker Relist Sep 13, 2017

jboyd01 reviewed Sep 13, 2017

View reviewed changes

jboyd01 approved these changes Sep 13, 2017

View reviewed changes

staebler suggested changes Sep 13, 2017

View reviewed changes

pmorie reviewed Sep 14, 2017

View reviewed changes

nilebox suggested changes Sep 14, 2017

View reviewed changes

eriknelson added 13 commits September 14, 2017 14:36

Base commit

f7bdbba

Test updates for passing

5e9f532

storage_interface_test updates

3499b57

* NOTE: May not be correct, but was required for passing the tests

Set default Duration in fuzzer

de5ccfd

RelistDuration + defaults and tests

87720b6

Generated code

9d7c0a9

Initial controller-manager changes

780f836

Fix bug in the defaults with Duration

def66d1

As written, was always setting Duration to 15m0s when Behavior was set to Duration

PR feedback items + updated tests

8180f55

Generated feedback

5f95a3b

Newline separate fields

3d97fc1

Defualt RelistDuration only when set to Duration

6bdce0a

Handle RelistDuration nil when set to Duration

196d959

* Log error and return false from shouldReconcileServiceBroker when RelistDuration is set to nil despite a Duration behavior

eriknelson added 11 commits September 14, 2017 14:37

Validation and default tests

0bff1de

Bump nil RelistDuration log to error

0215e51

Add controller_broker tests to trigger new paths

f904b33

Remove todos

4aed37c

Fix test names

05878d1

Minor spacing

b36a217

Additional tests and feedback response

935c206

Add comment for non-negative RelistRequests

728e775

Use non-default vals to test storage serialization

5ffd10f

Add updated types.generated.go

af29db8

eriknelson force-pushed the relist branch from 87da133 to af29db8 Compare September 14, 2017 18:40

Fix linter complaints

9d42d9b

nilebox reviewed Sep 15, 2017

View reviewed changes

Prune last of the inline str behaviors

7fdc37e

nilebox approved these changes Sep 15, 2017

View reviewed changes

nilebox added the LGTM1 label Sep 15, 2017

pmorie reviewed Sep 18, 2017

View reviewed changes

eriknelson added 2 commits September 18, 2017 10:18

Error message update based on feedback

2a76c50

Format API field godocs appropriately

0d8c01a

pmorie added the LGTM2 label Sep 19, 2017

pmorie merged commit e9328d3 into kubernetes-retired:master Sep 19, 2017

pmorie added this to the 0.0.21 milestone Sep 19, 2017

pmorie mentioned this pull request Sep 19, 2017

Fix lint errors and regenerate openapi #1238

Merged

		@@ -99,6 +99,12 @@ func validateServiceBrokerSpec(spec sc.ServiceBrokerSpec, fldPath field.Path)
		)
		}

Broker Relist #1183

Broker Relist #1183

Conversation

eriknelson commented Aug 31, 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

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

eriknelson Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

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

jboyd01 left a comment

Choose a reason for hiding this comment

staebler left a comment • edited Loading

Choose a reason for hiding this comment

staebler 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

eriknelson Sep 13, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson commented Sep 13, 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

nilebox left a comment

eriknelson Sep 12, 2017 •

edited

Loading

staebler left a comment •

edited

Loading

eriknelson Sep 13, 2017 •

edited

Loading