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.

Sign up for GitHub

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

Jump to bottom

Support typed parameter values #1376

Merged
merged 26 commits into from
Mar 6, 2020
Merged

Support typed parameter values #1376

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
749ae25
Support typed parameter values
Feb 28, 2020
effeed1
Update golden files
Mar 2, 2020
61c93dd
Fix a typo and document conversion functions
Mar 2, 2020
3e7da70
Fix 'parse-param-operator-in-operator'
Mar 2, 2020
bb35f03
Address review comments
Mar 3, 2020
13999e7
Update generated files
Mar 3, 2020
ac5703c
Golden files have to be updated as well
Mar 3, 2020
b51fb1a
Fix a compilation error
Mar 3, 2020
46072b8
object => map
Mar 3, 2020
5d00d42
Add 'ParamValue' to 'convert' package
Mar 4, 2020
77a6eaa
Improve readability of 'paramsMap'
Mar 4, 2020
bd94729
Reformat comment
Mar 4, 2020
73f222a
Update golden files
Mar 4, 2020
48ba154
Rename 'type' to 'value-type' to avoid bugs in JSON Schema validation
Mar 4, 2020
dbe000f
Allow parameter default values to be defined as YAML in 'params.yaml'
Mar 4, 2020
2b67286
Fix handling if empty default values
Mar 4, 2020
3505640
Rename conversion functions
Mar 4, 2020
9b0bbbf
Integration test for typed parameter values in operator package
Mar 4, 2020
ea8b0c7
Require an older version of Kubernetes
Mar 4, 2020
a10760a
Rename convert functions and parameters
Mar 5, 2020
f7c7cbf
Rename variable
Mar 5, 2020
ee4a316
Fix a compilation error
Mar 5, 2020
327e06a
Update pkg/util/convert/convert.go
Mar 5, 2020
82a47c2
Change function docs
Mar 5, 2020
cb920eb
Refactor package type converters into separate 'convert' package
Mar 6, 2020
91e69dc
Improve error messages
Mar 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions config/crds/kudo.dev_operatorversions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,10 @@ spec:
this parameter changes in the Instance object. Default is `update`
if a plan with that name exists, otherwise it's `deploy`.
type: string
type:
description: Type specifies the value type. By default a string
is assumed.
type: string
type: object
type: array
plans:
Expand Down
17 changes: 17 additions & 0 deletions pkg/apis/kudo/v1beta1/operatorversion_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,20 @@ type Plan struct {
Phases []Phase `json:"phases"`
}

// ParameterValueType specifies the type of a parameter value.
type ParameterValueType string
nfnt marked this conversation as resolved.
Show resolved Hide resolved

const (
// StringValueType is used for parameter values that are provided as a string.
StringValueType ParameterValueType = "string"
nfnt marked this conversation as resolved.
Show resolved Hide resolved

// ListValueType is used for parameter values that described a list of values.
ListValueType ParameterValueType = "list"
nfnt marked this conversation as resolved.
Show resolved Hide resolved

// DictValueType is used for parameter values that describe a dictionary.
DictValueType ParameterValueType = "dict"
)

