fix(steps): provide functions args in function conditions values #343
Conversation
|
CDS Report test#1310.0 ✘
|
|
CDS Report test#1309.0 ✘
|
|
CDS Report test#1310.1 ✘
|
|
LGTM. |
I guess as I cloned the values, the changes made by the condition wasn't saved in the DB. I think we have to alter the original Values with the FunctionArgs. What do you think? |
rclsilver
force-pushed
the
fix-function-conditions
branch
from
db8cb4e
to
b50aee2
Compare
|
I added a test to verify my work |
Signed-off-by: Thomas Bétrancourt <thomas@betrancourt.net>
rclsilver
force-pushed
the
fix-function-conditions
branch
from
b50aee2
to
c3a4e9b
Compare
|
CDS Report test#1326.0 ✘
|
|
CDS Report test#1327.0 ✘
|
|
CDS Report test#1327.1 ✘
|
|
CDS Report test#1327.2 ✘
|
|
CDS Report test#1327.3 ✘
|
|
CDS Report test#1327.4 ✘
|
|
CDS Report test#1327.5 ✘
|
Co-authored-by: Romain Beuque <556072+rbeuque74@users.noreply.github.com>
|
CDS Report test#1333.0 ✘
|
|
CDS Report test#1332.0 ✘
|
|
CDS Report test#1334.0 ✘
|
|
CDS Report test#1335.0 ✘
|
| - value: '{{ .function_args.name1 }}' | ||
| operator: EQ | ||
| expected: foobar |
There was a problem hiding this comment.
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
| ss(st.Name, StateServerError, err.Error()) | ||
| return | ||
| } | ||
| values.SetFunctionsArgs(functionInput) |
There was a problem hiding this comment.
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
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix
What is the current behavior? (You can also link to an open issue here)
Currently, we don't have the function args values available in functions conditions (skip and check).
What is the new behavior (if this is a feature change)?
Now, the function args are available in functions conditions (skip and check).
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No breaking change.
Other information: