Allow spec.arguments to be not supplied during linting.

Global parameters were not referencable from artifact arguments (resolves argoproj#791)

test/e2e/functional/global-param-sub-with-artifacts.yaml

@@ -0,0 +1,26 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+ generateName: global-param-sub-with-artifacts-
+spec:
+ entrypoint: foo
+ arguments:
+ parameters:
+ - name: artifact-url
+ value: https://date.jsontest.com
+ artifacts:
+ - name: foo-artifact
+ http:
+ url: "{{workflow.parameters.artifact-url}}"
+
+ templates:
+ - name: foo
+ inputs:
+ artifacts:
+ - name: foo-artifact
+ path: /tmp/foo.txt
+ mode: 755
+ container:
+ image: alpine:latest
+ command: ["cat"]
+ args: ["/tmp/foo.txt"]

test/e2e/functional/param-sub-with-artifacts.yaml

@@ -0,0 +1,27 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+ generateName: param-sub-with-artifacts-
+spec:
+ entrypoint: foo
+ arguments:
+ parameters:
+ - name: artifact-url
+ value: https://date.jsontest.com
+
+ templates:
+ - name: foo
+ inputs:
+ parameters:
+ - name: artifact-url
+ artifacts:
+ - name: foo-artifact
+ path: /tmp/foo.txt
+ mode: 755
+ http:
+ url: "{{inputs.parameters.artifact-url}}"
+
+ container:
+ image: alpine:latest
+ command: ["cat"]
+ args: ["/tmp/foo.txt"]

workflow/common/util.go

@@ -121,9 +121,10 @@ func DefaultConfigMapName(controllerName string) string {
 }
 
 // ProcessArgs sets in the inputs, the values either passed via arguments, or the hardwired values
-// It also substitutes parameters in the template from the arguments
-// It will also substitute any global variables referenced in template
-// (e.g. {{workflow.parameters.XX}}, {{workflow.name}}, {{workflow.status}})
+// It substitutes:
+// * parameters in the template from the arguments
+// * global parameters (e.g. {{workflow.parameters.XX}}, {{workflow.name}}, {{workflow.status}})
+// * local parameters (e.g. {{pod.name}})
 func ProcessArgs(tmpl *wfv1.Template, args wfv1.Arguments, globalParams, localParams map[string]string, validateOnly bool) (*wfv1.Template, error) {
  // For each input parameter:
  // 1) check if was supplied as argument. if so use the supplied value from arg
@@ -146,12 +147,8 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.Arguments, globalParams, localPa
  }
  tmpl.Inputs.Parameters[i] = inParam
  }
- tmpl, err := substituteParams(tmpl, globalParams, localParams)
- if err != nil {
- return nil, err
- }
 
- // Performs susbstitution of input artifacts
+ // Performs substitutions of input artifacts
  newInputArtifacts := make([]wfv1.Artifact, len(tmpl.Inputs.Artifacts))
  for i, inArt := range tmpl.Inputs.Artifacts {
  // if artifact has hard-wired location, we prefer that
@@ -172,7 +169,8 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.Arguments, globalParams, localPa
  newInputArtifacts[i] = *argArt
  }
  tmpl.Inputs.Artifacts = newInputArtifacts
- return tmpl, nil
+
+ return substituteParams(tmpl, globalParams, localParams)
 }
 
 // substituteParams returns a new copy of the template with global, pod, and input parameters substituted

workflow/common/validate.go

@@ -33,19 +33,27 @@ const (
  anyItemMagicValue = "item.*"
 )
 
