Bug 1605136 - allow empty string values for non-generated parameters by johnkim76 · Pull Request #21371 · openshift/origin

johnkim76 · 2018-10-27T02:44:08Z

allow empty string values to be set for non-generated parameters
ignores empty string values for generated parameters

Requires UI changes in the origin-web-catalog as discussed in BZ comment here

bparees · 2018-10-27T02:53:55Z

pkg/templateservicebroker/servicebroker/provision.go

-		secret.Data[k] = []byte(v)
+		for _, param := range template.Parameters {
+			if param.Name == k && ((v == "" && param.Generate == "") || v != "") {
+				secret.Data[k] = []byte(v)


have you confirmed that setting emptystring here ultimately results in the default value for the parameter being used during instantiation?

@bparees I was able to verify the following

if the default values were left alone, it's passed along as-is

if the default values were removed/deleted, the parameters were set to an empty value

empty values for the generated parameters were indeed generated, where it did not before.

However, I had difficulty getting any of the provisions to successfully complete with the cluster that @spadgett was nice enough to let me test with.

I'll change this PR as WIP, until I can fully verify a successful provision.

However, I had difficulty getting any of the provisions to successfully complete with the cluster that @spadgett was nice enough to let me test with.

I'll change this PR as WIP, until I can fully verify a successful provision.

It looks like react-jsonschema-form leaves out string values that are cleared by the user. The empty string isn't treated as an error for required properties in JSON schema, so they switched to undefined.

rjsf-team/react-jsonschema-form#442

I'll have to look at how to handle this from the client.

if the default values were removed/deleted, the parameters were set to an empty value

Just to clarify, if the parameter is missing entirely from the request, we'd still want to use the default value. We'd only want this behavior if it's explicitly set to "" (i.e., the user explicitly cleared the value in the web console form)

@bparees Does that make sense to you?

@johnkim76 can you please add unit test to exercise this?

@mfojtik will do

spadgett · 2018-10-27T13:02:32Z

@johnkim76 Can you give this PR a shot?

openshift/console#711

johnkim76 · 2018-10-28T13:47:52Z

@spadgett Thank you!. that console PR seems to work! I did a quick test on my own v3.9.49 cluster Tand the provision was successful.

I'll do more testing later today and remove the WIP.

mfojtik · 2018-10-29T11:45:45Z

pkg/templateservicebroker/servicebroker/provision.go

 	}

+	// allow empty string values for non-generated parameters.
+	// empty string values will be ignored for generated parameter/


nit: drop the trailing /

thank you, I will update

mfojtik · 2018-10-29T11:46:13Z

pkg/templateservicebroker/servicebroker/provision.go

 	for k, v := range preq.Parameters {
-		secret.Data[k] = []byte(v)
+		for _, param := range template.Parameters {
+			if param.Name == k && ((v == "" && param.Generate == "") || v != "") {


style: we generally use len(s) == 0 to check for an empty string

thank you, I'll fix

mfojtik · 2018-10-29T11:47:43Z

Before merging this, can you please add unit test that exercise this behavior? Other than small nits, this looks good to me.

johnkim76 · 2018-11-05T16:59:28Z

@mfojtik I've made the updates as requested

johnkim76 · 2018-11-09T15:41:41Z

/retest

…oker Requires openshift/origin#21371 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1605136

mfojtik · 2018-11-28T15:55:40Z

/lgtm

openshift-ci-robot · 2018-11-28T15:55:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnkim76, mfojtik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/OWNERS~~ [mfojtik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jmontleon · 2018-11-30T16:49:28Z

/test cmd

jmontleon · 2018-11-30T19:19:09Z

/test e2e-aws

jmontleon · 2018-11-30T19:19:18Z

/test e2e-gcp

jmontleon · 2018-11-30T19:36:30Z

/test e2e-aws

jmontleon · 2018-11-30T20:45:09Z

/test e2e-aws

johnkim76 · 2018-12-01T18:04:45Z

/test e2e-aws

openshift-bot · 2018-12-01T22:08:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-12-02T00:09:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 27, 2018

openshift-ci-robot requested review from csrwng and mfojtik October 27, 2018 02:44

johnkim76 assigned bparees and spadgett Oct 27, 2018

bparees reviewed Oct 27, 2018

View reviewed changes

johnkim76 changed the title ~~Bug 1605136 - allow empty string values for non-generated parameters~~ [WIP] Bug 1605136 - allow empty string values for non-generated parameters Oct 27, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2018

spadgett mentioned this pull request Oct 27, 2018

Bug 1605136 - Let users clear default fields with template service broker openshift/console#711

Merged

johnkim76 changed the title ~~[WIP] Bug 1605136 - allow empty string values for non-generated parameters~~ Bug 1605136 - allow empty string values for non-generated parameters Oct 29, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2018

mfojtik reviewed Oct 29, 2018

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 5, 2018

johnkim76 force-pushed the BZ1605136 branch from e6d74e4 to b8d73cb Compare November 5, 2018 16:56

johnkim76 force-pushed the BZ1605136 branch from ff8a1da to d22e7ff Compare November 8, 2018 19:35

jkim added 3 commits November 8, 2018 14:37

allow empty string values for non-generated parameters

f574e2f

replace empty string check with len()

f6f49c0

add unit test for ensureSecret()

b8df8c5

johnkim76 force-pushed the BZ1605136 branch from d22e7ff to b8df8c5 Compare November 8, 2018 19:37

spadgett added a commit to spadgett/console that referenced this pull request Nov 12, 2018

Bug 1605136 - Let users clear default fields with template service br…

604921c

…oker Requires openshift/origin#21371 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1605136

openshift-ci-robot assigned mfojtik Nov 28, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2018

openshift-merge-robot merged commit f049170 into openshift:master Dec 2, 2018

Conversation

johnkim76 commented Oct 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Oct 27, 2018

Uh oh!

johnkim76 commented Oct 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Oct 29, 2018

Uh oh!

johnkim76 commented Nov 5, 2018

Uh oh!

johnkim76 commented Nov 9, 2018

Uh oh!

mfojtik commented Nov 28, 2018

Uh oh!

openshift-ci-robot commented Nov 28, 2018

Uh oh!

jmontleon commented Nov 30, 2018

Uh oh!

jmontleon commented Nov 30, 2018

Uh oh!

jmontleon commented Nov 30, 2018

Uh oh!

jmontleon commented Nov 30, 2018

Uh oh!

jmontleon commented Nov 30, 2018

Uh oh!

johnkim76 commented Dec 1, 2018

Uh oh!

openshift-bot commented Dec 1, 2018

Uh oh!

openshift-bot commented Dec 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants