From 6afe592957634a1622bf20b57d5ac239ea2df03a Mon Sep 17 00:00:00 2001
From: Val Sichkovskyi <bbc88ks@gmail.com>
Date: Tue, 16 Oct 2018 16:22:00 -0400
Subject: [PATCH] Parameter and Argument names should support snake case

---
 workflow/validate/validate.go      | 24 ++++++++++++---
 workflow/validate/validate_test.go | 47 +++++++++++++++++++++++++-----
 2 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go
index c9dca2a47bd4..83d813b7a485 100644
--- a/workflow/validate/validate.go
+++ b/workflow/validate/validate.go
@@ -464,7 +464,7 @@ func validateOutputs(scope map[string]interface{}, tmpl *wfv1.Template) error {
 			}
 		}
 		if art.GlobalName != "" && !isParameter(art.GlobalName) {
-			errs := isValidWorkflowFieldName(art.GlobalName)
+			errs := isValidParamOrArtifactName(art.GlobalName)
 			if len(errs) > 0 {
 				return errors.Errorf(errors.CodeBadRequest, "templates.%s.%s.globalName: %s", tmpl.Name, artRef, errs[0])
 			}
@@ -492,7 +492,7 @@ func validateOutputs(scope map[string]interface{}, tmpl *wfv1.Template) error {
 			}
 		}
 		if param.GlobalName != "" && !isParameter(param.GlobalName) {
-			errs := isValidWorkflowFieldName(param.GlobalName)
+			errs := isValidParamOrArtifactName(param.GlobalName)
 			if len(errs) > 0 {
 				return errors.Errorf(errors.CodeBadRequest, "%s.globalName: %s", paramRef, errs[0])
 			}
@@ -556,7 +556,14 @@ func validateWorkflowFieldNames(slice interface{}) error {
 		if name == "" {
 			return errors.Errorf(errors.CodeBadRequest, "[%d].name is required", i)
 		}
-		if errs := isValidWorkflowFieldName(name); len(errs) != 0 {
+		var errs []string
+		t := reflect.TypeOf(item)
+		if t == reflect.TypeOf(wfv1.Parameter{}) || t == reflect.TypeOf(wfv1.Artifact{}) {
+			errs = isValidParamOrArtifactName(name)
+		} else {
+			errs = isValidWorkflowFieldName(name)
+		}
+		if len(errs) != 0 {
 			return errors.Errorf(errors.CodeBadRequest, "[%d].name: '%s' is invalid: %s", i, name, strings.Join(errs, ";"))
 		}
 		_, ok := names[name]
@@ -715,13 +722,22 @@ func verifyNoCycles(tmpl *wfv1.Template, nameToTask map[string]wfv1.DAGTask) err
 
 var (
 	// paramRegex matches a parameter. e.g. {{inputs.parameters.blah}}
-	paramRegex = regexp.MustCompile(`{{[-a-zA-Z0-9]+(\.[-a-zA-Z0-9]+)*}}`)
+	paramRegex               = regexp.MustCompile(`{{[-a-zA-Z0-9]+(\.[-a-zA-Z0-9_]+)*}}`)
+	paramOrArtifactNameRegex = regexp.MustCompile(`^[-a-zA-Z0-9_]+[-a-zA-Z0-9_]*$`)
 )
 
 func isParameter(p string) bool {
 	return paramRegex.MatchString(p)
 }
 
+func isValidParamOrArtifactName(p string) []string {
+	var errs []string
+	if !paramOrArtifactNameRegex.MatchString(p) {
+		return append(errs, "Parameter/Artifact name must consist of alpha-numeric characters, '_' or '-' e.g. my_param_1, MY-PARAM-1")
+	}
+	return errs
+}
+
 const (
 	workflowFieldNameFmt    string = "[a-zA-Z0-9][-a-zA-Z0-9]*"
 	workflowFieldNameErrMsg string = "name must consist of alpha-numeric characters or '-', and must start with an alpha-numeric character"
diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go
index 235c92b5faad..886dfd126bbf 100644
--- a/workflow/validate/validate_test.go
+++ b/workflow/validate/validate_test.go
@@ -322,9 +322,7 @@ spec:
 
 func TestInvalidArgParamName(t *testing.T) {
 	err := validate(invalidArgParamNames)
-	if assert.NotNil(t, err) {
-		assert.Contains(t, err.Error(), invalidErr)
-	}
+	assert.NotNil(t, err)
 }
 
 var invalidArgArtNames = `
@@ -336,7 +334,7 @@ spec:
   entrypoint: kubectl-input-artifact
   arguments:
     artifacts:
-    - name: -kubectl
+    - name: "&-kubectl"
       http:
         url: https://storage.googleapis.com/kubernetes-release/release/v1.8.0/bin/linux/amd64/kubectl
 
@@ -344,7 +342,7 @@ spec:
   - name: kubectl-input-artifact
     inputs:
       artifacts:
-      - name: -kubectl
+      - name: "&-kubectl"
         path: /usr/local/bin/kubectl
         mode: 0755
     container:
@@ -423,7 +421,7 @@ spec:
     container:
       image: docker/whalesay
       command: [cowsay]
-      args: ["{{inputs.parameters.message}}"]
+      args: ["{{inputs.parameters.message+123}}"]
 `
 
 func TestInvalidInputParamName(t *testing.T) {
@@ -500,7 +498,7 @@ spec:
       args: ["cowsay hello world | tee /tmp/hello_world.txt"]
     outputs:
       artifacts:
-      - name: __1
+      - name: "!1"
         path: /tmp/hello_world.txt
 `
 
@@ -1074,6 +1072,41 @@ func TestSpecArgumentNoValue(t *testing.T) {
 	assert.NotNil(t, err)
 }
 
+var specArgumentSnakeCase = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: spec-arg-snake-case-
+spec:
+  entrypoint: whalesay
+  arguments:
+    artifacts:
+    - name: __kubectl
+      http:
+        url: https://storage.googleapis.com/kubernetes-release/release/v1.8.0/bin/linux/amd64/kubectl
+    parameters:
+    - name: my_snake_case_param
+      value: "hello world"
+  templates:
+  - name: whalesay
+    inputs:
+      artifacts:
+      - name: __kubectl
+        path: /usr/local/bin/kubectl
+        mode: 0755
+    container:
+      image: docker/whalesay:latest
+      command: [sh, -c]
+      args: ["cowsay {{workflow.parameters.my_snake_case_param}} | tee /tmp/hello_world.txt && ls  /usr/local/bin/kubectl"]
+`
+
+// TestSpecArgumentSnakeCase we allow parameter and artifact names to be snake case
+func TestSpecArgumentSnakeCase(t *testing.T) {
+	wf := unmarshalWf(specArgumentSnakeCase)
+	err := ValidateWorkflow(wf, true)
+	assert.Nil(t, err)
+}
+
 var specBadSequenceCountAndEnd = `
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow