KEP-35 Impl Part1: New parameter types and attributes #1705

ANeumann82 · 2020-10-02T10:58:57Z

What this PR does / why we need it:
Part 1 of KEP 35:

New Types:

integer
number
boolean

New Attributes:

enum

Only in the package format:

hint
advanced
group

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Extended integration test operator Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed bug in required parameter validation Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> # Conflicts: # go.mod # go.sum # pkg/kudoctl/kudoinit/crd/bindata.go

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

alenkacz

Did a first pass, I don't think there's blocker. I would still like to see replies to some of my questions before I am ready to approve :)

Nice work!

alenkacz · 2020-10-08T08:57:43Z

pkg/apis/kudo/v1beta1/parameter_types.go

@@ -30,4 +30,7 @@ type Parameter struct {

 	// Specifies if the parameter can be changed after the initial installation of the operator
 	Immutable *bool `json:"immutable,omitempty"`
+
+	// Defines a list of allowed values. If Default is set and Enum is not nil, the value must be in this list as well
+	Enum *[]string `json:"enum,omitempty"`


why is this pointer to slice? 🤔

The intention was to make it clear if any value is set or not - but as an "enum" with no entries would be invalid anyway, it could be an unpointered slice as well.

I kind of like it as a pointer, but i'm not really married to it - if it bothers you, i'll change it :)

ah you mean you want to validate for user providing

enum: default: xxx

? Well ... okay :)

pkg/apis/kudo/v1beta1/parameter_types_helpers.go

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

alenkacz · 2020-10-08T11:03:38Z

pkg/kudoctl/packages/types.go

@@ -37,6 +37,12 @@ type Parameter struct {
 	Trigger     string                `json:"trigger,omitempty"`
 	Type        kudoapi.ParameterType `json:"type,omitempty"`
 	Immutable   *bool                 `json:"immutable,omitempty"`
+	Enum        *[]string             `json:"enum,omitempty"`
+
+	// The following fields are descriptive only and are not used in the OperatorVersion


I did not really understand what does the descriptive mean here, maybe it's just my problem buuut ... could we maybe say something like that these are prperties that live only on the package level and are used for additional data during the installation and are not necessary server-side? Something along those lines... I actually had to go back to KEP to understand this better

👍 Added a few more lines

alenkacz · 2020-10-08T11:05:06Z

pkg/kudoctl/packages/types.go

+
+type Group struct {
+	Name        string `json:"name,omitempty"`
+	DisplayName string `json:"displayName,omitempty"`


why do we need displayName? 🤔

To allow a nice human readable title for the group, i.e.

Name: "readiness" DisplayName: "Readiness Parameters"

alenkacz · 2020-10-08T11:12:17Z

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

+	for _, p := range pf.Params.Parameters {
+		if p.Group != "" {
+			if _, ok := groups[p.Group]; !ok {
+				res.AddParamWarning(p.Name, "has a group that is not defined in the group section")


why is this just warn and not err?

Because I think it's ok to not define the groups - in this case they'd have to be displayed without a description and displayName, simply with the group name.

hmm I thought that a potential UI will read all the groups and then render params in those groups. But then most of the params are not expected to be in any group or be in group but that group not defined? That sounds like a lot of edge cases a potential UI have to handle 🤔

Just a thought...

Yeah... Valid point. I guess it'd be acceptable if people want to use a group that they have to define the group first. I'll make it mandatory.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

nfnt

LGTM!

alenkacz

Looks much better now, thank you!

I have one more idea around moving around the validate methods to make the encapsulation a bit better but otherwise 👍

alenkacz · 2020-10-09T13:15:33Z

pkg/apis/kudo/v1beta1/parameter_types.go

@@ -30,4 +30,7 @@ type Parameter struct {

 	// Specifies if the parameter can be changed after the initial installation of the operator
 	Immutable *bool `json:"immutable,omitempty"`
+
+	// Defines a list of allowed values. If Default is set and Enum is not nil, the value must be in this list as well
+	Enum *[]string `json:"enum,omitempty"`


ah you mean you want to validate for user providing

enum: default: xxx

? Well ... okay :)

alenkacz · 2020-10-09T13:19:35Z

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

+			}
+			for _, enumVal := range p.EnumValues() {
+
+				if err := kudoapi.ValidateParameterValueForType(p.Type, enumVal); err != nil {


why is this enum validation not part of some kind of p.Validate as well? 🤔 then ValidateParameterValueForType could have been made private

Well, one reason is that here in this function we have a package.Parameter and not a kudoapi.Parameter - This caused quite a bit of headache while organising this - both types are distinct but share quite a bit of common functionality.
This is one of the cases where I'd really like to have generics ;)

ANeumann82 added 2 commits October 1, 2020 18:12

Started implementation

9fa63dc

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Add verify for group

cf13382

Extended integration test operator Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 requested review from alenkacz, gerred, kensipe, nfnt and zen-dog as code owners October 2, 2020 10:58

ANeumann82 added 6 commits October 2, 2020 12:59

Small cleanup

f1e1799

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Added groups, regenerated CRDs

607e521

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Updated go-bindata

7b9805d

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Updated golden files

170237f

Fixed bug in required parameter validation Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Merge branch 'main' into an/new-param-types-and-attributes

1fdc65a

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> # Conflicts: # go.mod # go.sum # pkg/kudoctl/kudoinit/crd/bindata.go

Update bindata

8636b67

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 added release/blocker release/highlight This PR is a highlight for the next release labels Oct 2, 2020

ANeumann82 added 2 commits October 8, 2020 10:19

More param verify rules

23052a0

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Extended package verify tests

dafd960

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

alenkacz reviewed Oct 8, 2020

View reviewed changes

ANeumann82 added 4 commits October 8, 2020 17:25

Review requests, fixed a lot of conversion and type validation

3438ca0

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed unit tests

c83b204

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Allow array and map types to be specified as a string

d747a31

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Make defined groups mandatory

400bc67

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

nfnt approved these changes Oct 9, 2020

View reviewed changes

alenkacz approved these changes Oct 9, 2020

View reviewed changes

ANeumann82 merged commit 040d5ed into main Oct 9, 2020

ANeumann82 deleted the an/new-param-types-and-attributes branch October 9, 2020 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-35 Impl Part1: New parameter types and attributes #1705

KEP-35 Impl Part1: New parameter types and attributes #1705

ANeumann82 commented Oct 2, 2020

alenkacz left a comment

alenkacz Oct 8, 2020

ANeumann82 Oct 8, 2020

alenkacz Oct 9, 2020

alenkacz Oct 8, 2020 •

edited

Loading

ANeumann82 Oct 8, 2020

alenkacz Oct 8, 2020

ANeumann82 Oct 8, 2020

alenkacz Oct 8, 2020

ANeumann82 Oct 8, 2020

alenkacz Oct 9, 2020

ANeumann82 Oct 9, 2020

nfnt left a comment

alenkacz left a comment

alenkacz Oct 9, 2020

alenkacz Oct 9, 2020

ANeumann82 Oct 9, 2020

KEP-35 Impl Part1: New parameter types and attributes #1705

KEP-35 Impl Part1: New parameter types and attributes #1705

Conversation

ANeumann82 commented Oct 2, 2020

alenkacz 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

alenkacz Oct 8, 2020 • edited Loading

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

nfnt left a comment

Choose a reason for hiding this comment

alenkacz 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

alenkacz Oct 8, 2020 •

edited

Loading