#1147 Store in-progress and external resource properties state #1250

staebler · 2017-09-21T14:40:50Z

Closes #1147.
Closes #882.

pmorie

Looking pretty solid - I have the most concerns about putting the status validations into update validation methods.

pmorie · 2017-09-25T21:27:04Z

pkg/apis/servicecatalog/types.go

+// ServiceInstancePropertiesState is the state of a ServiceInstance that
+// the ServiceBroker knows about.
+type ServiceInstancePropertiesState struct {
+	// PlanName is the name of the plan that the broker knows this


We should probably clarify that this is the human readable plan name from the OSB API.

pmorie · 2017-09-25T21:30:18Z

pkg/apis/servicecatalog/types.go

+
+	// InProgressProperties is the properties state for which there is a
+	// provision or update action in progress.
+	InProgressProperties *ServiceInstancePropertiesState


You might clarify that it will not be set for deprovision operations

pmorie · 2017-09-25T21:33:06Z

pkg/apis/servicecatalog/validation/binding.go

@@ -37,7 +44,12 @@ func internalValidateServiceInstanceCredential(binding *sc.ServiceInstanceCreden
 		validateServiceInstanceCredentialName,
 		field.NewPath("metadata"))...)
 	allErrs = append(allErrs, validateServiceInstanceCredentialSpec(&binding.Spec, field.NewPath("Spec"), create)...)
-
+	allErrs = append(allErrs, validateServiceInstanceCredentialStatus(&binding.Status, field.NewPath("Status"), create)...)


I think this should be in an update validation. For create current operation shouldn't be set at all.

always call field.NewPath with names starting with lowercase

pmorie · 2017-09-25T21:40:29Z

pkg/apis/servicecatalog/validation/binding.go

+		allErrs = append(allErrs, field.NotSupported(fldPath.Child("currentOperation"), status.CurrentOperation, validValues))
+	}
+
+	switch status.CurrentOperation {


Is this switch statement necessary at all?

pmorie · 2017-09-25T21:40:37Z

pkg/apis/servicecatalog/validation/binding.go

+			validValues[i] = string(operation)
+			i++
+		}
+		allErrs = append(allErrs, field.NotSupported(fldPath.Child("currentOperation"), status.CurrentOperation, validValues))


I think this should be field.Invalid

I thought this was exactly what field.NotSupported was intended for. We have enumerated values and we are enforcing that the field is one of those values.

With that being said, I don't really have a dog in the fight. I am fine either way. If you feel that it should be field.Invalid, I have no problem changing it.

pmorie · 2017-09-25T21:52:12Z

pkg/apis/servicecatalog/validation/instance.go

@@ -39,6 +47,11 @@ func internalValidateServiceInstance(instance *sc.ServiceInstance, create bool)
 		field.NewPath("metadata"))...)
 	allErrs = append(allErrs, validateServiceInstanceSpec(&instance.Spec, field.NewPath("Spec"), create)...)
 	allErrs = append(allErrs, validateServiceInstanceStatus(&instance.Status, field.NewPath("Status"), create)...)
+	if instance.Status.ReconciledGeneration == instance.Generation {


I think this should go into a separate update validation.

pmorie · 2017-09-25T21:53:54Z

pkg/apis/servicecatalog/validation/instance.go

-	// ongoing
-	if spec.AsyncOpInProgress {
-		for _, c := range spec.Conditions {
+	if !validServiceInstanceOperations[status.CurrentOperation] {


Why not just have a string with the current valid values instead of building it each time?

pmorie · 2017-09-25T22:07:42Z

pkg/controller/controller_binding.go

+		}
+
+		var rawParametersWithRedaction *runtime.RawExtension
+		if parametersWithSecretsRedacted != nil {


seems like this can be part of the above if statement.

We should validate that the parameters and redactedParameters are either both not-nil or both nil.

pmorie · 2017-09-25T22:09:35Z

pkg/controller/controller_binding.go

@@ -370,6 +422,8 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic
 			return err
 		}

+		toUpdate.Status.ExternalProperties = toUpdate.Status.InProgressProperties


This method is long enough that a quality saying "Now we care handling the success case" might be helpful.

pmorie · 2017-09-25T22:15:05Z

pkg/controller/controller_test.go

+	assertServiceInstanceCredentialExternalPropertiesUnchanged(t, obj, originalBinding)
+}
+
+func assertServiceInstanceCredentialInProgressPropertiesNil(t *testing.T, obj runtime.Object) {


I think it would be good to have explanatory godoc for these methods but I do not want to block this PR on it. Will you promise to do it in a follow-up?

I promise. I will create an issue for it to remind myself.

vaikas · 2017-09-27T00:57:27Z

pkg/apis/servicecatalog/types.go

+	// Deprovision, this will be nil.
+	InProgressProperties *ServiceInstancePropertiesState
+
+	// ExternalProperties is the properites state of the ServiceInstance which the


nit: properties

vaikas · 2017-09-27T00:58:36Z

pkg/apis/servicecatalog/types.go

+	// PlanName is the name of the plan that the broker knows this
+	// ServiceInstance to be on. This is the human readable plan name from the
+	// OSB API.
+	PlanName string


I'd prefer to call it ExternalServicePlanName just like what we decided to call it on ServiceInstance.Spec for consistency.

vaikas · 2017-09-27T00:59:07Z

pkg/apis/servicecatalog/v1alpha1/types.go

+	// PlanName is the name of the plan that the broker knows this
+	// ServiceInstance to be on. This is the human readable plan name from the
+	// OSB API.
+	PlanName string `json:"planName"`


ExternalServicePlanName for consistency

vaikas · 2017-09-27T22:36:15Z

Looks good just needs a rebase.

pmorie · 2017-09-28T03:54:17Z

pkg/apis/servicecatalog/types.go

+	// ExternalServicePlanName is the name of the plan that the broker knows this
+	// ServiceInstance to be on. This is the human readable plan name from the
+	// OSB API.
+	ExternalServicePlanName string `json:"externalServicePlanName"`


no json tags on internal

kibbles-n-bytes

LGTM, though please try to keep the scope of PRs as focused as possible! This is really two changes in one.

vaikas · 2017-09-28T20:54:59Z

I'll merge after rebase & tests pass.

…tus.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 21, 2017

staebler force-pushed the 1147-properties-by-view branch 3 times, most recently from be82b70 to 6a75d82 Compare September 25, 2017 21:22

pmorie reviewed Sep 25, 2017

View reviewed changes

staebler changed the title ~~WIP: #1147 Store in-progress and external resource properties state~~ #1147 Store in-progress and external resource properties state Sep 26, 2017

pmorie added the LGTM1 label Sep 26, 2017

staebler force-pushed the 1147-properties-by-view branch from bcce810 to fabd5a6 Compare September 26, 2017 20:06

vaikas reviewed Sep 27, 2017

View reviewed changes

staebler force-pushed the 1147-properties-by-view branch from fabd5a6 to f552d27 Compare September 27, 2017 16:05

staebler force-pushed the 1147-properties-by-view branch from f552d27 to 046f7ef Compare September 28, 2017 00:02

pmorie reviewed Sep 28, 2017

View reviewed changes

staebler force-pushed the 1147-properties-by-view branch from c4307a6 to 603d8db Compare September 28, 2017 14:46

staebler mentioned this pull request Sep 28, 2017

#1148: Support updates to Instances #1289

Merged

kibbles-n-bytes approved these changes Sep 28, 2017

View reviewed changes

vaikas added the LGTM2 label Sep 28, 2017

Store in-progress and external properties in instance and binding sta…

aa358be

…tus.

staebler force-pushed the 1147-properties-by-view branch from 603d8db to aa358be Compare September 28, 2017 21:12

pmorie merged commit 0c0035c into kubernetes-retired:master Sep 28, 2017

MHBauer mentioned this pull request Jul 9, 2018

Add godoc comments to the assert***InProgressProperties*** and assert**ExternalProperties*** test methods in controller_test.go. #1276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1147 Store in-progress and external resource properties state #1250

#1147 Store in-progress and external resource properties state #1250

staebler commented Sep 21, 2017 •

edited

Loading

pmorie left a comment

pmorie Sep 25, 2017

pmorie Sep 25, 2017

pmorie Sep 25, 2017

pmorie Sep 25, 2017

pmorie Sep 25, 2017

pmorie Sep 25, 2017

staebler Sep 26, 2017

pmorie Sep 25, 2017

pmorie Sep 25, 2017

pmorie Sep 25, 2017

pmorie Sep 25, 2017

pmorie Sep 25, 2017

staebler Sep 26, 2017

vaikas Sep 27, 2017

vaikas Sep 27, 2017

pmorie Sep 27, 2017

vaikas Sep 27, 2017

vaikas commented Sep 27, 2017

pmorie Sep 28, 2017

kibbles-n-bytes left a comment

vaikas commented Sep 28, 2017

#1147 Store in-progress and external resource properties state #1250

#1147 Store in-progress and external resource properties state #1250

Conversation

staebler commented Sep 21, 2017 • edited Loading

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

vaikas commented Sep 27, 2017

Choose a reason for hiding this comment

kibbles-n-bytes left a comment

Choose a reason for hiding this comment

vaikas commented Sep 28, 2017

staebler commented Sep 21, 2017 •

edited

Loading