Arguments of type array cannot be access via index

[jtestard]

Expected Behavior

When using a parameter of type Array and trying to access access a specific index, the variable substitution does not occur. Assuming the following pipeline spec, I would expect the tasks generated to have the value 'false' be substituted.

apiVersion: tekton.dev/v1beta1
kind: Pipeline
...
spec:
  params:
    - disabled:
        - 'false'
        - 'false'
        - 'false'
        - 'false'
...
  tasks:
    - name: ...
      params:
        - name: disabled
          value: '$(params.disabled[2])'

Actual Behavior

As can be seen here once the pipeline is run (seen through dashboard) , the variable is not substituted.

Steps to Reproduce the Problem

Outlined above

Additional Info

• Kubernetes version:

  Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"14", GitVersion:"v1.14.8", GitCommit:"211047e9a1922595eaa3a1127ed365e9299a6c23", GitTreeState:"clean", BuildDate:"2019-10-15T12:11:03Z", GoVersion:"go1.12.10", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"15+", GitVersion:"v1.15.12-gke.2", GitCommit:"fb7add51f767aae42655d39972210dc1c5dbd4b3", GitTreeState:"clean", BuildDate:"2020-06-01T22:20:10Z", GoVersion:"go1.12.17b4", Compiler:"gc", Platform:"linux/amd64"}

• Tekton Pipeline version:

  Output of tkn version or kubectl get pods -n tekton-pipelines -l app=tekton-pipelines-controller -o=jsonpath='{.items[0].metadata.labels.version}'

Client version: 0.11.0
Pipeline version: v0.14.3
Triggers version: v0.7.0

[vdemeester]

Hey @jtestard, thx for the issue. I think it's not clearly documented in here, but we do not currently support accessing array values by index.

/cc @skaegi @bobcatfish

[jtestard]

I noticed a typo indeed: the pipeline parameter name is disabled and matches that of the task parameter. Fixed.

[jtestard]

@vdemeester We are using Tekton in production at our startup, pretty eager on the tech. However, this (among other things) is a blocker to get us where we want to be. Is this the kind of issue an external contributor (us) could easily tackle could get things going faster?

[vdemeester]

@vdemeester We are using Tekton in production at our startup, pretty eager on the tech. However, this (among other things) is a blocker to get us where we want to be. Is this the kind of issue an external contributor (us) could easily tackle could get things going faster?