-// ValidateWorkflow accepts a workflow and performs validation against it
-func ValidateWorkflow(wf *wfv1.Workflow) error {
+// ValidateWorkflow accepts a workflow and performs validation against it. If lint is specified as
+// true, will skip some validations which is permissible during linting but not submission
+func ValidateWorkflow(wf *wfv1.Workflow, lint ...bool) error {
  ctx := wfValidationCtx{
  wf: wf,
  globalParams: make(map[string]string),
  results: make(map[string]bool),
  }
+ linting := len(lint) > 0 && lint[0]
 
  err := validateWorkflowFieldNames(wf.Spec.Templates)
  if err != nil {
  return errors.Errorf(errors.CodeBadRequest, "spec.templates%s", err.Error())
  }
- err = validateArguments("spec.arguments.", wf.Spec.Arguments)
+ if linting {
+ // if we are just linting we don't care if spec.arguments.parameters.XXX doesn't have an
+ // explicit value. workflows without a default value is a desired use case
+ err = validateArgumentsFieldNames("spec.arguments.", wf.Spec.Arguments)
+ } else {
+ err = validateArguments("spec.arguments.", wf.Spec.Arguments)
+ }
  if err != nil {
  return err
  }
@@ -259,6 +267,14 @@ func validateLeaf(scope map[string]interface{}, tmpl *wfv1.Template) error {
 }
 
 func validateArguments(prefix string, arguments wfv1.Arguments) error {
+ err := validateArgumentsFieldNames(prefix, arguments)
+ if err != nil {
+ return err
+ }
+ return validateArgumentsValues(prefix, arguments)
+}
+
+func validateArgumentsFieldNames(prefix string, arguments wfv1.Arguments) error {
  fieldToSlices := map[string]interface{}{
  "parameters": arguments.Parameters,
  "artifacts": arguments.Artifacts,
@@ -269,12 +285,16 @@ func validateArguments(prefix string, arguments wfv1.Arguments) error {
  return errors.Errorf(errors.CodeBadRequest, "%s%s%s", prefix, fieldName, err.Error())
  }
  }
+ return nil
+}
+
+// validateArgumentsValues ensures that all arguments have parameter values or artifact locations
+func validateArgumentsValues(prefix string, arguments wfv1.Arguments) error {
  for _, param := range arguments.Parameters {
  if param.Value == nil {
  return errors.Errorf(errors.CodeBadRequest, "%svalue is required", prefix)
  }
  }
-
  for _, art := range arguments.Artifacts {
  if art.From == "" && !art.HasLocation() {
  return errors.Errorf(errors.CodeBadRequest, "%sfrom or artifact location is required", prefix)

workflow/common/validate_test.go

@@ -1046,3 +1046,30 @@ func TestGlobalParamWithVariable(t *testing.T) {
  err := ValidateWorkflow(test.GetWorkflow("functional/global-outputs-variable.yaml"))
  assert.Nil(t, err)
 }
+
+var specArgumentNoValue = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+ generateName: spec-arg-no-value-
+spec:
+ entrypoint: whalesay
+ arguments:
+ parameters:
+ - name: required-param
+ templates:
+ - name: whalesay
+ container:
+ image: docker/whalesay:latest
+ command: [sh, -c]
+ args: ["cowsay hello world | tee /tmp/hello_world.txt"]
+`
+
+// TestSpecArgumentNoValue we allow parameters to have no value at the spec level during linting
+func TestSpecArgumentNoValue(t *testing.T) {
+ wf := unmarshalWf(specArgumentNoValue)
+ err := ValidateWorkflow(wf, true)
+ assert.Nil(t, err)
+ err = ValidateWorkflow(wf)
+ assert.NotNil(t, err)
+}

workflow/controller/operator_test.go

@@ -5,6 +5,7 @@ import (
  "testing"
 
  wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
+ "github.com/argoproj/argo/test"
  "github.com/argoproj/argo/workflow/common"
  "github.com/stretchr/testify/assert"
  apiv1 "k8s.io/api/core/v1"
@@ -789,3 +790,27 @@ func TestAddGlobalArtifactToScope(t *testing.T) {
  assert.Equal(t, art.GlobalName, woc.wf.Status.Outputs.Artifacts[1].Name)
  assert.Equal(t, "new/new/key", woc.wf.Status.Outputs.Artifacts[1].S3.Key)
 }
+
+func TestParamSubstitutionWithArtifact(t *testing.T) {
+ wf := test.GetWorkflow("functional/param-sub-with-artifacts.yaml")
+ woc := newWoc(*wf)
+ woc.operate()
+ wf, err := woc.controller.wfclientset.ArgoprojV1alpha1().Workflows("").Get(wf.ObjectMeta.Name, metav1.GetOptions{})
+ assert.Nil(t, err)
+ assert.Equal(t, wf.Status.Phase, wfv1.NodeRunning)
+ pods, err := woc.controller.kubeclientset.CoreV1().Pods("").List(metav1.ListOptions{})
+ assert.Nil(t, err)
+ assert.Equal(t, len(pods.Items), 1)
+}
+
+func TestGlobalParamSubstitutionWithArtifact(t *testing.T) {
+ wf := test.GetWorkflow("functional/global-param-sub-with-artifacts.yaml")
+ woc := newWoc(*wf)
+ woc.operate()
+ wf, err := woc.controller.wfclientset.ArgoprojV1alpha1().Workflows("").Get(wf.ObjectMeta.Name, metav1.GetOptions{})
+ assert.Nil(t, err)
+ assert.Equal(t, wf.Status.Phase, wfv1.NodeRunning)
+ pods, err := woc.controller.kubeclientset.CoreV1().Pods("").List(metav1.ListOptions{})
+ assert.Nil(t, err)
+ assert.Equal(t, len(pods.Items), 1)
+}

workflow/controller/workflowpod_test.go

@@ -5,7 +5,6 @@ import (
 
  wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
  "github.com/ghodss/yaml"
- log "github.com/sirupsen/logrus"
  "github.com/stretchr/testify/assert"
  apiv1 "k8s.io/api/core/v1"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -28,21 +27,13 @@ func newWoc(wfs ...wfv1.Workflow) *wfOperationCtx {
  } else {
  wf = &wfs[0]
  }
- woc := wfOperationCtx{
- wf: wf,
- orig: wf.DeepCopyObject().(*wfv1.Workflow),
- updated: false,
- log: log.WithFields(log.Fields{
- "workflow": wf.ObjectMeta.Name,
- "namespace": wf.ObjectMeta.Namespace,
- }),
- controller: newController(),
- completedPods: make(map[string]bool),
- }
- if woc.wf.Status.Nodes == nil {
- woc.wf.Status.Nodes = make(map[string]wfv1.NodeStatus)
+ fakeController := newController()
+ _, err := fakeController.wfclientset.ArgoprojV1alpha1().Workflows(wf.ObjectMeta.Namespace).Create(wf)
+ if err != nil {
+ panic(err)
  }
- return &woc
+ woc := newWorkflowOperationCtx(wf, fakeController)
+ return woc
 }
 
 // getPodName returns the podname of the created pod of a workflow