-
Notifications
You must be signed in to change notification settings - Fork 104
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
Support typed parameter values #1376
Changes from 25 commits
749ae25
effeed1
61c93dd
3e7da70
bb35f03
13999e7
ac5703c
b51fb1a
46072b8
5d00d42
77a6eaa
bd94729
73f222a
48ba154
dbe000f
2b67286
3505640
9b0bbbf
ea8b0c7
a10760a
f7c7cbf
ee4a316
327e06a
82a47c2
cb920eb
91e69dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't that done by the line below? If There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's not a string it can't be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Booleans can also be represented by a boolean or integer type. Also |
||||||
} | ||||||
|
||||||
enabled, err := strconv.ParseBool(stringVal) | ||||||
if err != nil { | ||||||
return task, fmt.Errorf("could not parse value of parameter %s: %v", tt.Parameter, err) | ||||||
} | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use
type
here, because this will result in bugs with Kubernetes' schema validation. This bug didn't show when testing locally against a Kubernetes 1.17.2 cluster, so it might already be fixed. Anyways, the version ofcontrol-plane
used in the integration tests is affected, other versions possibly as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the combination of
type: array
triggering this, as this is also used in JSON Schema. The test was working withtype: list
. But we also usetype: string
which might trigger bugs as well.