[TEP-0142] Enable Step Reusability

[chitrangpatel]

This TEP enables step reusability by separating the actionable parts of the Step into a separate CRD. The name of this CRD is still not decided. We plan to converge on the name before making the TEP implementable.

[chitrangpatel]

/kind tep

[afrittoli]

/assign

[afrittoli]

/assign @pritidesai

[vdemeester]

/assign

[afrittoli]

Thanks, left a couple more comments.

[afrittoli]

I really don't like the idea of having a rejectWorkingDir field to be honest. It complexifies the API for weird reason imo.

I think that default should be that a workingDir is set by the Task and StepAction authors should not really need to set it or think about it in the majority of cases. There are some corner cases where a container image only works with a specific workingDir - for those cases the StepAction author needs the ability to opt-out from the Task-set workingDir.

An alternative to the out-out could be an optional field in StepAction that can be used to override the workingDir, but with a name that makes it obvious that they should not use it unless strictly needed, something like overrideWorkingDir or so.

The proposal says "actionable" things are going into StepAction and the rest ("orchestration") go into the Task consume it. A workingDir is an "actionable", so it should be there, and only there. Nothing prevents a StepAction author to make the workingDir parametrable (either by exposing a workingDir parameter, or something with more meaning (e.g. in git-clone, the path where to clone to repository, …).

I think this option will lead to a majority of StepActions exposing an extra parameter so that they can get the value of workingDir from the Task because they don't have enough context to set it themselves.
The orchestration of various steps is done by the Task, so why not give ownership of workingDir to the Task directly so that StepActions authors do not need to even start thinking about it?

A workingDir is the "current dir" where the process is executed, it makes a lot of sense to be available, and only available on StepAction because this is part of the "action". If the workingDir needs to be able to changed upon "consumption" (in a Task), then, the StepAction author can and should make it parametrizable (but with a "domain" meaning).

Uhm, I kind of disagree - why should the StepAction care about the absolute path to the current_dir?
If a StepAction clones a repo, it will do so in the current dir, it doesn't matter what dir it is, that's up to the Task author to define.
If a StepAction builds an artifact, from source, it will expect to have sources in the current dir. It may need info about some relative paths within the current dir, and those can be passed via parameters where needed.

We can make the parallel with GitHub Actions : when using actions in a Workflow, you have no way to specify the workingDir of "steps" from an Action if the author of the action didn't make it parametrizable. And imo, it is just fine as it is. It is simple to understand, and simple to "work around".

I don't think the parallel holds. The runtime environment in GitHub Actions is a VM. When you run a workflow you're given a folder in the VM which is your work area. This is something that we used to emulate in Tekton by setting a consistent workDir for all steps; unfortunately we removed that behaviour because of some odd container images, rather than giving the users of those container images an option to opt-out.

[afrittoli]

cc @vdemeester @wlynch @afrittoli @jerop @pritidesai :

Update from Data Interfaces WG: On the discussion related to workingDir, please read the section in the TEP. We will start by keeping it in StepAction.

I fear that this will promote actively promote bad practices in StepAction authoring, with every StepAction exposing one parameter that ends up in some way into the workingDir.

In the future, we will revisit the discussion in about the ability for users to opt-out or opt-in to a default workingDir in a separate effort. That way, this work can be unblocked and we can follow up this conversation in a dedicated effort.

[wlynch]

I think that default should be that a workingDir is set by the Task and StepAction authors should not really need to set it or think about it in the majority of cases.

I agree! But I think the limiting factor here is consistency with existing steps in v1. 😢
Ideally I think we should be able to take an existing step, move it into a step action and have similar behavior similar to how you can move a TaskSpec in a TaskRun to a Task. Having in-line steps default to the image workingDir and step actions default to the controller workingDir is probably going to cause more trouble than it's worth.

We either need to:

1. Make a breaking change and reintroduce the default working dir for steps with an opt-out value - my guess is that this isn't feasible at this point.
2. Make workingDir pseudo-required, though 90% of StepActions will likely be able to use workingDir: ~ to say "give me the default working dir" without needing to define a param.

[chitrangpatel]

I think that default should be that a workingDir is set by the Task and StepAction authors should not really need to set it or think about it in the majority of cases.

I agree! But I think the limiting factor here is consistency with existing steps in v1. 😢 Ideally I think we should be able to take an existing step, move it into a step action and have similar behavior similar to how you can move a TaskSpec in a TaskRun to a Task. Having in-line steps default to the image workingDir and step actions default to the controller workingDir is probably going to cause more trouble than it's worth.

We either need to:

1. Make a breaking change and reintroduce the default working dir for steps with an opt-out value - my guess is that this isn't feasible at this point.
2. Make workingDir pseudo-required, though 90% of StepActions will likely be able to use workingDir: ~ to say "give me the default working dir" without needing to define a param.

One of the things we discussed today at the Data Interfaces WG meeting was that we descope workingDir out of StepActions for now. That would completely be under the control of the Task. Note that this would provide that consistency with v1 where you would be able to stick a StepAction into a Step and out of it.

In the very near future, in a dedicated effort for workingDir, which we are already thinking about actively, we revisit workingDir as a whole from a big picture view and assess whether we need to being it back into StepActions.

Starting without it will also give us some user feedback on how Step Actions are being used by the community and if there is friction writing StepActions without workingDir.

We can always bring it back in. Once it is introduced, it is challenging to take it out.

WDYT?

cc @vdemeester @wlynch @afrittoli @jerop @pritidesai

Reverting back my approval given the workingdir changes.

[vdemeester]

WDYT?

cc @vdemeester @wlynch @afrittoli @jerop @pritidesai

I disagree 😅. I am not sure I follow too much on the v1 consistency (except if we remove workingDir from StepAction as we just put the problem aside).

We can make the parallel with GitHub Actions : when using actions in a Workflow, you have no way to specify the workingDir of "steps" from an Action if the author of the action didn't make it parametrizable. And imo, it is just fine as it is. It is simple to understand, and simple to "work around".

I don't think the parallel holds. The runtime environment in GitHub Actions is a VM. When you run a workflow you're given a folder in the VM which is your work area. This is something that we used to emulate in Tekton by setting a consistent workDir for all steps; unfortunately we removed that behaviour because of some odd container images, rather than giving the users of those container images an option to opt-out.

My point here was : if the "steps" in the action are defining/changing the working directory they act in (here), you have no way to change that from the workflow, except if it was "set as an input" (param) in the action. The rest I agree, in GitHub action, if you do not set it, you are "somewhere" that's always the same, whereas in Tekton you are where the image used defined it.

But this discussion, keep me making feel we do not necessarily have the same views on where tektoncd/pipeline stop and where something else starts.

From tektoncd/pipeline#1836.

I've been integrating with a few pen testing tool images that are relying on a very specific workingDir being set. The problem I've been having is that Tekton overrides this setting and then I had to go back into the image to reverse engineer its original value.

In the end, it's about who needs to override what, and when. Someone who rely on setting workingdir on the image, will find it painful to have to override it in the Task spec. On the other hand, having to set workingdir on each Task "could" be seen as painful. In my opinion this is a bit "better" because

• We do not write Task each and everyday
• I tend to prefer making sure I set the workingDir all the time on my task, because, that way, I am sure it's not gonna change if the underlining execution runtime (tekton here) changes behavior.

That said, I won't block that proposal to get in as is 👼🏼. I have a feeling we are overthinking the problem a bit, especially as I am not sure we are the one affected the most by it or the one using and writing the most Task.

[afrittoli]

But this discussion, keep me making feel we do not necessarily have the same views on where tektoncd/pipeline stop and where something else starts.

I would like CI/CD workflow definitions to be concise and sane default that help with that. At the same time, the API should be flexible enough to cover most corner cases and advanced users who need full control.

I see a Tekton Task as an orchestrator of Steps - ideally, unless a step has a strong need for it (i.e. a tool may work only if executed from within a certain folder), the orchestrator should pick a working folder and execute the various steps in it, so that it may reduce the need for extra parameters setting paths being passed to the StepAction.

In the end, it's about who needs to override what, and when. Someone who rely on setting workingdir on the image, will find it painful to have to override it in the Task spec. On the other hand, having to set workingdir on each Task "could" be seen as painful.

Agreed. However:

• we aim in future to have a default workingDir, so setting the workingDir in the task won't be necessary anymore
• I don't have numbers to support this statement, but I would guess that most tool images do not really need a special workingDir to work correctly. For cases where an image needs a special workingDir to function, we need to provide an option for StepAction authors to opt-out from the Task set workingDir.

[chitrangpatel]

Thanks @afrittoli @vdemeester for the discussion.

For now I have kept workingDir as a Step level field. We can being it into StepActions following user input and the work on default WorkingDir which is being actively pursued. I have updated the section on WorkingDir accordingly. PTAL.

Hopefully this resolves this discussion for now 🙂.

And please let me know if there are any other outstanding concerns that I can resolve. 🙏

[afrittoli]

Thanks for all the work on this and latest updates!
/approve

[vdemeester]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, vdemeester

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

The pull request process is described here

Needs approval from an approver in each of these files:

• teps/OWNERS [afrittoli,vdemeester]

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

[pritidesai]

Hey @chitrangpatel, I haven't been able to review it further, please go ahead and merge it as you have enough approvals. Thanks for all the work! Looking forward to this enhancement 🙏

[jerop]

Thank you to all reviewers, and many thanks @chitrangpatel for the hard work!

/lgtm