Add step composition to Tasks

[jstrachan]

so that we can reuse steps from tekton catalog or any git repository or OCI bundle easily to promote reuse and avoid copy and paste

implements TEP-0054 based on the reuse model we have been using on the Jenkins X project

Check out the Step Composition User Guide

You can see the current unit tests for step composition using input.yaml as the source tekton resource and the expected.yaml the result of applying the step composition.

Pending work

• currently the implementation of the step reuse is done in the stepper/stepper.go package but its not wired into the actual controller yet
• no work yet for handling private git repositories and mapping git servers <-> secrets

[tekton-robot]

@jstrachan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ James Strachan (8cce40e)

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign pritidesai
You can assign the PR to them by writing /assign @pritidesai 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:

• OWNERS

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

[tekton-robot]

Hi @jstrachan. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[jstrachan]

there's an issue with the CLA bot so gonna close and reopen the PR...

[michaelsauter]

In general, I think step reuse would be great to have.

When I read the Jenkins X blog post prior to seeing this PR, I have to say I found it not that "simple". It feels a bit like an overlay, instead of something that is truly integrated.

I'm wondering if it would feel different if the step reference would not be done by uses, but would be more aligned with task references?

E.g., to reference a task in a pipeline, you specify this:

spec:
   tasks:
     - name: hello-world
       taskRef:
         name: echo-task
         bundle: docker.com/myrepo/mycatalog

Imagine referencing specs would look like this:

spec:
   steps:
     - name: hello-world
       stepRef:
         task: echo-task
         step: echo-step
         bundle: docker.com/myrepo/mycatalog

The bundle could be replaced with git to reference Git repositories. I think this might be conceptually simpler, and aligns better with what is already in place (and taskRef might gain the git property as well to gain the feature too). That said, I'm not entirely happy with this either, and I realise that uses may expand to more than one step, which is something for which the example above doesn't work as nicely ...

[michaelsauter]

The env (and similar) customisation feels slightly odd to me at this hierarchy level.

It reminds me of https://tekton.dev/docs/pipelines/tasks/#specifying-a-step-template, with the difference that the stepTemplate is a default, and here we have an override. What about making this explicit, and wrapping it e.g. stepOverride? Like this:

spec:
   steps:
   - uses:
       git: tektoncd/catalog/task/kaniko/0.1/kaniko.yaml
     stepOverride:
       env:
       - name: MY_VAR
         value: CHEESE

[jstrachan]

having a Container struct nested in a Container for one container feels odd to me too ;).

We've found its often handy to start off reusing a step; extending it, then even completely inlining it (so that its no longer extending another step) and then going back to extending again.

During this change, it feels clumsy to have to keep wrapping the environment variables/commands/volume properties in a stepOverride: block and then unwrap again. Also it's going to cause confusion that in a step there are 2 places you can put env: and script: etc and developers have to remember which place to put things and deal with errors when folks forget to wrap/unwrap. I'm not sure all that complexity and confusion helps make things more clear.

e.g. think of Java for a second and a class. When you extend a class...

class Cheese extends Wine {
  // members
}

at any time you can add or remove the extends Wine and by adding/removing the extension you don't have to go around wrapping/unwrapping the members - the extension clause is enough and folks don't get confused.

So if you want to make the override mode more clear, why don't we use override instead of uses / ref?

spec:
   steps:
   - override:
       git: tektoncd/catalog/task/kaniko/0.1/kaniko.yaml
       step: my-step
     env:
     - name: MY_VAR
       value: CHEESE

we don't need to say stepOverride as we are in a step so its clear we are overriding a step. Then, rather like Java, we can add/remove the override at any time without having to make large changes to the rest of the step and the override mode is clear?

[jstrachan]

@michaelsauter incidentally I just did a little refactor to align the Uses struct so that it's mostly just a TaskRef now but with an optional step name which hopefully opens the door to reusing git with Pipeline.taskRef as well.

I take your point about uses feeling different to taskRef in a Pipeline. But at the same time we want to be able to reference all the steps in a Task or a single Step in a Task so calling it stepRef feels slightly odd when it might actually be a reference to a Task, TaskRun, Pipeline or PipelineRun in git/OCI/k8s and we consume 1 or all steps inside the referenced Task

What if we just went with ref instead of uses?

spec:
   steps:
     - name: use-all-steps-resource
       ref:
         name: echo-task
     - name: use-all-steps-git
       ref:
         git: tektoncd/catalog/task/kaniko/0.1/kaniko.yaml
     - name: use-all-steps-from-pipeline-git
       ref:
         name: echo-task
         git: tektoncd/catalog/task/kaniko/0.1/kaniko.yaml
     - name: use-all-steps-bundle
       ref:
         name: echo-task
         bundle: docker.com/myrepo/mycatalog
     - name: use-single-step-resource
       ref:
         name: echo-task
         step: echo-step
         bundle: docker.com/myrepo/mycatalog
     - name: use-single-step-git
       ref:
         step: echo-step
         git: tektoncd/catalog/task/kaniko/0.1/kaniko.yaml

FWIW the current code is using Uses reusing TaskRef - it might be cleaner to go with a similar but different struct for Steps so we can use task instead of name to be more explicit and avoid folks getting confused over what name means?

https://github.com/jstrachan/pipeline/blob/use/pkg/apis/pipeline/v1beta1/task_types.go#L149-L157

[michaelsauter]

Thinking a littler more about this, maybe what I'm really trying to say is that conceptually, reusing tasks and reusing steps should probably look and work similarly. Tasks can reference resources, or bundles, but only one at a time. I think referencing steps should feel and work the same way.

Two other, unrelated comments:

1. where to pull steps from

when it might actually be a reference to a Task, TaskRun, Pipeline or PipelineRun

While I see how that works technically, it feels awkward to me to pull steps from any other resource than a Task ...

2. reusing multiple steps

steps:
- name: use-all-steps-git
  ref:
    git: tektoncd/catalog/task/kaniko/0.1/kaniko.yaml

This array item actually expands to multiple array items (whereas other array items do not expand to more). This feels a bit hackish to me as well ...

Anyway ... as you've seen in my earlier comment I don't really have a better alternative either, but there are quite a few things which bugged me when I read the Jenkins X blog post, and I still feel that way reading this proposal. To me personally, it feels like it's close to the right abstraction, but not quite there yet.

[jstrachan]

Thinking a littler more about this, maybe what I'm really trying to say is that conceptually, reusing tasks and reusing steps should probably look and work similarly. Tasks can reference resources, or bundles, but only one at a time. I think referencing steps should feel and work the same way.

The problem is its common to just want to reuse the steps of a Task in place (e.g. git clone + kaniko) - without the user really caring about how many steps are inside those tasks and what they are called.

We could have explicit configuration to differentiate use all the steps versus use just this named step; but in the interests of simplicity the current proposal just uses an optional step name to differentiate these 2 cases.

Two other, unrelated comments:

1. where to pull steps from

when it might actually be a reference to a Task, TaskRun, Pipeline or PipelineRun

While I see how that works technically, it feels awkward to me to pull steps from any other resource than a Task ...

FWIW it's always a Task - it's just it might be defined inside a TaskRun, Pipeline or PipelineRun file/bundle/resource. Tasks are rarely stand alone resources.

[bobcatfish]

note that we will need to go through the TEP review process before merging this:

/hold

[tekton-robot]

@jstrachan: PR needs rebase.

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.

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

[jerop]

The PR to add a TEP for this change is closed in tektoncd/community#369 in light of work in TEP-0044

So, closing this PR for now (@jstrachan feel free to open when the TEP is implementable)