Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1498618 - Support bind parameters. #467

Merged
merged 1 commit into from Oct 5, 2017
Merged

Bug 1498618 - Support bind parameters. #467

merged 1 commit into from Oct 5, 2017

Conversation

cfchase
Copy link
Contributor

@cfchase cfchase commented Oct 3, 2017

Support bind parameters. Proposal included.

@cfchase cfchase added the feature label Oct 3, 2017
@cfchase
Copy link
Contributor Author

cfchase commented Oct 3, 2017

(8) Bind Parameters

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done. clear proposal, code changes are reasonable, and the tests work. A++ will merge again.

* ASB Unit tests
* OpenShift UI form definition changes
* Experimental APB with async bind and passed parameters
* APB documentation changes (getting started / developer guide)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally the proposal is a separate PR done before the implementation :) But the proposal seems sane and it is very clear of your intentions. +1

Free bool `json:"free,omitempty"`
Bindable bool `json:"bindable,omitempty"`
Parameters []ParameterDescriptor `json:"parameters"`
BindParameters []ParameterDescriptor `json:"bind_parameters,omitempty" yaml:"bind_parameters,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

params["provision_params"] = *instance.Parameters
params := req.Parameters
if params == nil {
params = make(apb.Parameters)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -171,10 +200,44 @@ func getType(paramType string) schema.PrimitiveTypes {
return []schema.PrimitiveType{schema.UnspecifiedType}
}

func parametersToSchema(params []apb.ParameterDescriptor) Schema {
func parametersToSchema(plan apb.Plan) Schema {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sane change to parametersToSchema. 👍

@@ -76,18 +76,43 @@ var PlanParams = []apb.ParameterDescriptor{
},
}

var PlanBindParams = []apb.ParameterDescriptor{
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YES! to updating tests. 👏

@cfchase cfchase added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Oct 3, 2017
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@djzager
Copy link
Member

djzager commented Oct 3, 2017

Added the do-not-merge to hold onto this until after we have branched for 3.7 release.

@cfchase cfchase changed the title Support bind parameters. Bug 1498618 - Support bind parameters. Oct 4, 2017
@cfchase cfchase added bug and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. feature labels Oct 4, 2017
@jmrodri jmrodri merged commit 97250e6 into openshift:master Oct 5, 2017
@cfchase cfchase deleted the bind-params branch November 29, 2017 14:17
jianzhangbjz pushed a commit to jianzhangbjz/ansible-service-broker that referenced this pull request May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants