Pipeline: Consider removing type from params (or _really_ support types)

[skaegi]

We currently have params of type string and array with string being far more typical. Support for the array type does not strictly seem required and can be worked around without undue hardship. On the flip-side interpolation of an array type param inside of a string is both limited and not terribly intuitive. In addition all use of $(variable) interpolation in core Kubernetes is string based.

I'm proposing we remove type support from Param and ParamSpec for now until we're sure what we want to do with param types.

[skaegi]

I was thinking about this issue in the context of beta and in particular how @afrittoli and @vdemeester mentioned that the Array "merge" support was solving a real problem they had in the plumbing project.

I think removing Array support is just going to be a big pain and also believe that we want to support Param typing in the futures so... instead of removing it we should just ensure our current use oftype is future safe and move on. fwiw I believe it is safe.

So... my only real reservation is with the merge syntax of the array type -- https://github.com/tektoncd/pipeline/blob/master/docs/tasks.md#variable-substitution-with-parameters-of-type-array

["first", "$(params.array-param)", "last"]

It's not bad, but gives the impression that the array-param itself will be added to the array as opposed to being merged in and there maybe cases in the future where that is the desired behavior. I do not want to encourage too much smarts in our interpolation but I would suggest borrowing some bash syntax to express the array being expanded helps here.

["first", "$(params.array-param[@])", "last"]

Anyway, I'll wait a few days for feedback but then open a new issue just for the array merge syntax and assign it to 0.9 and close this issue.

[skaegi]

Actually a slightly better syntax in this case is to use a spread operator. e.g. ...

["first", "$(...params.array-param)", "last"]

Our lookup syntax is closer to a json lookup than flat bash interpolation. A spread operator is slightly more future-proof and from a parser point of view does not introduce a new magic encoding rule

[bobcatfish]

