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: enable when expressions to use expr; add new json variables to avoid expr conflicts #9761

Merged
merged 11 commits into from
Oct 11, 2022
36 changes: 21 additions & 15 deletions examples/conditionals.yaml
Original file line number Diff line number Diff line change
@@ -1,35 +1,41 @@
# Conditionals provide a way to affect the control flow of a
# workflow at runtime, depending on parameters. In this example
# the 'print-hello' template may or may not be executed depending
# on the input parameter, 'should-print'. When submitted with:
# the 'printHello' template may or may not be executed depending
# on the input parameter, 'shouldPrint'. When submitted with:
# argo submit examples/conditionals.yaml
# the step will be skipped since 'should-print' will evaluate false.
# the step will be skipped since 'shouldPrint' will evaluate false.
# When submitted with:
# argo submit examples/conditionals.yaml -p should-print=true
# the step will be executed since 'should-print' will evaluate true.
# argo submit examples/conditionals.yaml -p shouldPrint=true
# the step will be executed since 'shouldPrint' will evaluate true.
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: conditional-
spec:
entrypoint: conditional-example
entrypoint: conditionalExample
arguments:
parameters:
- name: should-print
value: "false"
- name: shouldPrint
value: "true"

templates:
- name: conditional-example
- name: conditionalExample
inputs:
parameters:
- name: should-print
- name: shouldPrint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the dash from the name since expr expressions which include dash need to be formatted with brackets due to the special character

Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify with some examples? Does this mean names with dashes will not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

steps:
- - name: print-hello
template: whalesay
when: "{{inputs.parameters.should-print}} == true"
- - name: printHello-govaluate
template: argosay
when: "{{inputs.parameters.shouldPrint}} == true" # govaluate form
- name: printHello-expr
template: argosay
when: "{{= inputs.parameters.shouldPrint == 'true'}}" # expr form
- name: printHello-expr-json
template: argosay
when: "{{=jsonpath(workflow.parameters.json, '$[0].value') == 'true'}}" # expr form

- name: whalesay
- name: argosay
container:
image: docker/whalesay:latest
image: argoproj/argosay:v1
command: [sh, -c]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to change the whalesay image to argosay image since apparently whalesay isn't supposed to be used for e2e tests

args: ["cowsay hello"]
38 changes: 38 additions & 0 deletions test/e2e/functional_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,44 @@ spec:
})
}

func (s *FunctionalSuite) TestWhenExpressions() {
s.Given().
Workflow("@functional/conditionals.yaml").
When().
SubmitWorkflow().
WaitForWorkflow(fixtures.ToBeSucceeded, 2*time.Minute).
Then().
ExpectWorkflowNode(wfv1.NodeWithDisplayName("printHello-govaluate"), func(t *testing.T, n *wfv1.NodeStatus, p *apiv1.Pod) {
assert.NotEqual(t, wfv1.NodeTypeSkipped, n.Type)
}).
ExpectWorkflowNode(wfv1.NodeWithDisplayName("printHello-expr"), func(t *testing.T, n *wfv1.NodeStatus, p *apiv1.Pod) {
assert.NotEqual(t, wfv1.NodeTypeSkipped, n.Type)
}).
ExpectWorkflowNode(wfv1.NodeWithDisplayName("printHello-expr-json"), func(t *testing.T, n *wfv1.NodeStatus, p *apiv1.Pod) {
assert.NotEqual(t, wfv1.NodeTypeSkipped, n.Type)
})
}

func (s *FunctionalSuite) TestJSONVariables() {

s.Given().
Workflow("@testdata/json-variables.yaml").
When().
SubmitWorkflow().
WaitForWorkflow().
Then().
ExpectWorkflowNode(wfv1.SucceededPodNode, func(t *testing.T, n *wfv1.NodeStatus, p *apiv1.Pod) {
for _, c := range p.Spec.Containers {
if c.Name == "main" {
assert.Equal(t, 3, len(c.Args))
assert.Equal(t, "myLabelValue", c.Args[0])
assert.Equal(t, "myAnnotationValue", c.Args[1])
assert.Equal(t, "myParamValue", c.Args[2])
}
}
})
}

func (s *FunctionalSuite) TestWorkflowTTL() {
s.Given().
Workflow(`
Expand Down
26 changes: 26 additions & 0 deletions test/e2e/testdata/json-variables.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
apiVersion: argoproj.io/v1alpha1
kind: Workflow

metadata:
generateName: json-variables-

labels:
myLabel: myLabelValue
annotations:
myAnnotation: myAnnotationValue
spec:
entrypoint: argosay1
arguments:
parameters:
- name: myParam
value: myParamValue

templates:
- name: argosay1
container:
image: argoproj/argosay:v1
command: [echo]
args:
- "{{=jsonpath(workflow.labels.json, '$.myLabel')}}"
- "{{=jsonpath(workflow.annotations.json, '$.myAnnotation')}}"
- "{{=jsonpath(workflow.parameters.json, '$[0].value')}}"
12 changes: 9 additions & 3 deletions workflow/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,12 +175,18 @@ const (
GlobalVarWorkflowFailures = "workflow.failures"
// GlobalVarWorkflowDuration is the current duration of this workflow
GlobalVarWorkflowDuration = "workflow.duration"
// GlobalVarWorkflowAnnotations is a JSON string containing all workflow annotations
// GlobalVarWorkflowAnnotations is a JSON string containing all workflow annotations - which will be deprecated in favor of GlobalVarWorkflowAnnotationsJSON
GlobalVarWorkflowAnnotations = "workflow.annotations"
// GlobalVarWorkflowLabels is a JSON string containing all workflow labels
// GlobalVarWorkflowAnnotationsJSON is a JSON string containing all workflow annotations
GlobalVarWorkflowAnnotationsJSON = "workflow.annotations.json"
// GlobalVarWorkflowLabels is a JSON string containing all workflow labels - which will be deprecated in favor of GlobalVarWorkflowLabelsJSON
GlobalVarWorkflowLabels = "workflow.labels"
// GlobalVarWorkflowParameters is a JSON string containing all workflow parameters
// GlobalVarWorkflowLabelsJSON is a JSON string containing all workflow labels
GlobalVarWorkflowLabelsJSON = "workflow.labels.json"
// GlobalVarWorkflowParameters is a JSON string containing all workflow parameters - which will be deprecated in favor of GlobalVarWorkflowParametersJSON
GlobalVarWorkflowParameters = "workflow.parameters"
// GlobalVarWorkflowParametersJSON is a JSON string containing all workflow parameters
GlobalVarWorkflowParametersJSON = "workflow.parameters.json"
// GlobalVarWorkflowCronScheduleTime is the scheduled timestamp of a Workflow started by a CronWorkflow
GlobalVarWorkflowCronScheduleTime = "workflow.scheduledTime"

Expand Down
3 changes: 3 additions & 0 deletions workflow/controller/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,7 @@ func (woc *wfOperationCtx) setGlobalParameters(executionParameters wfv1.Argument

if workflowParameters, err := json.Marshal(woc.execWf.Spec.Arguments.Parameters); err == nil {
woc.globalParams[common.GlobalVarWorkflowParameters] = string(workflowParameters)
woc.globalParams[common.GlobalVarWorkflowParametersJSON] = string(workflowParameters)
}
for _, param := range executionParameters.Parameters {
if param.Value == nil && param.ValueFrom == nil {
Expand Down Expand Up @@ -605,12 +606,14 @@ func (woc *wfOperationCtx) setGlobalParameters(executionParameters wfv1.Argument

if workflowAnnotations, err := json.Marshal(woc.wf.ObjectMeta.Annotations); err == nil {
woc.globalParams[common.GlobalVarWorkflowAnnotations] = string(workflowAnnotations)
woc.globalParams[common.GlobalVarWorkflowAnnotationsJSON] = string(workflowAnnotations)
}
for k, v := range woc.wf.ObjectMeta.Annotations {
woc.globalParams["workflow.annotations."+k] = v
}
if workflowLabels, err := json.Marshal(woc.wf.ObjectMeta.Labels); err == nil {
woc.globalParams[common.GlobalVarWorkflowLabels] = string(workflowLabels)
woc.globalParams[common.GlobalVarWorkflowLabelsJSON] = string(workflowLabels)
}
for k, v := range woc.wf.ObjectMeta.Labels {
// if the Label will get overridden by a LabelsFrom expression later, don't set it now
Expand Down
15 changes: 3 additions & 12 deletions workflow/validate/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ func ValidateWorkflow(wftmplGetter templateresolution.WorkflowTemplateNamespaced
}
if len(wfArgs.Parameters) > 0 {
ctx.globalParams[common.GlobalVarWorkflowParameters] = placeholderGenerator.NextPlaceholder()
ctx.globalParams[common.GlobalVarWorkflowParametersJSON] = placeholderGenerator.NextPlaceholder()
}

for _, param := range wfArgs.Parameters {
Expand Down Expand Up @@ -190,11 +191,13 @@ func ValidateWorkflow(wftmplGetter templateresolution.WorkflowTemplateNamespaced
ctx.globalParams["workflow.annotations."+k] = placeholderGenerator.NextPlaceholder()
}
ctx.globalParams[common.GlobalVarWorkflowAnnotations] = placeholderGenerator.NextPlaceholder()
ctx.globalParams[common.GlobalVarWorkflowAnnotationsJSON] = placeholderGenerator.NextPlaceholder()

for k := range mergedLabels {
ctx.globalParams["workflow.labels."+k] = placeholderGenerator.NextPlaceholder()
}
ctx.globalParams[common.GlobalVarWorkflowLabels] = placeholderGenerator.NextPlaceholder()
ctx.globalParams[common.GlobalVarWorkflowLabelsJSON] = placeholderGenerator.NextPlaceholder()

if wf.Spec.Priority != nil {
ctx.globalParams[common.GlobalVarWorkflowPriority] = strconv.Itoa(int(*wf.Spec.Priority))
Expand Down Expand Up @@ -825,10 +828,6 @@ func (ctx *templateValidationCtx) validateSteps(scope map[string]interface{}, tm
return errors.Errorf(errors.CodeBadRequest, "templates.%s.steps[%d].%s %s", tmpl.Name, i, step.Name, err.Error())
}

if !validateWhenExpression(step.When) {
return errors.Errorf(errors.CodeBadRequest, "templates.%s when expression doesn't support 'expr' format '{{='. 'When' expression is only support govaluate format {{", tmpl.Name)
}

if step.HasExitHook() {
ctx.addOutputsToScope(resolvedTmpl, fmt.Sprintf("steps.%s", step.Name), scope, false, false)
}
Expand Down Expand Up @@ -1150,10 +1149,6 @@ func (d *dagValidationContext) GetTaskFinishedAtTime(taskName string) time.Time
return time.Now()
}

func validateWhenExpression(when string) bool {
return !strings.HasPrefix(when, "{{=")
}

func (ctx *templateValidationCtx) validateDAG(scope map[string]interface{}, tmplCtx *templateresolution.Context, tmpl *wfv1.Template) error {
err := validateNonLeaf(tmpl)
if err != nil {
Expand Down Expand Up @@ -1202,10 +1197,6 @@ func (ctx *templateValidationCtx) validateDAG(scope map[string]interface{}, tmpl
return errors.Errorf(errors.CodeBadRequest, "templates.%s cannot use 'continueOn' when using 'depends'. Instead use 'dep-task.Failed'/'dep-task.Errored'", tmpl.Name)
}

if !validateWhenExpression(task.When) {
return errors.Errorf(errors.CodeBadRequest, "templates.%s when doesn't support 'expr' expression '{{='. 'When' expression is only support govaluate format {{", tmpl.Name)
}

resolvedTmpl, err := ctx.validateTemplateHolder(&task, tmplCtx, &FakeArguments{})
if err != nil {
return errors.Errorf(errors.CodeBadRequest, "templates.%s.tasks.%s %s", tmpl.Name, task.Name, err.Error())
Expand Down