Support typed parameter values #1376

nfnt · 2020-03-02T13:12:56Z

What this PR does / why we need it:
This introduces the 'string', 'array', and 'map' types for parameter values. By default, the 'string' type is used. This type is used for a simple value. The types 'array' and 'map' can be used to describe lists and dictonaries. Their values are input using YAML.

Fixes #1349

This introduces the 'string', 'list', and 'dict' types for parameter values. By default, the 'string' type is used. This type is used for a simple value. The types 'list' and 'dict' can be used to describe lists and dictonaries. Their values are input using YAML. Signed-off-by: Jan Schlicht <jan@d2iq.com>

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt · 2020-03-02T14:58:46Z

I don't really know what parse-param-operator-in-operator is actually testing, but it looks like my refactoring of paramsMap fixed a bug. This bug caused this test to pass, even though it shouldn't. I'm fixing the test.

Signed-off-by: Jan Schlicht <jan@d2iq.com>

kensipe

nice work so far... some nits

I strongly encourage that we move functions that are not "kudo" features out of kudo package. (I realize there is some examples that deviate)

Also... I'm assuming that maps and lists (our new types) are restricted to values which are strings. It should be great to have the documented... but also tests that show incorrect cases failing... such as a map of lists or a map of maps.

pkg/util/kudo/convert_types.go

test/integration/parse-param-operator-in-operator/01-assert.yaml

kensipe · 2020-03-02T22:36:35Z

pkg/engine/task/task_toggle.go