If we do want to make the syntax more powerful, it would be great if we could align with Triggers which is already using a more powerful syntax (https://github.com/tektoncd/triggers/blob/master/docs/triggerbindings.md#event-variable-interpolation) tho also currently debating which syntax to use (https://github.com/tektoncd/triggers/issues/178)

• @dibyom

[dibyom]

I'm all for aligning on the syntax and reusing an existing one like CEL or JSONPath instead of inventing our own! :angel:

[skaegi]

For param lookup we currently support "dot-notation". (Hurray!)
"Bracket-notation" is used elsewhere in Kubernetes to support cases where . (or any other path-specific character is used) and I logged #1453 for that already. I've looked at our param substitution logic and there is work to do for that certainly, but that might be something I could tackle. So...

object.property
object['property']
object["property"]

To that the only new accessor we "might" want to add is "bracket-notation" for indexed array access. I suspect there may be existing precedent for that in Kubernetes too but haven't looked and cannot see how the syntax would be controversial. So...

array[24]

For "accessors" I think that syntax is already pretty standard and likely sufficient. I'm still hoping we can avoid the need for strong predicate support like in JSONPath etc. but who knows. One reservation I have is that unlike simple property access there is no defacto standard for predicates.

[skaegi]

@dibyom Reading the triggers issue... https://github.com/kubernetes/client-go/tree/master/util/jsonpath looks like it might be reasonable -- the minimal syntax above is of course more than covered by JSONPath. e.g. We might not need all of JSONPath but that's the syntax we can/should align with if we add new capabilities.

[skaegi]

For the typing piece of parameters I'm looking at .. https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#parameterObject -- this might feel verbose but I'm actually only concerned about the type aspect or "schema" let me try to put together an example.

e.g. where schema is an instance of ... https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.16/#jsonschemaprops-v1-apiextensions-k8s-io

Also see... https://json-schema.org/understanding-json-schema/basics.html

[skaegi]

The idea here would be to use schema instead of type since 'type' is just one field of a schema. Some param examples that use schema as a mechanism to express type...

params:
  - name: mystring
    description: best string ever
    default: best-default-ever
    schema:
      type: string
  - name: mystringarray
    description: best string array ever
    default: ["ant","bee"]
    schema:
      type: array
      items:
        type: string
      minItems: 1
  - name: catstruct
    description: best struct ever?
    schema:
      type: object
      properties:
        cat_name:
          type: string
        cat_breed:
          type: string
          enum:
            - persian
            - siamese
            - himalayan
      examples:
        - cat_name: mabel
          cat_breed: siamese
        - cat_name: yeti
          cat_breed: himalayan 

In terms of implementation change what this would mean is that Param.Value and ParamSpec.Default would be parsed into a flexible type like interface{} (or a JSONValue) and validated by ParamSpec.Schema if present.

There obviously is a bunch of work here to get interpolation working properly but the general idea is that we would follow JSONPath style lookup to support extracting fields from structures and arrays. For example:

1. $(params.mystringarray[0] could be used to get the value ant for our default string array.
2. $(params.catstruct.cat_breed) could be used to get the value himalayan for our example cat named yeti.

In terms of how parameter values are interpolated they should be interpreted as emitting their character representation into the parse stream. For the scalar types this is more-or-less a fancy way of saying string substitution with the exception that string types should omit quotes when concatenated with an existing string. For array and objects they should emit their structure in-place where they were referenced. For example for myval: $(params.catstruct) and again using our cat named yeti, we would interpolate the result as...

myval:
  cat_name: yeti
  cat_breed: himalayan

Going back to the original array example from a few comments ago it makes sense to align with JSONPath for expansion syntax so we would use ["first", "$(params.array-param[])", "last"] to expand out an array. This actually really works well now as array-param[] creates a comma separated list of values (which serializes to "val1", "val2") as opposed to an array ["val1", "val2"]. So.... $(params.array-param) is equal to [$(params.array-param[*]] but $(params.array-param) is not equal to $(params.array-param[*] e.g. the square brackets matter.

[skaegi]

/assign

[bobcatfish]

This is a cool idea @skaegi - and I think it ties into some discussions we've had before about validating params before as well. I'm really excited we have someone looking closely at this!! :D

I think this could be great for folks that want to express complicated types and restrictions - do you have any thoughts on how we might be able to support this and also more simple use cases? e.g. say I just want a string, and I want it to be non-empty. Or I want to default it.

I'm thinking to support those use cases we'd want to have some simple "types" out of the box?

[skaegi]

so... I think the schema and jsonpath stuff is probably post "beta". I think the above approach is probably reasonable and good enough that we should also firm up plans for what we want to support in beta so as to avoid future problems. I'll open a separate issue for the schema/jsonpath stuff as my intent is to continue moving on it but again... post beta unless people feel strongly.

This issue can just focus on beta and the original issue title ;)

So... for beta I propose that we...
1) remove "type" (we will silently ignore it if present or else have the admission webhook remove it)
2) for beta only support params that are "string" or "[]string" -- implicit default schema contract for now and forever
3) Support the jsonpath $(params.array-param[*] syntax for expanding out arrays. I unfortunately think we should "break" the current usage pattern where the expansion is implicit based on where it's used as I believe it will cause future problems. We of course can and should do this over a release or two, but for beta I'd suggest this is an honest breaking change.

[vdemeester]

So... for beta I propose that we...

1. remove "type" (we will silently ignore it if present or else have the admission webhook remove it)
2. for beta only support params that are "string" or "[]string"  -- implicit default schema contract for now and forever

If we remove type, how does the user (and/or consumer like the cli) know if it should provide a string or an array of string []string ? Same question goes for the validation part of it : right now we are validating that the array is not "mis-used" (or at least we try). Removing the type will remove the possibility of doing such a check.

3. Support the jsonpath `$(params.array-param[*])` syntax for expanding out arrays. I unfortunately think we should "break" the current usage pattern where the expansion is implicit based on where it's used as I believe it will cause future problems. We of course can and should do this over a release or two, but for beta I'd suggest this is an honest breaking change.

I'm fine with "breaking" stuff now, before beta :angel:

[skaegi]

I just noticed I never came back and updated this issue. For beta (and for the rest of time) we will continue to have type which is currently limited to string and array. In the future when we have full jsonpath support I would like to expand this list to include object, boolean, and number. Even further in the future we can consider also supporting using a schema to further restrict the set of possibilities.

[popcor255]

For param lookup we currently support "dot-notation". (Hurray!)
"Bracket-notation" is used elsewhere in Kubernetes to support cases where . (or any other path-specific character is used) and I logged #1453 for that already. I've looked at our param substitution logic and there is work to do for that certainly, but that might be something I could tackle. So...

object.property
object['property']
object["property"]

To that the only new accessor we "might" want to add is "bracket-notation" for indexed array access. I suspect there may be existing precedent for that in Kubernetes too but haven't looked and cannot see how the syntax would be controversial. So...

array[24]

For "accessors" I think that syntax is already pretty standard and likely sufficient. I'm still hoping we can avoid the need for strong predicate support like in JSONPath etc. but who knows. One reservation I have is that unlike simple property access there is no defacto standard for predicates.

I believe the accessors listed in this parameter is on the param type array. Does this issue include indexing the params array in a task?

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[vdemeester]

/remove-lifecycle stale
/lifecycle frozen