fix: enable when expressions to use expr; add new json variables to avoid expr conflicts #9761
Conversation
…void expr conflicts Signed-off-by: Julie Vogelman <[email protected]>
| container: | ||
| image: docker/whalesay:latest | ||
| image: argoproj/argosay:v1 | ||
| command: [sh, -c] |
There was a problem hiding this comment.
had to change the whalesay image to argosay image since apparently whalesay isn't supposed to be used for e2e tests
examples/conditionals.yaml
Outdated
| inputs: | ||
| parameters: | ||
| - name: should-print | ||
| - name: shouldPrint |
There was a problem hiding this comment.
removed the dash from the name since expr expressions which include dash need to be formatted with brackets due to the special character
There was a problem hiding this comment.
Could you clarify with some examples? Does this mean names with dashes will not work?
There was a problem hiding this comment.
Thanks for the suggestion. I changed it back to the dashed name to show how you can handle that with expr.
BTW, this is the example Bala had, which I imitated.
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
|
Assigned to @sarabala1979 since he and I discussed this one |
Signed-off-by: Julie Vogelman <[email protected]>
|
@juliev0 PR LGTM. Can we add validation? so it will fail in validation |
Signed-off-by: Julie Vogelman <[email protected]>
It's a good idea but I think it's a bigger ask than just this as we're not currently doing validation even in the case that somebody adds an expr which uses "workflow.blah" for example, let alone that they use "workflow.parameters". According to this line, "we do not validate expression templates". I'd be happy to add a separate issue and take it on if you like. What I did realize was that I hadn't updated the documentation to reflect the new ".json" variables and the fact that the old ones are deprecated, so I did that just now. |
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
|
What happens if I already have a workflow parameter named 'json'? Can it cause backward compatibility issues? |
It's a good question. I think we're okay, though. If you look at the changes in workflow/controller/operator.go, lines 585 and 587 occur after line 573, meaning that the user-defined "json" parameter should override the new "workflow.parameters.json". |
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
…void expr conflicts (argoproj#9761) Signed-off-by: Julie Vogelman <[email protected]> Signed-off-by: juchao <[email protected]>
Fixes #8911
Fixes #9742
This removes the validation check which prevents expr from being used in a "when" condition, which had gotten added here.
Apparently, the actual problem that was being addressed there was the fact that "workflow.parameters" couldn't be used in an expr expression. The issue was that there were conflicts since both "workflow.parameters" and "workflow.parameters.x" were variables that need replacing, and the call to evaluate expr can't handle cases in which one variable is a prefix of another. Therefore, this adds a new variable "workflow.parameters.json" which is equivalent to "workflow.parameters". "workflow.parameters" should eventually be phased out in favor of this. (For now, we can keep it: it's just that it can't be handled in an expr expression.)