Multi-plan support #298

eriknelson · 2017-07-24T20:00:11Z

No description provided.

eriknelson · 2017-07-24T20:11:52Z

Going to fix the ol' linter

jmrodri

Having followed the discussion and the testing. I'll ack this, despite the mean sticker @eriknelson sent me.

shawn-hurley · 2017-07-24T20:05:48Z

pkg/handler/handler.go

@@ -174,14 +174,22 @@ func (h handler) deprovision(w http.ResponseWriter, r *http.Request, params map[
 		writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: "invalid instance_uuid"})
 		return
 	}
+
 	var async bool
 	queryparams := r.URL.Query()


So small nit, just because your editing this.

I would prefer to use the request.FormValue(). I personally think it is nicer and easier to use.

Much better, didn't know about this. TY @shawn-hurley

Logged an issue to update the request layer, #303

But only if they are coming in as a form. If they are json or coming in on the actual url hook. I'd be careful making that change blindly.

@jmrodri Are you satisfied with this after your investigation?

@eriknelson yes FormValue is ok. Weird but it seems to work.

shawn-hurley · 2017-07-24T20:12:04Z

pkg/registries/registry.go

+
+func validateSpecs(log *logging.Logger, inSpecs []*apb.Spec) []*apb.Spec {
+	var wg sync.WaitGroup
+	wg.Add(len(inSpecs))


I prefer to call the addition to the wait group as I call the go routines. To me, it makes it more clear what we are supposed to be waiting on. This is still really clear, just throwing it out there for the future.

I think I like saying "I expect this to run for every spec" up front.

@shawn-hurley I'd like to also talk about this fn. It's not in the scope of this PR, but where do we want to validate what we bring in, and how? There is probably room for the registry to hard vet loaded specs.

@shawn-hurley coming back to this now that the deprovision fire is (hopefully) out. I think we're talking about how the broker should validate Specs that come in, which IMO is outside the scope of this PR. I would like to lodge a card to consider spec validation explicitly, because it's something we need to address. The broker needs to defend against adapters sending busted Specs, and that's not in the domain of a Plans support feature.

shawn-hurley · 2017-07-24T20:14:49Z

pkg/registries/registry.go

@@ -61,7 +62,7 @@ func (r Registry) LoadSpecs() ([]*apb.Spec, int, error) {
 	r.log.Debug("Filter applied against registry: %s", r.config.Name)

 	if len(validNames) != 0 {
-		r.log.Debugf("Passing APBs:")
+		r.log.Debugf("APBs passing white/blacklist filter:")


shawn-hurley · 2017-07-24T20:34:21Z

pkg/registries/registry.go

+		go func(s *apb.Spec) {
+			ok, failReason := validateSpecPlans(s)
+			out <- resultT{ok, s, failReason}
+			wg.Done()


You should defer this, things could break if an unexpected panic happens.

eriknelson · 2017-07-25T02:16:34Z

Updated based on feedback.

eriknelson · 2017-07-25T02:54:44Z

Realizing this requires some decent testing changes. Working on that. Will continue to send @jmrodri rude stickers.

rthallisey · 2017-07-25T14:12:38Z

pkg/apb/types.go

@@ -39,9 +47,8 @@ type Spec struct {
 	Metadata    map[string]interface{} `json:"metadata,omitempty"`

 	// required, optional, unsupported


This comment looks out of place now.

rthallisey · 2017-07-25T14:24:49Z

pkg/broker/broker.go

+			"PlanID from provision request is blank. " +
+				"Provision requests must specify PlanIDs"
+		a.log.Error(errMsg)
+		return nil, errors.New(errMsg)


This is going to print errMsg twice. Here's where err gets passed to: https://github.com/openshift/ansible-service-broker/blob/master/pkg/broker/provision_job.go#L50. Same thing for all the custom error messages below.

Because we have the caller capturing and printing error everywhere, I think the only places it makes sense to print a custom error message is if you return nil, err. That way we print both the custom errMsg and the actually error.

rthallisey · 2017-07-25T15:52:12Z

closes #297

* Initial buildable refactoring * Add validations * Accept and require plan_id for all actions * Feedback * Linter fixes * Update test to be multi-plan compliant * Updates based on Ryan's feedback

eriknelson changed the title ~~Multi-Plain~~ Multi-plan support Jul 24, 2017

eriknelson requested review from jmrodri, shawn-hurley and rthallisey July 24, 2017 20:00

eriknelson mentioned this pull request Jul 24, 2017

Update all APBs to match proper format for multi-plans in the broker fusor/apb-examples#96

Merged

jmrodri approved these changes Jul 24, 2017

View reviewed changes

shawn-hurley approved these changes Jul 24, 2017

View reviewed changes

shawn-hurley reviewed Jul 24, 2017

View reviewed changes

eriknelson changed the title ~~Multi-plan support~~ [DO NOT MERGE] Multi-plan support Jul 25, 2017

eriknelson mentioned this pull request Jul 25, 2017

spec logs spec ids instead of useful info #297

Closed

rthallisey reviewed Jul 25, 2017

View reviewed changes

eriknelson added 6 commits July 28, 2017 14:39

Initial buildable refactoring

d194e1c

Add validations

9bb58fd

Accept and require plan_id for all actions

b9e7119

Feedback

284e5de

Linter fixes

c028877

Update test to be multi-plan compliant

f28a74d

eriknelson force-pushed the plans-support branch from 8ee318c to f28a74d Compare July 28, 2017 18:39

eriknelson changed the title ~~[DO NOT MERGE] Multi-plan support~~ Multi-plan support Jul 28, 2017

Updates based on Ryan's feedback

4878304

rthallisey approved these changes Jul 28, 2017

View reviewed changes

eriknelson merged commit 96b554c into openshift:master Jul 28, 2017

Multi-plan support #298

Multi-plan support #298

eriknelson commented Jul 24, 2017

eriknelson commented Jul 24, 2017

jmrodri left a comment •

edited

shawn-hurley Jul 24, 2017

eriknelson Jul 25, 2017

eriknelson Jul 25, 2017

jmrodri Jul 25, 2017

eriknelson Jul 27, 2017

jmrodri Jul 28, 2017

shawn-hurley Jul 24, 2017

eriknelson Jul 25, 2017

eriknelson Jul 25, 2017 •

edited

eriknelson Jul 27, 2017 •

edited

shawn-hurley Jul 24, 2017

shawn-hurley Jul 24, 2017

eriknelson commented Jul 25, 2017

eriknelson commented Jul 25, 2017 •

edited

rthallisey Jul 25, 2017

rthallisey Jul 25, 2017

rthallisey commented Jul 25, 2017 •

edited

		@@ -39,9 +47,8 @@ type Spec struct {
		Metadata map[string]interface{} `json:"metadata,omitempty"`

		// required, optional, unsupported

Multi-plan support #298

Multi-plan support #298

Conversation

eriknelson commented Jul 24, 2017

eriknelson commented Jul 24, 2017

jmrodri left a comment • edited

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 Jul 25, 2017 • edited

Choose a reason for hiding this comment

eriknelson Jul 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson commented Jul 25, 2017

eriknelson commented Jul 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey commented Jul 25, 2017 • edited

jmrodri left a comment •

edited

eriknelson Jul 25, 2017 •

edited

eriknelson Jul 27, 2017 •

edited

eriknelson commented Jul 25, 2017 •

edited

rthallisey commented Jul 25, 2017 •

edited