[cel] parse variables in context

[vincent-pli]

fix #716

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes docs (if user facing)
• Commit messages follow commit message best practices
• Commit messages includes a project tag in the subject - e.g. [OCI], [hub], [results], [task-loops]

See the contribution guide
for more details.

[vincent-pli]

/assign @jerop

[afrittoli]

Thanks @vincent-pli !
It's great to see this being used :)

A few small asks 🙏

• could you add "[cel]" in beginning of the commit message
• could you add a unit test too for the changes you've done?

[afrittoli]

/cc @jerop

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jerop
You can assign the PR to them by writing /assign @jerop in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• cel/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jerop]

Thanks for the contribution to this project @vincent-pli!

I have a couple of concerns about this change:

1. How can users distinguish between a static string and a context variable? In your example, a user can no longer use static strings name and types because they'll interpreted as variable strings that are substituted with hello and string respectively
2. In Pipelines, we don't substitute Parameter values with Results from the same Task so doing that here would be inconsistent and confusing to users
3. What happens when the order of the expressions is rearranged?
   1. This change introduces dependencies between the Parameters (Expressions) which requires that users are careful about the ordering of the Parameters (Expressions)
   2. Is the order of Parameters in the YAML maintained when marshalling & unmarshalling?

One way to solve issue the first concern would be to express the context variables as variables using $(...) to distinguish them from the static strings, that is:

# current proposal
apiVersion: tekton.dev/v1alpha1
kind: Run
metadata:
  generateName: celrun-with-context-
spec:
  ref:
    apiVersion: cel.tekton.dev/v1alpha1
    kind: CEL
  params:
    - name: name
      value: "'hello'"
    - name: types
      value: "type(name)"
    - name: expression-use-context
      value: "name + ' is type of ' + types"
---
# suggested proposal
apiVersion: tekton.dev/v1alpha1
kind: Run
metadata:
  generateName: celrun-with-context-
spec:
  ref:
    apiVersion: cel.tekton.dev/v1alpha1
    kind: CEL
  params:
    - name: name
      value: "'hello'"
    - name: types
      value: "type(name)"
    - name: expression-use-context
      value: "$(result.name) + ' is type of ' + $(result.types)"

An alternative approach I'd prefer would be to support substituting Parameters from the Pipeline and Results from other Tasks in CEL Custom Tasks, similarly to what we do in the rest of the Pipeline, something like:

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: example-pr-
spec:
  params:
    - name: name
      value: "hello"
  pipelineSpec:
    params:
      - name: name
        type: string
    tasks:
      - name: get-type
        taskRef:
          apiVersion: cel.tekton.dev/v1alpha1
          kind: CEL
        params:
          - name: type
            value: "$(params.name)"
      - name: use-type
        taskRef:
          apiVersion: cel.tekton.dev/v1alpha1
          kind: CEL
        params:
          - name: expression-use-type
            value: "$(params.name) is of type $(tasks.get-type.results.type)"

separately, could you please add documentation updates as well to describe the change?

[vincent-pli]

@jerop thanks for comments.
I agree with you it will introduce dependencies in Parameters (order of list/array in yaml is preserved and Parameters is Array).
The subsequent parameter adopt the parameter defined previously for expression, I think it's make sense. Actually it's exactly what I want. if no such ability we have to defined multiple Task for CEL one by one, and use $(task.result.xxx) in the last expression, it's complex for pipeline designer and will cause bad performance.

Basically, it's what CEL expect to, the cel.NewEnv and prg.Eval is design for it. Seems CEL let user define the expression with variables(a express template) firstly then do compile and verification then parse it with runtime parameters:

out, details, err := prg.Eval(map[string]interface{}{
    "name": "/groups/acme.co/documents/secret-stuff",
    "group": "acme.co"})
fmt.Println(out) // 'true'

So if we substitute expression before enter CEL, I believe it could work but introduce complex and not necessary.
I think this project is not only serve for when express but for more cases and the new introduced functionality will not hurt the when express scenario.

The user should use quotation(single or double) marks for static string, I means "types" and "name", without quotation will treat as variable.

[jerop]

@vincent-pli thanks for the detailed response!

order of list/array in yaml is preserved

that's right, the parameters order is preserved deterministic since tektoncd/pipeline#3024 (will need to add docs for that though)

it's what CEL expect to, the cel.NewEnv and prg.Eval is design for it

makes sense, we can try it out and iterate as needed given it's experimental

the user should use quotation(single or double) marks for static string, I means "types" and "name", without quotation will treat as variable

please add documentation for this in the PR, I want to make sure that static vs variable strings is clear to users

[vincent-pli]

@jerop thanks
Add some description in the README, please take a look.

[jerop]

@vincent-pli thank you for adding the documentation and patience on this issue!

1. order of parameters/expressions
   there's an assumption here that the parameters/expressions would be evaluated in the order in which they were declared, but @bobcatfish clarified that while the order was made deterministic in Make string variable interpolation deterministic, and single-pass. pipeline#3024 it's not necessarily the order in which the user declared them

2. static vs variable strings
   thanks for adding the docs docs which are a great improvement but the distinction between the static and variable strings in the parameters/expressions still seems unclear to me

3. use cases
   thanks for providing the simple example -- it would really help if you could also provide a CI/CD use case that would help us understand the issue better and find the best solution for it

wondering what the other CEL owners think about this, /cc @bobcatfish @imjasonh @pritidesai @bigkevmcd

[vincent-pli]

@jerop
For the order things, if we insist to it should no dependency between params, we can turn to tektoncd/community#403, but will lost some flexibility. @bobcatfish

[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.

[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: Closed this PR.

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.