// Parameter captures the variability of an OperatorVersion being instantiated in an instance.
type Parameter struct {
// DisplayName can be used by UIs.
Expand All @@ -95,6 +109,9 @@ type Parameter struct {
// Trigger identifies the plan that gets executed when this parameter changes in the Instance object.
// Default is `update` if a plan with that name exists, otherwise it's `deploy`.
Trigger string `json:"trigger,omitempty"`

// Type specifies the value type. By default a string is assumed.
nfnt marked this conversation as resolved.
Show resolved Hide resolved
Type ParameterValueType `json:"type,omitempty"`
}

// Phase specifies a list of steps that contain Kubernetes objects.
Expand Down
44 changes: 33 additions & 11 deletions pkg/controller/instance/instance_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,11 @@ func preparePlanExecution(instance *kudov1beta1.Instance, ov *kudov1beta1.Operat
return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not find required plan: %v", engine.ErrFatalExecution, activePlanStatus.Name), EventName: "InvalidPlan"}
}

params := paramsMap(instance, ov)
params, err := paramsMap(instance, ov)
if err != nil {
return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not parse parameters: %v", engine.ErrFatalExecution, err), EventName: "InvalidParams"}
}

pipes, err := pipesMap(activePlanStatus.Name, &planSpec, ov.Spec.Tasks, meta)
if err != nil {
return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not make task pipes: %v", engine.ErrFatalExecution, err), EventName: "InvalidPlan"}
Expand Down Expand Up @@ -382,22 +386,40 @@ func GetOperatorVersion(instance *kudov1beta1.Instance, c client.Client) (ov *ku
return ov, nil
}

// paramsMap generates {{ Params.* }} map of keys and values which is later used during template rendering.
func paramsMap(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.OperatorVersion) map[string]string {
params := make(map[string]string)

for k, v := range instance.Spec.Parameters {
params[k] = v
func paramValue(v *string, t v1beta1.ParameterValueType) (r interface{}, err error) {
nfnt marked this conversation as resolved.
Show resolved Hide resolved
switch t {
case kudov1beta1.DictValueType:
r, err = kudo.YAMLDict(kudo.StringValue(v))
nfnt marked this conversation as resolved.
Show resolved Hide resolved
case kudov1beta1.ListValueType:
r, err = kudo.YAMLList(kudo.StringValue(v))
default:
r = kudo.StringValue(v)
}

// Merge instance parameter overrides with operator version, if no override exist, use the default one
return
}

// paramsMap generates {{ Params.* }} map of keys and values which is later used during template rendering.
func paramsMap(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.OperatorVersion) (map[string]interface{}, error) {
params := make(map[string]interface{}, len(operatorVersion.Spec.Parameters))

for _, param := range operatorVersion.Spec.Parameters {
if _, ok := params[param.Name]; !ok {
params[param.Name] = kudo.StringValue(param.Default)
var err error

params[param.Name], err = paramValue(param.Default, param.Type)
nfnt marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}

if value, ok := instance.Spec.Parameters[param.Name]; ok {
params[param.Name], err = paramValue(&value, param.Type)
if err != nil {
return nil, err
}
}
}

return params
return params, nil
}

// pipesMap generates {{ Pipes.* }} map of keys and values which is later used during template rendering.
Expand Down
6 changes: 3 additions & 3 deletions pkg/engine/task/task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ type Context struct {
Config *rest.Config
Enhancer renderer.Enhancer
Meta renderer.Metadata
Templates map[string]string // Raw templates
Parameters map[string]string // Instance and OperatorVersion parameters merged
Pipes map[string]string // Pipe artifacts
Templates map[string]string // Raw templates
Parameters map[string]interface{} // Instance and OperatorVersion parameters merged
Pipes map[string]string // Pipe artifacts
}

// Tasker is an interface that represents any runnable task for an operator. This method is treated
Expand Down
8 changes: 7 additions & 1 deletion pkg/engine/task/task_toggle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,13 @@ func (tt ToggleTask) delegateTask(ctx Context) (Tasker, error) {
if !exists {
return task, fmt.Errorf("no value for parameter %s found", tt.Parameter)
}
enabled, err := strconv.ParseBool(val)

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

Choose a reason for hiding this comment

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

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..

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@nfnt nfnt Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

enabled, err := strconv.ParseBool(stringVal)
if err != nil {
return task, fmt.Errorf("could not parse value of parameter %s: %v", tt.Parameter, err)
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/engine/task/task_toggle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func TestToggleTask_Run(t *testing.T) {
done: true,
wantErr: false,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "true",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -86,7 +86,7 @@ func TestToggleTask_Run(t *testing.T) {
done: false,
wantErr: false,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "true",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand Down Expand Up @@ -142,7 +142,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: false,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "true",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -162,7 +162,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: false,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "false",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -182,7 +182,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: true,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "notABooleanValue",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -202,7 +202,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: true,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -221,7 +221,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: true,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "someValue",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand Down
2 changes: 1 addition & 1 deletion pkg/engine/workflow/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type ActivePlan struct {
Spec *v1beta1.Plan
Tasks []v1beta1.Task
Templates map[string]string
Params map[string]string
Params map[string]interface{}
Pipes map[string]string
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/kudoctl/cmd/testdata/deploy-kudo-ns.yaml.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ spec:
this parameter changes in the Instance object. Default is `update`
if a plan with that name exists, otherwise it's `deploy`.
type: string
type:
description: Type specifies the value type. By default a string
is assumed.
type: string
type: object
type: array
plans:
Expand Down
4 changes: 4 additions & 0 deletions pkg/kudoctl/cmd/testdata/deploy-kudo-sa.yaml.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ spec:
this parameter changes in the Instance object. Default is `update`
if a plan with that name exists, otherwise it's `deploy`.
type: string
type:
description: Type specifies the value type. By default a string
is assumed.
type: string
type: object
type: array
plans:
Expand Down
4 changes: 4 additions & 0 deletions pkg/kudoctl/cmd/testdata/deploy-kudo-webhook.yaml.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ spec:
this parameter changes in the Instance object. Default is `update`
if a plan with that name exists, otherwise it's `deploy`.
type: string
type:
description: Type specifies the value type. By default a string
is assumed.
type: string
type: object
type: array
plans:
Expand Down
4 changes: 4 additions & 0 deletions pkg/kudoctl/cmd/testdata/deploy-kudo.yaml.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ spec:
this parameter changes in the Instance object. Default is `update`
if a plan with that name exists, otherwise it's `deploy`.
type: string
type:
description: Type specifies the value type. By default a string
is assumed.
type: string
type: object
type: array
plans:
Expand Down
Loading