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 for Resource creation where template has same parameter templating #1283

Merged
merged 11 commits into from
Mar 27, 2019
32 changes: 32 additions & 0 deletions test/e2e/functional/custom_template_variable.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# This template demonstrates the customer variable suppport.
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: custom-template-variable-
spec:
entrypoint: hello-hello-hello

templates:
- name: hello-hello-hello
steps:
- - name: hello1
template: whalesay
arguments:
parameters: [{name: message, value: "hello1"}]
- - name: hello2a
template: whalesay
arguments:
parameters: [{name: message, value: "hello2a"}]
- name: hello2b
template: whalesay
arguments:
parameters: [{name: message, value: "hello2b"}]

- name: whalesay
inputs:
parameters:
- name: message
container:
image: docker/whalesay
command: [cowsay]
args: ["{{custom.variable}}"]
3 changes: 3 additions & 0 deletions workflow/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ const (
KubeConfigDefaultVolumeName = "kubeconfig"
)

// GlobalVarWorkflowRootTags is a list of root tags in workflow which could be used for variable reference
var GlobalVarValidWorkflowVariablePrefix = [6]string{"item", "steps", "inputs", "outputs", "workflow", "tasks"}
Copy link
Member

Choose a reason for hiding this comment

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

  1. outputs is not a tag that we use
  2. pod is missing
  3. style nit: the [6] seems unnecessary:
var GlobalVarValidWorkflowVariablePrefix = []string{"item", "steps", "inputs", "outputs", "workflow", "tasks"}

Copy link
Member Author

Choose a reason for hiding this comment

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

sure


// ExecutionControl contains execution control parameters for executor to decide how to execute the container
type ExecutionControl struct {
// Deadline is a max timestamp in which an executor can run the container before terminating it
Expand Down
15 changes: 15 additions & 0 deletions workflow/validate/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,11 @@ func resolveAllVariables(scope map[string]interface{}, tmplStr string) error {
fstTmpl := fasttemplate.New(tmplStr, "{{", "}}")

fstTmpl.ExecuteFuncString(func(w io.Writer, tag string) (int, error) {

// Skip the custom variable references
if !checkValidWorkflowVariablePrefix(tag) {
return 0, nil
}
_, ok := scope[tag]
if !ok && unresolvedErr == nil {
if (tag == "item" || strings.HasPrefix(tag, "item.")) && allowAllItemRefs {
Expand All @@ -245,6 +250,16 @@ func resolveAllVariables(scope map[string]interface{}, tmplStr string) error {
return unresolvedErr
}

// checkValidWorkflowVariablePrefix is a helper methood check variable starts workflow root elements
func checkValidWorkflowVariablePrefix(tag string) bool {
for _, rootTag := range common.GlobalVarValidWorkflowVariablePrefix {
if strings.HasPrefix(tag, rootTag) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add . to HasPrefix (e.g. rootTag + '.') so that things like stepsfoo won't match but steps. will match?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch. Thanks. I will fix it

return true
}
}
return false
}

func validateNonLeaf(tmpl *wfv1.Template) error {
if tmpl.ActiveDeadlineSeconds != nil {
return errors.Errorf(errors.CodeBadRequest, "templates.%s.activeDeadlineSeconds is only valid for leaf templates", tmpl.Name)
Expand Down