@jtestard sure 😉 I wonder if this needs a TEP or not (my guess is that we may not but… it's kinda user-facing 🤔)

[ghost]

I wonder if this needs a TEP or not

I think it's probably a good idea. We've had other proposals for this problem that suggest a more wide-ranging solution (see the JSONPath work @skaegi started for instance here and, related, here). And we've had similar requests asking for CEL. So there's been some different proposals. It seems like a good idea, if we're going to expand our variable access features, to write something up and talk it through as a community.

WDYT?

[jtestard]

@sbwsg So only adding index-based array access would be too incremental a change. I don't know if we have the resources on our end to assist with adding support for a whole new language such as JSONPath/CEL. Let us know if/how we can help

[ghost]

So only adding index-based array access would be too incremental a change.

Ah, that wasn't quite what I was trying to suggest, sorry. I agree with what @vdemeester suggested: this is a user-facing change. And this is an area that's had people propose changes to it in the past. So, given it's user-facing and we've got other folks in the community who are interested in this area, I think that writing up a short TEP to declare the change you're proposing is the way to go.

For what it's worth I think the incremental improvement is a good idea. But I just want to make sure it gets surfaced to the right folks before we commit to it getting implemented and merged.

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
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 with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[ghost]

/remove-lifecycle rotten

Keeping this one alive because I think array params could still use some improvements like this to make them far more useful.

[jerop]

CEL Custom Tasks is now available and can be used for indexing and other array functions on parameters: https://github.com/tektoncd/experimental/tree/master/cel

Taking this example PipelineRun with contains and indexing:

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: pr-indexing-
spec:
  pipelineSpec:
    params:
      - name: alpha
        type: string
    tasks:
      - name: indexing
        taskRef:
          apiVersion: cel.tekton.dev/v1alpha1
          kind: CEL
        params:
          - name: c-exists
            value: "'c' in $(params.alpha)"
          - name: at-index-1
            value: "$(params.alpha)[1]"
  params:
    - name: alpha
      value: "['a', 'b', 'c']"

When executed, the CEL Custom Task produces evaluation Results that can be used in subsequent tasks:

Name:         pr-indexing-d47gj
Namespace:    default
API Version:  tekton.dev/v1beta1
Kind:         PipelineRun
Metadata:
  Creation Timestamp:  2021-01-22T16:13:22Z
  Generate Name:       pr-indexing-
# […]
Status:
  Completion Time:  2021-01-22T16:13:22Z
  Conditions:
    Last Transition Time:  2021-01-22T16:13:22Z
    Message:               Tasks Completed: 1 (Failed: 0, Cancelled 0), Skipped: 0
    Reason:                Succeeded
    Status:                True
    Type:                  Succeeded
  Pipeline Spec:
    # […]
  Runs:
    pr-indexing-d47gj-indexing-2t9vr:
      Pipeline Task Name:  indexing
      Status:
        Completion Time:  2021-01-22T16:13:22Z
        Conditions:
          Last Transition Time:  2021-01-22T16:13:22Z
          Message:               CEL expressions were evaluated successfully
          Reason:                EvaluationSuccess
          Status:                True
          Type:                  Succeeded
        Extra Fields:            <nil>
        Observed Generation:     1
        Results:
          Name:      c-exists
          Value:     true
          Name:      at-index-1
          Value:     b
        Start Time:  2021-01-22T16:13:22Z
  Start Time:        2021-01-22T16:13:22Z
Events:
  Type    Reason     Age   From         Message
  ----    ------     ----  ----         -------
  Normal  Started    12s   PipelineRun  
  Normal  Running    12s   PipelineRun  Tasks Completed: 0 (Failed: 0, Cancelled 0), Incomplete: 1, Skipped: 0
  Normal  Succeeded  12s   PipelineRun  Tasks Completed: 1 (Failed: 0, Cancelled 0), Skipped: 0

@jtestard if you use this, please share any feedback on your experience and how we can make it better 😄

TEP: tektoncd/community#314

(note: CEL Custom Tasks is currently an experimental project)

[jerop]

Keeping this one alive because I think array params could still use some improvements like this to make them far more useful.

@sbwsg another alternative we could consider, if CEL custom tasks is promoted from experimental, is deprecating array params and supporting string params only for simplicity and flexibility (avoiding to implement our own expression syntax)

[ghost]

@sbwsg another alternative we could consider, if CEL custom tasks is promoted from experimental, is deprecating array params and supporting string params only for simplicity and flexibility (avoiding to implement our own expression syntax)

Very interesting! I agree this would solve a lot of different problems we have with the existing array syntax's complexity and offer flexibility on syntax choice / behaviour. I wonder how we could handle situations where a user passes an array of varying length and this needs to be merged / appended to a Step's command or args list.

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
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 with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[ghost]

@jtestard have you tried the CEL custom task that @jerop mentioned above? It doesn't directly handle tekton's concept of "array params" but you can pass it a stringified array and process it with CEL.

[bobcatfish]

I'm creating a proposal to add specifically support for indexing into arrays; however I'm limiting the proposal to that so if we want to talk about full JSONPath (or even CEL) support that would have to be a separate proposal.

/remove-lifecycle rotten

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
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 with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[bobcatfish]

Want to keep this one open, still hoping to address this via TEP-0076

/remove-lifecycle stale

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[bobcatfish]

Still happening I promise! 🙏

/remove-lifecycle stale

[tekton-robot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.