TaskSpec Params needs an Array type

[tanner-bruce]

In https://github.com/knative/build-pipeline/blob/8a2b6c30efa7f4dd1fedafacc2733a0d290e12c6/examples/deploy_tasks.yaml#L21, the Input Param helmArgs is used. If one were to set helmArgs to be --set arg1=a --set arg2=b with the intention of providing multiple arguments to helm, this task would fail due to --set arg1=a --set arg2=b being interpreted as a single argument.

A couple questions that comes to mind:

1. How do we make it obvious that '${helmArgs}' could possibly be expanded to multiple arguments, while also allowing it to have a single argument? The spread operator may be a nice fit: ${helmArgs...}

2.How should a TaskParam represent an array? Currently it just has a name field, but description and default will be added soon. How does it look supporting defaults for an array alongside a regular string?

[tanner-bruce]

This could be done something like the following, with the idea borrowed from IntOrStr.

type ArrayOrStr struct {
	StringVal string
	ArrayVal []string
	Type Type
}

func (as ArrayOrStr) String() string {
	if as.Type == String { 
		return as.StringVal
	}
	return strings.Join(as.ArrayVal, " ")
}

type Type int

const (
	String Type = iota
	Array
)

// UnmarshalJSON implements the json.Unmarshaller interface.
func (as *ArrayOrStr) UnmarshalJSON(value []byte) error {
	if value[0] == '"' {
		as.Type = String
		return json.Unmarshal(value, &as.StringVal)
	}
	as.Type = Array
	return json.Unmarshal(value, &as.ArrayVal)
}

This could work nicely, as there is no additional field needed and is compatible with already created resources. Alongside this, introducing the spread operator, ..., in the interpolation phase would allow supporting arrays in a simple manner

I propose:

• add this new type,ArrayOrStr
• change the Param and TaskParam Value fields types to be ArrayOrStrs
• add a new operator, ..., to force treating the param as an array during replacement

[bobcatfish]

@tanner-bruce could you show an example of what the corresponding Task yaml would look like with your proposal?

My personal preference is for the type of the param to be explicit, e.g.:

        params:
            - name: testImage
              type: string
            - name: testArgs
              type: array

And then the use would look like:

          params:
              - name: testArgs
                value:
                     - "--set"
                     - "arg1=a"

(maybe this is the same thing you're suggesting!)

Alongside this, introducing the spread operator, ..., in the interpolation phase would allow supporting arrays in a simple manner

I'd like to understand a bit more about why we need the spread operator, in my mind using the args was {$testArgs} still makes sense, without introducing the spread operator?

(Sorry for the push back @tanner-bruce , I think we might need a bit more back and forth on this one before we're all clear on it, thanks for working on this ❤️ !)

[tanner-bruce]

@bobcatfish The main reason for ... was to allow the user to treat the array as a single string, however I've been trying to think of a use case for that, and can't.

Thinking more about it, adding the type field to the Task, would allow better, more explicit validation of the Task and TaskRun and likely lead to less user/runtime errors.

With that, I would change my proposal to be:

1. add the ArrayOrStr type for use with the value and default fields in TaskParam
2. add a ParamType type, and add it to input params. This would only be used during validation, as the type can also be gotten from ArrayOrStr
3. change the interpolation mechanisms to expand out array type params

[bobcatfish]

@tanner-bruce sounds good to me! Lets do it :D

[vdemeester]

/assign

[vdemeester]

/assign EliZucker
@EliZucker assigning this to you (given your comment on #1037)

[xihaLong]

I gave some feedback on the design document. I do not see a link to the design document here yet, so: https://docs.google.com/document/d/1A_4wWFGQ6puNk1KpM_cuyVRnES2JP6-5XjceYenUh2g

[skaegi]

@EliZucker I'm sorry to pick on a closed issue but I think there are a few things here that we need to think about... (perhaps a few already have an answer)

1. How do we represent an array parameter with a CLI
2. Is args the only use-case? This is not something the container spec's env/arg based substitution supports so we should be careful. If we want to get param values "from" configmaps or secrets in the future only strings are supported there too.
3. Context based type conversion has a history of being a footgun -- e.g. docker run $(IMAGE) - we might consider an operator ( e.g. $(...MYARRAY) or $(MYARRAY[@]) to make the conversion explicit even if correct use of "type" might prevent problems
   --
   I understand the use-case and I'm very interested in getting params right but I think array support is something we might want to take a bit of time to make sure is right and also make certain is not a mis-feature. Although perhaps not as nice looking this can be solved in user space with a shell script that parses out a delimiter in a param or in a preprocessor that directly populates args with a merged array.