Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support referencing task results from task params in a pipeline #1978

Closed
ghost opened this issue Jan 29, 2020 · 11 comments · Fixed by #2042
Closed

Support referencing task results from task params in a pipeline #1978

ghost opened this issue Jan 29, 2020 · 11 comments · Fixed by #2042
Labels
kind/feature Categorizes issue or PR as related to a new feature. maybe-next-milestone For consideration when planning the next milestone priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@ghost
Copy link

ghost commented Jan 29, 2020

Expected Behavior

A user should be able to author a pipeline and link the results from one task into the params of another. In the short term my suggestion here is to implement this using "from" syntax that is similar to how pipeline resources and workspaces refer to one another. In the slightly longer term I suggest variable interpolation as a syntax sugar. Here's how a complete pipeline + task yaml might look with the "from" syntax (modified from an example YAML that @skaegi put together here)

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: add
spec:
  params:
    - name: first
      description: the first operand
    - name: second
      description: the second operand
  results:
    - name: sum
      description: the sum of the first and second operand
  steps:
    - name: add
      image: alpine
      env:
        - name: OP1
          value: $(params.first)
        - name: OP2
          value: $(params.second)        
      script: echo $((${OP1}+${OP2})) | tee /workspace/results/sum
---
apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: sum-three 
spec:
  params:
    - name: first
      description: the first operand
    - name: second
      description: the second operand
    - name: third
      description: the third operand      
  tasks:
    - name: first-add
      taskRef:
        name: add
      params:
        - name: first
          value: $(params.first)
        - name: second
          value: $(params.second)
    - name: second-add
      taskRef:
        name: add
      params:
        - name: first
          from:
            - task: first-add
              result: sum
        - name: second
          value: $(params.third)

The key part here is the "from" clause in second-add's first param. With this clause the user expresses that the value for the param should come from first-add's sum result.

A key point for implementers here is that by expressing this relationship with a "from" clause Tekton should infer that it needs to run the first-add task before it runs the second-add task. Again this is similar to the way the "from" clause works with pipeline resources and workspaces. Were we to go the route of using variable interpolation for this feature then we would likewise need to infer the ordering based on the variables used.

Actual Behavior

Right now it's not possible to use a task result as the value for a subsequent task's param.

@ghost ghost added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. maybe-next-milestone For consideration when planning the next milestone labels Jan 29, 2020
@eddie4941
Copy link
Contributor

@sbwsg The idea of inferring task order based on the from seems like a cool idea.

Another options which will keep things simple this first round is require(via validation) that all tasks mentioned in the tasks[].params[].from.task clause also appear in the tasks[].runAfter. This way we won't have to infer any ordering based on the from values.

We can then come back and drop this requirement for the inference based solution you mentioned.

@ghost
Copy link
Author

ghost commented Jan 30, 2020

Totally, good point. One thing that I was hoping was that we would have parity between the way workspaces declare "from" and the way results declare "from". But I think I'd totally be ok on a first pass with the validation enforcing that the user has to add the runAfters.

@ghost
Copy link
Author

ghost commented Jan 30, 2020

Although now that I think about it a bit more - inferring the order of the tasks based on From is possibly the easy part of this issue :) See the few lines it takes for workspaces here:

https://github.com/tektoncd/pipeline/pull/1936/files#diff-f4450edc23218cd8c91cc4ebec265299R155-R160

Here we just walk over the workspaces and add them to the Task's list of dependencies. The DAG library takes care of figuring out the rest like deciding if there're cycles or any other issues.

@afrittoli
Copy link
Member

@sbwsg is this something you plan on working on yourself, or does it need a volunteer?

@ghost
Copy link
Author

ghost commented Feb 4, 2020

I have a call set up with @eddie4941 and @othomann to talk through the proposal. I'll update this issue once we've had that.

@ghost
Copy link
Author

ghost commented Feb 6, 2020

Update after chatting with @skaegi a bit about "from" syntax / variable interpolation :-

At the moment we're putting a hold on the idea of using "from" to dictate ordering in workspaces. Therefore reaching "parity" between workspaces and params on this feature is less of a priority. So we figure it's probably easiest to just jump straight to introducing variable interpolation between results and params.

We also don't want to overcomplicate things right now regarding the ordering. So we propose dropping the requirement from this issue that using a variable enforces the ordering of the tasks in a pipeline. In other words users should be required to include a "runAfter" field if they want to correctly receive the results of one task into the params of another. This will also be true for workspaces - a user who wants to use a workspace across multiple tasks must explicitly order those tasks using "runAfter".

We can add the ordering part later if there is demand or if it becomes abundantly clear that it's a necessary addition.

Here's a short example YAML of what we want to support as a result of completing this issue:

kind: Pipeline
metadata:
  name: my-pipeline
spec:
  tasks:
  - name: task-a
    taskRef:
      name: foo # foo task declares a result named "sha"
  - name: task-b
    taskRef:
      name: bar # bar task declares a param named "in-sha"
    params:
    - name: in-sha
      value: $(tasks.task-a.results.sha) # NOTE: using variable to reference result
---
kind: Task
metadata:
  name: foo
spec:
  steps:
    # ... omitting for brevity
  results:
  - name: sha
    description: The SHA that this task generates
---
kind: Task
metadata:
  name: bar
spec:
  steps:
    # ... omitting for brevity
  params:
  - name: in-sha

@ghost
Copy link
Author

ghost commented Feb 10, 2020

So after some more conversation around the design of "from" we've decided that we still do want to be able to express a link between one task's workspace and another task consuming that same workspace.

Whoops, wrong issue to post this to!

@nishpa214
Copy link

Is there any consideration to add from in the future to make this dependency explicit?

It'd be nice to make this explicit so UIs or other tooling can be built easily.

@ghost
Copy link
Author

ghost commented Feb 14, 2020

Is there any consideration to add from in the future to make this dependency explicit?

Yup this is definitely still open for debate! If there were clear reasons to support it I'd be open to implementing it in future.

It'd be nice to make this explicit so UIs or other tooling can be built easily.

Just to clarify here - the variable interpolation syntax isn't explicit enough for these use cases? Is the issue that it requires parsing a string to capture the dependencies?

@bobcatfish bobcatfish added this to the Pipelines 1.0/beta 🐱 milestone Feb 17, 2020
@nishpa214
Copy link

Right, it requires some parsing to capture the dependencies.

Though thinking about it more, I guess from would just indicate that there is a dependency on one more tasks, UIs/clients would still need to figure out the why. So its not perfect either.

@othomann othomann mentioned this issue Mar 16, 2020
3 tasks
@bobcatfish
Copy link
Collaborator

Fixed in #2042

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. maybe-next-milestone For consideration when planning the next milestone priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants