Wildcard parameters and arguments

[markterm]

This enables input parameters and arguments to be passed through using regexes, from suggestion by @dtaniwaki. It needs some further tidying and tests. Background scenario:

Using the WorkflowTemplate I’m creating a generic template that invokes a job (for example ‘SparkWorkflowTemplate’ that invokes a Spark job. This would take in a set of parameters that get passed onto Spark.

I don’t want to need to hardcode all the possible parameters that any Spark job may need into this ‘SparkWorkflowTemplate’, but at the moment it’s only possible to pass on explicitly declared parameters.

[markterm]

Too much nesting here!

[markterm]

Code needs to be deduplicated with above.

[markterm]

Maybe 'wildcard' is a better name than 'passthrough' that I've used in the code ...

[dtaniwaki]

Thank you for the PR. I'll review it soon!

[dtaniwaki]

@mark9white Thank you for sending a PR! I actually wanted to use this feature. Although the overall change is good, I made several comments. Would you check them?

[dtaniwaki]

I think this change is not related to this PR.

[markterm]

Fixing.

[dtaniwaki]

Maybe, this is just a preference issue, but Go lang uses Regexp instead of Regex for regular expressions. How about naming it Regexp?

[dtaniwaki]

Hmm... "passthrough" sounds good in arguments, but a little bit weird in inputs because it doesn't "passthrough" but rather just receiving multiple parameters by regexp. However, I don't come up with a good name...

[markterm]

Am renaming it.

[dtaniwaki]

Thank you for renaming the variable. Since we renamed this variable, I think renaming related functions and comments in this change helps maintainability.

Hmm... "passthrough" sounds good in arguments, but a little bit weird in inputs because it doesn't "passthrough" but rather just receiving multiple parameters by regexp.

What do you think about this?

[markterm]

Good point, I've renamed it everywhere else.

I agree is sounds weird in inputs, but I can't think of anything else other than 'wildcard'. I'm open to suggestions ...

[dtaniwaki]

Do you think defining 2 fields like NameRegexp for inputs and PassthroughNameRegexp for arguments is easier for users although the implementation will be more complicated?

[dtaniwaki]

And maybe defining ArgumentPatameter and InputParameter separately which contains inline Parameter?

[markterm]

Two fields would be easier for users, I'll do that.

I'm not convinced that it's worth the extra implementation complexity of ArgumentParameter and InputParameter, but happy to go ahead if you think it is.

[dtaniwaki]

I think having two fields is enough for now. :)

[dtaniwaki]

Strictly speaking, we shouldn't allow PassthroughRegex in global arguments.

[markterm]

Are you looking for a boolean flag passed through to the validateParameterField function indicating whether it's global arguments that are being validated?

[dtaniwaki]

That’s one option. Or, how about defining another function for global arguments?

[markterm]

I've done that in the extra commit I just pushed, but still ended up with isValidatingForWorkflow for the intermediate functions which feels a little messy isValidatingForWorkflow

[markterm]

I've applied the feedback, and also this commit is based on 'template-crd' as that adds some extra complications.

You'll want to review just the last commit: fcdf0c7

[dtaniwaki]

I think this one is now the same as validateWorkflowFieldNames if it doesn't check PassthroughRegexp.

[dtaniwaki]

I think this one is now the same as validateWorkflowFieldNames if it doesn't check PassthroughRegexp.

Did you check this comment?

[dtaniwaki]

I think the WorkflowTemplate CRD PR won't get merged so soon while this PR will get merged much sooner for the complexity difference, so I suggest making your change based on the master.

[markterm]

I've modified this PR to be based on master. At this point it's ready to merge on my end, pending any further review notes or build issues.

[dtaniwaki]

#1490 (comment)
I'm trying your code in my local env and realized this tmpl overwriting is necessary to validate templates with passthroughRegexp. Sorry for confusing.

[markterm]

The template overwriting caused other problems, and once I fixed other issues it seemed no longer necessary. On 26 July 2019 at 16:18:16, Daisuke Taniwaki (notifications@github.com) wrote: #1490 (comment) <#1490 (comment)> I'm trying your code in my local env and realized this tmpl overwriting is necessary to validate templates with passthroughRegexp. Sorry for confusing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1490>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABO7LSPN62EFLAW3OG7JS3QBMITPANCNFSM4IFAKIZQ> .

[dtaniwaki]

Thank you for your hard-working on this PR. I think the implementation looks great except very minor requests. Would you check them?

[dtaniwaki]

Please update the manifest for separated fields of passthroughRegex and regexp.

[dtaniwaki]

I tried the following manifest to test your code in my local.

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: passthrough-parameters-
spec:
  entrypoint: passthrough-parameters-steps
  arguments:
    parameters:
      - name: message
        value: test1
  templates:
    - name: passthrough-parameters-steps
      inputs:
        parameters:
          - regexp: ".*"
      steps:
        - - name: display-parameters
            template: display-parameters
            arguments:
              parameters:
                - passthroughRegexp: ".*"
                - name: extra-parameter
                  value: extra-value

    - name: display-parameters
      inputs:
        parameters:
          - name: message
          - name: extra-parameter
      script:
        image: alpine:latest
        command: [sh, -x]
        source: |
          #!/bin/sh
          echo {{inputs.parameters.message}}
          echo {{inputs.parameters.extra-parameter}}

[dtaniwaki]

I think this comment is not from this PR.

[dtaniwaki]

I think this one is now the same as validateWorkflowFieldNames if it doesn't check PassthroughRegexp.

Did you check this comment?

[dtaniwaki]

The template overwriting caused other problems, and once I fixed other issues it seemed no longer necessary.

Really? It didn't work with my manifest. Could you check your code with the manifest?

Please also don't forget to check the CI failure.

[markterm]

I've fixed up the issue of it operating with your manifest and the build problem. I also swapped your suggested manifest into the validate unit test.

[dtaniwaki]

Should it be Regexp?

[markterm]

Good catch. I've gone through all the others and think they're all correct.

[dtaniwaki]

LGTM except for the one I commented inline. Please make sure Regexp and PassthroughRegexp are used in proper places again just in case.

[dtaniwaki]

I don't fully understand why Regexp is copied from PassthroughRegexp here. Could you tell me an example manifests which describe the necessity of this copy?

[markterm]

TestPassthroughParams in operator_test.go fails without it - where we have arguments using a passthroughRegexp being passed into a step with regex inputs.

[dtaniwaki]

#1490 (comment)
Please try overwriting tmpl.

[dtaniwaki]

Hmm, changing it causes another error. I'm trying to fix all of them.

[dtaniwaki]

I confirmed unit tests succeeded without copying PassthroughRegexp to Regexp. If there's a case we need it, please add a unit test.

[dtaniwaki]

We're copying parameters from arguments to inputs, so I think this Regexp: inputParam.Regexp is not necessary.

[markterm]

If the top-level arguments aren't available (eg we're validating a WorkflowTemplate) then the regex needs to flow through ...

[dtaniwaki]

I think we don’t have to care WorkflowTemplate so much in this PR. That is the next step.

[markterm]

Ok, I'll fix this up once #1508 is merged.

[dtaniwaki]

#1508 has been merged.

[dtaniwaki]

If we don't copy Regexp as https://github.com/argoproj/argo/pull/1490/files#r308056723, inParam here won't have Regexp.

[dtaniwaki]

The comment should be updated. However, I think we don't need this check as https://github.com/argoproj/argo/pull/1490/files#r308056868.

[dtaniwaki]

I made more comments after I sent a PR to fix processArgs function overwriting in #1508.

Let me make sure what we're doing in this PR.

• Have Regexp to receive matched argument parameters.
• Have PassthroughRegexp to copy matched input parameters to arguments.
• The global arguments can't have PassthroughRegexp.

Is it correct?

[markterm]

I made more comments after I sent a PR to fix processArgs function overwriting in #1508.

Let me make sure what we're doing in this PR.

• Have Regexp to receive matched argument parameters.
• Have PassthroughRegexp to copy matched input parameters to arguments.
• The global arguments can't have PassthroughRegexp.

Is it correct?

This is correct, and I've rebased on master and applied those comments.

[dtaniwaki]

@mark9white Thank you for updating the PR. I made several comments to remove unnecessary code as much as possible. I also found we need passthroughRegexp handling in MergeReferredTemplate in the template resolution. Please check it too.

[dtaniwaki]

I confirmed unit tests succeeded without copying PassthroughRegexp to Regexp. If there's a case we need it, please add a unit test.

[dtaniwaki]

I think setting a placeholder just to pass the validation is not good. It might cause other problems in other places.

I modified your code and confirmed the unit tests passed.

diff --git a/workflow/common/util.go b/workflow/common/util.go
index d0ba361..599d574 100644
--- a/workflow/common/util.go
+++ b/workflow/common/util.go
@@ -258,12 +258,11 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.ArgumentsProvider, globalParams,
                        for _, argParam := range argParams {
                                newName := argParam.Name
                                newValue := argParam.Value
-                               newRegexp := argParam.PassthroughRegexp //used for workflow templates where all arguments may not have been resolved already
-                               newParameter := wfv1.Parameter{Name: newName, Value: newValue, Regexp: newRegexp}
+                               newParameter := wfv1.Parameter{Name: newName, Value: newValue}

                                newInputParameters = append(newInputParameters, newParameter)
                        }
-               } else {
+               } else if inParam.Name != "" {
                        if inParam.Default != nil {
                                // first set to default value
                                inParam.Value = inParam.Default
@@ -274,13 +273,10 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.ArgumentsProvider, globalParams,
                                newValue := *argParam.Value
                                inParam.Value = &newValue
                        }
-                       //if one of the args is a regexp (this can be true during validation) then we add a placeholder
-                       if args.HasPassthroughRegexpParameter() {
-                               newValue := "placeholder"
-                               inParam.Value = &newValue
-                       }
-                       if inParam.Value == nil {
-                               return nil, errors.Errorf(errors.CodeBadRequest, "inputs.parameters.%s was not supplied", inParam.Name)
+                       if !args.HasPassthroughRegexpParameter() {
+                               if inParam.Value == nil {
+                                       return nil, errors.Errorf(errors.CodeBadRequest, "inputs.parameters.%s was not supplied", inParam.Name)
+                               }
                        }
                        newInputParameters = append(newInputParameters, inParam)
                }
@@ -390,7 +386,8 @@ func substituteParams(tmpl *wfv1.Template, globalParams, localParams map[string]
        // Now replace the rest of substitutions (the ones that can be made) in the template
        replaceMap = make(map[string]string)
        for _, inParam := range globalReplacedTmpl.Inputs.Parameters {
-               if inParam.Regexp == "" { //this can still be set if we're handling a WorkflowTemplate
+               if inParam.Regexp == "" && inParam.Name != "" { //this can still be set if we're handling a WorkflowTemplate
                        if inParam.Value == nil {
                                return nil, errors.InternalErrorf("inputs.parameters.%s had no value", inParam.Name)
                        }

[markterm]

Yes this works, will commit.

[markterm]

I think the current test failures are caused by not having a placeholder to pass the validation. I don't see a straightforward way around this, so I'm re-applying it, but am open to ideas ...

[dtaniwaki]

Should it be regexp instead of passthroughRegexp?

[dtaniwaki]

I think we should add validation to avoid Inputs having PassthroughRegexp and Arguments having Regexp.

[markterm]

Agreed, will commit.

[dtaniwaki]

I think this validation does nothing for passthroughRegexp handling but just does the same template resolving as the others. I realized we need passthroughRegexp handling in MergeReferredTemplate and we can write tests for that function instead.

[markterm]

Agreed, am resolving.

[dtaniwaki]

To implement #1525, I'm thinking we should not modify templates directly. Instead, we can modify Arguments to pass to executeTemplate and validateTemplate in validation. What do you think?

e.g.

diff --git a/pkg/apis/workflow/v1alpha1/workflow_types.go b/pkg/apis/workflow/v1alpha1/workflow_types.go
index c435d5a..bc7a436 100644
--- a/pkg/apis/workflow/v1alpha1/workflow_types.go
+++ b/pkg/apis/workflow/v1alpha1/workflow_types.go
@@ -7,6 +7,7 @@ import (
        "regexp"
        "strings"

+       "github.com/argoproj/argo/errors"
        apiv1 "k8s.io/api/core/v1"
        metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -1116,6 +1117,29 @@ func (in *Inputs) GetParameterByName(name string) *Parameter {
        return nil
 }

+// GetPassthroughParameters returns parameter argument list where items matching PassthroughRegexp are added from input parameters
+func (in *Inputs) GetPassthroughParameters(argumentParameters []Parameter) ([]Parameter, error) {
+       updatedArgumentParameters := make([]Parameter, 0)
+
+       for _, parameter := range argumentParameters {
+               if parameter.PassthroughRegexp != "" {
+
+                       inputParams, err := in.GetParametersByRegexp(parameter.PassthroughRegexp)
+                       if err != nil {
+                               return nil, errors.Errorf(errors.CodeBadRequest, "regexp %s could not be processed", parameter.PassthroughRegexp)
+                       }
+
+                       for _, inputParam := range inputParams {
+                               newParameter := Parameter{Name: inputParam.Name, Value: inputParam.Value}
+                               updatedArgumentParameters = append(updatedArgumentParameters, newParameter)
+                       }
+               } else {
+                       updatedArgumentParameters = append(updatedArgumentParameters, parameter)
+               }
+       }
+       return updatedArgumentParameters, nil
+}
+
 // HasInputs returns whether or not there are any inputs
 func (in *Inputs) HasInputs() bool {
        if len(in.Artifacts) > 0 {
diff --git a/workflow/common/util.go b/workflow/common/util.go
index 40dd54f..28785ca 100644
--- a/workflow/common/util.go
+++ b/workflow/common/util.go
@@ -311,54 +311,9 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.ArgumentsProvider, globalParams,
        }
        newTmpl.Inputs.Artifacts = newInputArtifacts

-       //copy passthrough parameters from Inputs to Arguments
-       if newTmpl.Steps != nil {
-               for i, stepList := range newTmpl.Steps {
-                       for j, step := range stepList {
-                               updatedParameters, err := handlePassthroughParameters(newTmpl.Inputs, step.Arguments.Parameters)
-                               if err != nil {
-                                       return nil, err
-                               }
-                               newTmpl.Steps[i][j].Arguments.Parameters = updatedParameters
-                       }
-               }
-       }
-       if newTmpl.DAG != nil {
-               for i, task := range newTmpl.DAG.Tasks {
-                       updatedParameters, err := handlePassthroughParameters(tmpl.Inputs, task.Arguments.Parameters)
-                       if err != nil {
-                               return nil, err
-                       }
-                       newTmpl.DAG.Tasks[i].Arguments.Parameters = updatedParameters
-               }
-       }
-
        return substituteParams(newTmpl, globalParams, localParams)
 }

-// handlePassthroughParameters returns parameter argument list where items matching PassthroughRegexp are added from input parameters
-func handlePassthroughParameters(inputs wfv1.Inputs, argumentParameters []wfv1.Parameter) ([]wfv1.Parameter, error) {
-       updatedArgumentParameters := make([]wfv1.Parameter, 0)
-
-       for _, parameter := range argumentParameters {
-               if parameter.PassthroughRegexp != "" {
-
-                       inputParams, err := inputs.GetParametersByRegexp(parameter.PassthroughRegexp)
-                       if err != nil {
-                               return nil, errors.Errorf(errors.CodeBadRequest, "regexp %s could not be processed", parameter.PassthroughRegexp)
-                       }
-
-                       for _, inputParam := range inputParams {
-                               newParameter := wfv1.Parameter{Name: inputParam.Name, Value: inputParam.Value}
-                               updatedArgumentParameters = append(updatedArgumentParameters, newParameter)
-                       }
-               } else {
-                       updatedArgumentParameters = append(updatedArgumentParameters, parameter)
-               }
-       }
-       return updatedArgumentParameters, nil
-}
-
 // substituteParams returns a new copy of the template with global, pod, and input parameters substituted
 func substituteParams(tmpl *wfv1.Template, globalParams, localParams map[string]string) (*wfv1.Template, error) {
        tmplBytes, err := json.Marshal(tmpl)
diff --git a/workflow/controller/dag.go b/workflow/controller/dag.go
index 744238a..b684fc4 100644
--- a/workflow/controller/dag.go
+++ b/workflow/controller/dag.go
@@ -404,8 +404,16 @@ func (woc *wfOperationCtx) executeDAGTask(dagCtx *dagContext, taskName string) {
                        }
                }

+               args := t.Arguments.DeepCopy()
+               updatedParameters, err := dagCtx.tmpl.Inputs.GetPassthroughParameters(args.Parameters)
+               if err != nil {
+                       woc.initializeNode(nodeName, wfv1.NodeTypeSkipped, task, dagCtx.boundaryID, wfv1.NodeError, err.Error())
+                       return
+               }
+               args.Parameters = updatedParameters
+
                // Finally execute the template
-               _, _ = woc.executeTemplate(taskNodeName, &t, dagCtx.tmplCtx, t.Arguments, dagCtx.boundaryID)
+               _, _ = woc.executeTemplate(taskNodeName, &t, dagCtx.tmplCtx, args, dagCtx.boundaryID)
        }

        if taskGroupNode != nil {
diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go
index 2e51d52..177f56d 100644
--- a/workflow/controller/operator.go
+++ b/workflow/controller/operator.go
@@ -205,7 +205,7 @@ func (woc *wfOperationCtx) operate() {

        var workflowStatus wfv1.NodePhase
        var workflowMessage string
-       node, err := woc.executeTemplate(woc.wf.ObjectMeta.Name, &wfv1.Template{Template: woc.wf.Spec.Entrypoint}, woc.tmplCtx, woc.wf.Spec.Arguments, "")
+       node, err := woc.executeTemplate(woc.wf.ObjectMeta.Name, &wfv1.Template{Template: woc.wf.Spec.Entrypoint}, woc.tmplCtx, &woc.wf.Spec.Arguments, "")
        if err != nil {
                // the error are handled in the callee so just log it.
                woc.log.Errorf("%s error in entry template execution: %+v", woc.wf.Name, err)
@@ -233,7 +233,7 @@ func (woc *wfOperationCtx) operate() {
                }
                woc.log.Infof("Running OnExit handler: %s", woc.wf.Spec.OnExit)
                onExitNodeName := woc.wf.ObjectMeta.Name + ".onExit"
-               onExitNode, err = woc.executeTemplate(onExitNodeName, &wfv1.Template{Template: woc.wf.Spec.OnExit}, woc.tmplCtx, woc.wf.Spec.Arguments, "")
+               onExitNode, err = woc.executeTemplate(onExitNodeName, &wfv1.Template{Template: woc.wf.Spec.OnExit}, woc.tmplCtx, &woc.wf.Spec.Arguments, "")
                if err != nil {
                        // the error are handled in the callee so just log it.
                        woc.log.Errorf("%s error in exit template execution: %+v", woc.wf.Name, err)
@@ -1067,7 +1067,7 @@ func (woc *wfOperationCtx) getLastChildNode(node *wfv1.NodeStatus) (*wfv1.NodeSt
 // for the created node (if created). Nodes may not be created if parallelism or deadline exceeded.
 // nodeName is the name to be used as the name of the node, and boundaryID indicates which template
 // boundary this node belongs to.
-func (woc *wfOperationCtx) executeTemplate(nodeName string, orgTmpl wfv1.TemplateHolder, tmplCtx *templateresolution.Context, args wfv1.Arguments, boundaryID string) (*wfv1.NodeStatus, error) {
+func (woc *wfOperationCtx) executeTemplate(nodeName string, orgTmpl wfv1.TemplateHolder, tmplCtx *templateresolution.Context, args *wfv1.Arguments, boundaryID string) (*wfv1.NodeStatus, error) {
        woc.log.Debugf("Evaluating node %s: template: %s, boundaryID: %s", nodeName, common.GetTemplateHolderString(orgTmpl), boundaryID)

        node := woc.getNodeByName(nodeName)
@@ -1093,7 +1093,7 @@ func (woc *wfOperationCtx) executeTemplate(nodeName string, orgTmpl wfv1.Templat
                localParams[common.LocalVarPodName] = woc.wf.NodeID(nodeName)
        }
        // Do not overwrite tmpl here because the old value is used in the error case.
-       newTmpl, err := common.ProcessArgs(tmpl, &args, woc.globalParams, localParams, false)
+       newTmpl, err := common.ProcessArgs(tmpl, args, woc.globalParams, localParams, false)
        if err != nil {
                return woc.initializeNode(nodeName, wfv1.NodeTypeSkipped, orgTmpl, boundaryID, wfv1.NodeError, err.Error()), err
        }
diff --git a/workflow/controller/steps.go b/workflow/controller/steps.go
index 1c16ff2..2de2e1e 100644
--- a/workflow/controller/steps.go
+++ b/workflow/controller/steps.go
@@ -203,7 +203,16 @@ func (woc *wfOperationCtx) executeStepGroup(stepGroup []wfv1.WorkflowStep, sgNod
                        }
                        continue
                }
-               childNode, err := woc.executeTemplate(childNodeName, &step, stepsCtx.tmplCtx, step.Arguments, stepsCtx.boundaryID)
+
+               args := step.Arguments.DeepCopy()
+               updatedParameters, err := stepsCtx.scope.tmpl.Inputs.GetPassthroughParameters(args.Parameters)
+               if err != nil {
+                       return woc.markNodeError(sgNodeName, err)
+               }
+               args.Parameters = updatedParameters
+
+               childNode, err := woc.executeTemplate(childNodeName, &step, stepsCtx.tmplCtx, args, stepsCtx.boundaryID)
                if err != nil {
                        switch err {
                        case ErrDeadlineExceeded:
diff --git a/workflow/templateresolution/context_test.go b/workflow/templateresolution/context_test.go
index a311561..e78d9be 100644
--- a/workflow/templateresolution/context_test.go
+++ b/workflow/templateresolution/context_test.go
@@ -76,14 +76,14 @@ spec:
     template: whalesay-with-passthrough-arguments-inner
     inputs:
       parameters:
-      - passthroughRegexp: ".*"
+      - regexp: ".*"
     arguments:
       parameters:
       - passthroughRegexp: ".*"
   - name: whalesay-with-passthrough-arguments-inner
     inputs:
       parameters:
-      - passthroughRegexp: ".*"
+      - regexp: ".*"
     container:
       image: docker/whalesay
       command: [cowsay]
diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go
index 8f1da74..ce29e71 100644
--- a/workflow/validate/validate.go
+++ b/workflow/validate/validate.go
@@ -338,9 +338,7 @@ func validateInputs(tmpl *wfv1.Template, extraScope map[string]interface{}) (map
                scope[name] = value
        }
        for _, param := range tmpl.Inputs.Parameters {
-               if param.Regexp != "" {
-                       scope[fmt.Sprintf("inputs.parameters.regexp.%s", param.Regexp)] = true
-               } else {
+               if param.Name != "" {
                        scope[fmt.Sprintf("inputs.parameters.%s", param.Name)] = true
                }
        }
@@ -394,14 +392,6 @@ func resolveAllVariables(scope map[string]interface{}, tmplStr string) error {
        var unresolvedErr error
        _, allowAllItemRefs := scope[anyItemMagicValue] // 'item.*' is a magic placeholder value set by addItemsToScope

-       //inputs.parameters.regexp. stores regexps for input parameters
-       inputRegexps := make([]string, 0)
-       for k := range scope {
-               if strings.HasPrefix(k, "inputs.parameters.regexp.") {
-                       inputRegexps = append(inputRegexps, strings.Replace(k, ".regexp", "", -1))
-               }
-       }
-
        fstTmpl := fasttemplate.New(tmplStr, "{{", "}}")

        fstTmpl.ExecuteFuncString(func(w io.Writer, tag string) (int, error) {
@@ -417,20 +407,7 @@ func resolveAllVariables(scope map[string]interface{}, tmplStr string) error {
                                // NOTE: this is far from foolproof.
                        } else if strings.HasPrefix(tag, common.GlobalVarWorkflowCreationTimestamp) {
                        } else {
-                               //check for regexp matches
-                               foundRegexpMatch := false
-                               for _, regexpToMatch := range inputRegexps {
-                                       match, err := regexp.MatchString(regexpToMatch, tag)
-                                       if err != nil {
-                                               return 0, err
-                                       }
-                                       if match {
-                                               foundRegexpMatch = true
-                                       }
-                               }
-                               if foundRegexpMatch == false {
-                                       unresolvedErr = fmt.Errorf("failed to resolve {{%s}}", tag)
-                               }
+                               unresolvedErr = fmt.Errorf("failed to resolve {{%s}}", tag)
                        }
                }
                return 0, nil
@@ -585,7 +562,15 @@ func (ctx *templateValidationCtx) validateSteps(scope map[string]interface{}, tm
                        if err != nil {
                                return err
                        }
-                       resolvedTmpl, err := ctx.validateTemplateHolder(&step, tmplCtx, &step.Arguments, scope)
+
+                       args := step.Arguments.DeepCopy()
+                       updatedParameters, err := tmpl.Inputs.GetPassthroughParameters(args.Parameters)
+                       if err != nil {
+                               return err
+                       }
+                       args.Parameters = updatedParameters
+
+                       resolvedTmpl, err := ctx.validateTemplateHolder(&step, tmplCtx, args, scope)
                        if err != nil {
                                return errors.Errorf(errors.CodeBadRequest, "templates.%s.steps[%d].%s %s", tmpl.Name, i, step.Name, err.Error())
                        }
@@ -1049,8 +1034,16 @@ func (ctx *templateValidationCtx) validateDAG(scope map[string]interface{}, tmpl
                if err != nil {
                        return err
                }
+
+               args := task.Arguments.DeepCopy()
+               updatedParameters, err := tmpl.Inputs.GetPassthroughParameters(args.Parameters)
+               if err != nil {
+                       return err
+               }
+               args.Parameters = updatedParameters
+
                // Validate the template again with actual arguments.
-               _, err = ctx.validateTemplateHolder(&task, tmplCtx, &task.Arguments, scope)
+               _, err = ctx.validateTemplateHolder(&task, tmplCtx, args, scope)
                if err != nil {
                        return errors.Errorf(errors.CodeBadRequest, "templates.%s.tasks.%s %s", tmpl.Name, task.Name, err.Error())
                }
diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go
index d7e4010..13722d7 100644
--- a/workflow/validate/validate_test.go
+++ b/workflow/validate/validate_test.go
@@ -294,7 +294,7 @@ func TestPassthrough(t *testing.T) {

        err = validate(badArgsRegex)
        if assert.NotNil(t, err) {
-               assert.Contains(t, err.Error(), "regexp ( could not be processed")
+               assert.Contains(t, err.Error(), "error parsing regexp: missing closing ): `(`")
        }

        err = validate(badInputRegex)

[dtaniwaki]

Maybe, we have to make like above for ProcessArgs instead of executeTemplate and validateTemplate, or make the same in MergeReferredTemplate.

[markterm]

Good idea, I've just included this, with the same logic in MergeReferredTemplate.

[markterm]

@dtaniwaki Any next steps here?

[dtaniwaki]

@mark9white Sorry for the late reply. I still need to finish #1552 and it will make huge change around argument passing.

[markterm]

@dtaniwaki
I've rebased now #1552 is merged and verified that all the tests are passing.

[dtaniwaki]

@mark9white Sorry for the late reply. I will review the change soon.

[Ark-kun]

I have mixed feelings about this change. This will make it harder in future to have evaluated placeholders or just moving to a placeholder system that does not require knowing all placeholder names beforehand ({{item.attr1}} is a primitive example of that).

I also feel that the bigger challenge right now is properly declaring dependencies. When you just get all output references that exist at the given model, you get something random unless you properly express dependencies on the tasks producing those outputs. And dependencies are very limited at this moment. I cannot even depend on "A.succeeded or else B.succeeded" and then pass "A.result if A.succeeded else B.result".

[Ark-kun]

An easy way to solve the original issue would have been to just pass a single JSON-serialized dictionary to the SparkTemplate.

template: spark-template
arguments:
- name: arguments
  value: '{"param1": "{{tasks.producer1.outputs.param1}}", "param2": "{{tasks.producer2.outputs.param2}}" }'

Maybe this can be done even nicer using WithItems dictionary-passing capability.

Is there any problem with this approach?

[markterm]

@Ark-kun Something like that could be feasible, but by building strings without building mechanisms to do it natively is subject to escaping problems and could easily start making the yaml look really ugly ...

[Ark-kun]

building strings without building mechanisms to do it natively is subject to escaping problems

AFAIK, WithItems has native support for maps

template: spark-template
arguments:
- name: arguments
  value: "{{item}}"
  withItems:
  - param1: "{{tasks.producer1.outputs.param1}}"
    param2: "{{tasks.producer2.outputs.param2}}"

Having structural templates support can make this even better: #1684 #1293 (comment)

[markterm]

@Ark-kun Description of WithItems is that it "expands a task into multiple parallel tasks from the items in the list" - here we're just looking to run the task once, passing in a map.

I agree structural templates would be another option.

[Ark-kun]

Description of WithItems is that it "expands a task into multiple parallel tasks from the items in the list" - here we're just looking to run the task once, passing in a map.

Yes. The intent of WithItems is to launch multiple tasks. But it can also launch a single task. And it supports maps (syntactically correct).

[markterm]

@Ark-kun Can you give an example? If I do this then I get an error: failed to resolve {{item}}

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: hello-world-
spec:
  entrypoint: entry
  arguments:
    parameters:
    - name: param1
      value: hello world
    - name: param2
      value: hello world
  templates:
  - name: entry
    steps:
    - - name: step-1
        template: whalesay
        arguments:
          parameters:
          - name: arguments
            value: "{{item}}"
        withItems:
        - { passthroughParam1: '{{workflow.parameters.param1}}', passthroughParam2: '{{workflow.parameters.param2}}' }

  - name: whalesay
    container:
      image: docker/whalesay:latest
      command: [cowsay]
      args: ["{{input.parameters}}"]

[Ark-kun]

@Ark-kun Can you give an example? If I do this then I get an error: failed to resolve {{item}}

Oh. Strange. The 3rd and 4th examples here seemd to be implying that it's supported. I need to check. https://github.com/argoproj/argo/tree/master/examples#loops

[markterm]

@Ark-kun @dtaniwaki Just checking your current thinking on this PR ...

[dtaniwaki]

@mark9white Sorry for the late reply. Because of #1744, it might be hard to implement this PR's feature. As @Ark-kun instructed, we may have another approach for what you want to achieve. Let's discuss it before starting further implementation.

[markterm]

@Ark-kun Can you give an example? If I do this then I get an error: failed to resolve {{item}}

Oh. Strange. The 3rd and 4th examples here seemd to be implying that it's supported. I need to check. https://github.com/argoproj/argo/tree/master/examples#loops

I've created a PR to fix this (#1906), and am closing this PR.