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.

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

fix(steps): provide functions args in function conditions values #343

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions engine/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,29 @@ func TestFunctionCustomState(t *testing.T) {
assert.Equal(t, []string{"STATE_HELLO"}, customStates)
}

func TestFunctionConditions(t *testing.T) {
input := map[string]interface{}{}
res, err := runTask("functionConditions.yaml", input, nil)

require.Nil(t, err)

assert.Equal(t, map[string]interface{}{
"value": "Hello foobar !",
}, res.Steps["asExpected"].Output)
assert.Equal(t, "SAID_HELLO", res.Steps["asExpected"].State)
assert.Equal(t, "Said hello to foobar", res.Steps["asExpected"].Error)

assert.Equal(t, map[string]interface{}{
"value": "Hello foo !",
}, res.Steps["notExpected"].Output)
assert.Equal(t, "NOT_EXPECTED", res.Steps["notExpected"].State)
assert.Equal(t, "Expected bar, got foo", res.Steps["notExpected"].Error)

assert.Nil(t, res.Steps["skipped"].Output)
assert.Equal(t, "PRUNE", res.Steps["skipped"].State)
assert.Equal(t, "No hello foo !", res.Steps["skipped"].Error)
}

func TestFunctionPreHook(t *testing.T) {
input := map[string]interface{}{}
res, err := runTask("functionPreHook.yaml", input, nil)
Expand Down
34 changes: 34 additions & 0 deletions engine/functions_tests/hello-with-conditions.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
name: hello-with-conditions
description: Say hello with conditions
action:
type: echo::hello::world
configuration:
name: '{{.function_args.name}}'
custom_states:
- SAID_HELLO
- NOT_EXPECTED
conditions:
- type: skip
if:
- value: '{{.function_args.skip | default `false`}}'
operator: EQ
expected: 'true'
then:
this: PRUNE
message: 'No hello {{.function_args.name}} !'
- type: check
if:
- value: '{{.function_args.name}}'
operator: NE
expected: '{{.function_args.expected}}'
then:
this: NOT_EXPECTED
message: 'Expected {{.function_args.expected}}, got {{.function_args.name}}'
- type: check
if:
- value: '{{.function_args.name}}'
operator: EQ
expected: '{{.function_args.expected}}'
then:
this: SAID_HELLO
message: 'Said hello to {{.function_args.name}}'
3 changes: 3 additions & 0 deletions engine/functions_tests/nested1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,8 @@ conditions:
- value: '{{.step.this.state}}'
operator: EQ
expected: 'NESTED2_STATE2'
- value: '{{ .function_args.name }}'
operator: EQ
expected: foobar
then:
this: NESTED1_STATE
3 changes: 3 additions & 0 deletions engine/functions_tests/nested2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ conditions:
- value: '{{.step.this.state}}'
operator: EQ
expected: 'NESTED2_STATE1'
- value: '{{ .function_args.name1 }}'
operator: EQ
expected: foobar
Comment on lines +25 to +27
Copy link
Member

Choose a reason for hiding this comment

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

I added this condition in the test : function nested2 has correctly been called with foobar, but it's failing in the test.
This comes from the fact that the pull request is only using the function args of the "main" step (the one declared in the template): if we start to have nested functions, templating start to fails, because only the function_args on the "main" step is populated in the values.

I also found why this test was failing if we used values.Clone, follow the second comment

then:
this: NESTED2_STATE2

34 changes: 34 additions & 0 deletions engine/step/step.go
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,23 @@ func PreRun(st *Step, values *values.Values, ss StateSetter, executedSteps map[s
return
}

// Check if we have a function as runner or not.
runner, err := getRunner(st.Action.Type)
if err != nil {
ss(st.Name, StateServerError, err.Error())
return
}

_, isFunction := runner.(*functions.Function)
if isFunction {
var functionInput map[string]interface{}
if err := utils.JSONnumberUnmarshal(bytes.NewBuffer(st.Action.Configuration), &functionInput); err != nil {
ss(st.Name, StateServerError, err.Error())
return
}
values.SetFunctionsArgs(functionInput)
}

for _, sc := range conditions {
if sc.Type != condition.SKIP {
continue
Expand Down Expand Up @@ -521,6 +538,23 @@ func AfterRun(st *Step, values *values.Values, ss StateSetter) {
return
}

// Check if we have a function as runner or not.
runner, err := getRunner(st.Action.Type)
if err != nil {
ss(st.Name, StateServerError, err.Error())
return
}

_, isFunction := runner.(*functions.Function)
if isFunction {
var functionInput map[string]interface{}
if err := utils.JSONnumberUnmarshal(bytes.NewBuffer(st.Action.Configuration), &functionInput); err != nil {
ss(st.Name, StateServerError, err.Error())
return
}
values.SetFunctionsArgs(functionInput)
Copy link
Member

Choose a reason for hiding this comment

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

Here, we previously used values.Clone to prevent alter on the values object of the caller.
But, in this function, we have a second parameter, which is ss for StateSetter: https://github.com/ovh/utask/blob/master/engine/step/step.go#L521
Called from here: https://github.com/ovh/utask/blob/master/engine/engine.go#L431
definition of this function is here: https://github.com/ovh/utask/blob/master/engine/engine.go#L1064-L1071

Everytime a condition is matching, we need to alter the state of some steps, using ss (such as here https://github.com/ovh/utask/blob/master/engine/step/step.go#L551 )
And the call of the closure in resolutionStateSetter is using the parent values object to alter the current state.
So when the second condition is evaluated, and require that the first condition altered the state to match, we are using the cloned value, which has not been altered by the closure in resolutionStateSetter, which mean that the condition state will not match, making the test failing.

I guess not cloning the values is fine, but the drawback of function_args not working in conditions if they are nested seems a huge drawback that we should address before merging this one

}

for _, sc := range conditions {
if sc.Type != condition.CHECK {
continue
Expand Down
29 changes: 29 additions & 0 deletions engine/templates_tests/functionConditions.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
name: functionConditions
description: Test function conditions
title_format: "[test] Test function conditions"
auto_runnable: true
steps:
asExpected:
description: Say hello to foobar
action:
type: hello-with-conditions
configuration:
name: foobar
expected: foobar

notExpected:
description: Say hello to foo but expect bar
action:
type: hello-with-conditions
configuration:
name: foo
expected: bar

skipped:
description: Say hello to foo but skipping step before it starts
action:
type: hello-with-conditions
configuration:
name: foo
expected: foor
skip: true