+
+	stringVal, ok := val.(string)
+	if !ok {
+		return task, fmt.Errorf("value of parameter %s isn't a string", tt.Parameter)


shouldn't there be an error on the use of a toggle param that is NOT a string... which is technically a bool ?
It seems like we are missing that..

Isn't this what this check does? We try to cast to a string, if it's !ok we error out?

Isn't that done by the line below? If stringVal is neither "true" nor "false", strconv.ParseBool will return an error.

The error message is somewhat confusing, we expect a string/boolean there. Maybe smth. like:

Suggested change

return task, fmt.Errorf("value of parameter %s isn't a string", tt.Parameter)

return task, fmt.Errorf("value of parameter %s must be either 'true' or 'false'", tt.Parameter)

wdyt?

I disagree, because this is checked in the line below when parsing the string to bool. This line is checking that typeof(interface) == string, not the content of the string.

If it's not a string it can't be true or false which is what we expect.

Booleans can also be represented by a boolean or integer type. Also strconv.ParseBool accepts 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False.
At this point it's only clear whether the input is a string or not, the content of the input isn't known yet.

pkg/controller/instance/instance_controller.go

pkg/apis/kudo/v1beta1/operatorversion_types.go

pkg/controller/instance/instance_controller.go

pkg/apis/kudo/v1beta1/operatorversion_types.go

pkg/controller/instance/instance_controller.go

pkg/kudoctl/packages/verifier/template/verify_render_test.go

test/integration/parse-param-operator-in-operator/01-assert.yaml

* Moves type conversion functions to `convert` package * Renames parameter types * Different value type switch Signed-off-by: Jan Schlicht <jan@d2iq.com>

Signed-off-by: Jan Schlicht <jan@d2iq.com>

pkg/kudoctl/packages/verifier/template/verify_render.go

Signed-off-by: Jan Schlicht <jan@d2iq.com>

ANeumann82

Nice work! Some naming requests, and the integration test could be improved, but this is getting close!

pkg/util/convert/convert.go

test/integration/parameter-types/01-install.yaml

Signed-off-by: Jan Schlicht <jan@d2iq.com>

kensipe

man I love the single document params approach so much better!

A few minor nits... the only real blocker for me is around ToYAMLMap and ToYAMLArray

nice work!

pkg/apis/kudo/v1beta1/instance_types_helpers.go

pkg/util/convert/convert.go

pkg/controller/instance/instance_controller.go

pkg/kudoctl/packages/types.go

pkg/util/convert/convert.go

Signed-off-by: Jan Schlicht <jan@d2iq.com>

kensipe

a couple of comments or nits... nothing blocking. nice work 👏

pkg/util/convert/convert.go

pkg/kudoctl/packages/types.go

Signed-off-by: Jan Schlicht <jan@d2iq.com> Co-Authored-By: Ken Sipe <kensipe@gmail.com>

Signed-off-by: Jan Schlicht <jan@d2iq.com>

The change request has been handled... feel free to reject again if appropriate

pkg/kudoctl/packages/types.go

pkg/kudoctl/packages/verifier/template/verify_render.go

pkg/util/convert/convert.go

pkg/util/convert/convert_test.go

test/integration/parameter-types-operator/operator/templates/deployment.yaml

test/integration/parameter-types/01-install.yaml

Signed-off-by: Jan Schlicht <jan@d2iq.com>

This introduces the 'string', 'array', and 'masp' types for parameter values. By default, the 'string' type is used. This type is used for a simple value. The types 'array' and 'map' can be used to describe lists and dictionaries. Their values are input using YAML. Co-Authored-By: Ken Sipe <kensipe@gmail.com> Signed-off-by: Jan Schlicht <jan@d2iq.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>

nfnt added the kep/24 Map and array parameter types label Mar 2, 2020

nfnt self-assigned this Mar 2, 2020

Jan Schlicht added 2 commits March 2, 2020 14:25

Update golden files

effeed1

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Fix a typo and document conversion functions

61c93dd

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Fix 'parse-param-operator-in-operator'

3e7da70

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt marked this pull request as ready for review March 2, 2020 15:08

nfnt requested review from alenkacz, gerred, kensipe and zen-dog as code owners March 2, 2020 15:08

nfnt requested review from ANeumann82 and porridge March 2, 2020 15:09

nfnt added the release/highlight This PR is a highlight for the next release label Mar 2, 2020

kensipe requested changes Mar 2, 2020

View reviewed changes

kensipe reviewed Mar 2, 2020

View reviewed changes

pkg/apis/kudo/v1beta1/operatorversion_types.go Outdated Show resolved Hide resolved

porridge previously requested changes Mar 3, 2020

View reviewed changes

Jan Schlicht added 5 commits March 3, 2020 13:04

Address review comments

bb35f03

* Moves type conversion functions to `convert` package * Renames parameter types * Different value type switch Signed-off-by: Jan Schlicht <jan@d2iq.com>

Update generated files

13999e7

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Golden files have to be updated as well

ac5703c

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Fix a compilation error

b51fb1a

Signed-off-by: Jan Schlicht <jan@d2iq.com>

object => map

46072b8

Signed-off-by: Jan Schlicht <jan@d2iq.com>

ANeumann82 reviewed Mar 3, 2020

View reviewed changes

pkg/kudoctl/packages/verifier/template/verify_render.go Outdated Show resolved Hide resolved

Jan Schlicht added 5 commits March 4, 2020 09:53

Add 'ParamValue' to 'convert' package

5d00d42

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Improve readability of 'paramsMap'

77a6eaa

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Reformat comment

bd94729

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Update golden files

73f222a

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Rename 'type' to 'value-type' to avoid bugs in JSON Schema validation

48ba154

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt force-pushed the nfnt/typed-params branch from 20b53dd to 48ba154 Compare March 4, 2020 10:13

ANeumann82 requested changes Mar 4, 2020

View reviewed changes

pkg/util/convert/convert.go Outdated Show resolved Hide resolved

pkg/util/convert/convert.go Outdated Show resolved Hide resolved

test/integration/parameter-types/01-install.yaml Show resolved Hide resolved

Jan Schlicht added 2 commits March 4, 2020 16:02

Rename conversion functions

3505640

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Integration test for typed parameter values in operator package

9b0bbbf

Signed-off-by: Jan Schlicht <jan@d2iq.com>

ANeumann82 approved these changes Mar 4, 2020

View reviewed changes

Require an older version of Kubernetes

ea8b0c7

Signed-off-by: Jan Schlicht <jan@d2iq.com>

kensipe requested changes Mar 4, 2020

View reviewed changes

kensipe reviewed Mar 4, 2020

View reviewed changes

pkg/controller/instance/instance_controller.go Outdated Show resolved Hide resolved

kensipe reviewed Mar 4, 2020

View reviewed changes

pkg/controller/instance/instance_controller.go Outdated Show resolved Hide resolved

kensipe reviewed Mar 4, 2020

View reviewed changes

pkg/kudoctl/packages/types.go Outdated Show resolved Hide resolved

kensipe reviewed Mar 4, 2020

View reviewed changes

pkg/util/convert/convert.go Outdated Show resolved Hide resolved

Jan Schlicht added 3 commits March 5, 2020 10:35

Rename convert functions and parameters

a10760a

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Rename variable

f7c7cbf

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Fix a compilation error

ee4a316

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt mentioned this pull request Mar 5, 2020

Document parameter value types kudobuilder/kudo.dev#215

Merged

kensipe approved these changes Mar 5, 2020

View reviewed changes

pkg/util/convert/convert.go Outdated Show resolved Hide resolved

pkg/kudoctl/packages/types.go Outdated Show resolved Hide resolved

Jan Schlicht and others added 2 commits March 5, 2020 15:38

Update pkg/util/convert/convert.go

327e06a

Signed-off-by: Jan Schlicht <jan@d2iq.com> Co-Authored-By: Ken Sipe <kensipe@gmail.com>

Change function docs

82a47c2

Signed-off-by: Jan Schlicht <jan@d2iq.com>

zen-dog reviewed Mar 6, 2020

View reviewed changes

pkg/kudoctl/packages/types.go Outdated Show resolved Hide resolved

porridge requested changes Mar 6, 2020

View reviewed changes

Jan Schlicht added 2 commits March 6, 2020 12:15

Refactor package type converters into separate 'convert' package

cb920eb

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Improve error messages

91e69dc

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt requested a review from porridge March 6, 2020 14:12

porridge approved these changes Mar 6, 2020

View reviewed changes

nfnt merged commit 51bf78c into master Mar 6, 2020

nfnt deleted the nfnt/typed-params branch March 6, 2020 14:44

kensipe mentioned this pull request Mar 6, 2020

Package Verification Bug bufio.Scanner: token too long #1397

Closed

This was referenced Mar 11, 2020

How could I use a struct as a parameter ? #951

Closed

Provide configuration support for nodeSelector, podAnnotations, and resources #1221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support typed parameter values #1376

Support typed parameter values #1376

nfnt commented Mar 2, 2020 •

edited by porridge

Loading

nfnt commented Mar 2, 2020 •

edited

Loading

kensipe left a comment

kensipe Mar 2, 2020

ANeumann82 Mar 3, 2020

nfnt Mar 3, 2020

zen-dog Mar 6, 2020

nfnt Mar 6, 2020

zen-dog Mar 6, 2020

nfnt Mar 6, 2020 •

edited

Loading

ANeumann82 left a comment

kensipe left a comment

kensipe left a comment

	return task, fmt.Errorf("value of parameter %s isn't a string", tt.Parameter)
	return task, fmt.Errorf("value of parameter %s must be either 'true' or 'false'", tt.Parameter)

Support typed parameter values #1376

Support typed parameter values #1376

Conversation

nfnt commented Mar 2, 2020 • edited by porridge Loading

nfnt commented Mar 2, 2020 • edited Loading

kensipe left a comment

Choose a reason for hiding this comment

kensipe Mar 2, 2020

Choose a reason for hiding this comment

ANeumann82 Mar 3, 2020

Choose a reason for hiding this comment

nfnt Mar 3, 2020

Choose a reason for hiding this comment

zen-dog Mar 6, 2020

Choose a reason for hiding this comment

nfnt Mar 6, 2020

Choose a reason for hiding this comment

zen-dog Mar 6, 2020

Choose a reason for hiding this comment

nfnt Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

ANeumann82 left a comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

nfnt commented Mar 2, 2020 •

edited by porridge

Loading

nfnt commented Mar 2, 2020 •

edited

Loading

nfnt Mar 6, 2020 •

edited

Loading