TEP-0010 Propose optional workspaces feature #169

ghost · 2020-08-07T16:16:09Z

Workspaces declared in Tasks are currently always required: a TaskRun
cannot elect to omit a Workspace even if the data produced or consumed
on that Workspace is irrelevant to the user's needs.

This TEP proposes a new boolean optional field for Workspace
declarations in Tasks. When the optional field is true a TaskRun
may choose to omit a Workspace binding, resulting in no filesystem
being mounted at that location. This feature would support a couple of
use cases:

credentials could be optionally provided. e.g. git-clone could accept
a .ssh directory for cloning private repos
catalog tasks could be written to support both creds-init and explicit
credentials-via-workspace.
tasks could be written to optionally take some extra configuration,
like a lint config that changes some default errors into warnings
tasks could be written to generate some optional data, such as a manifest
of the files copied out to external storage.
tasks would be able to conditionally skip some amount of work if an
optional workspace is not provided by a TaskRun: there'd be no need
to generate verbose test output to a file if no workspace was passed in
to receive that verbose test result file.
catalog tasks could be written to accept either an optional
pipelineresource OR an optional workspace at the same location on
disk, resulting in a Task that is more flexible to user need

afrittoli · 2020-08-14T14:17:14Z

Thank for this proposal! I very much like the idea, I will review the TEP asap.
Would you mind picking up a TEP number, and setting it in both the file name, file content and PR title?

ghost · 2020-08-14T14:19:43Z

Sure thing, I'm claiming number 10!

Edit: update pr, filename, and tep contents to use tep-0010

dlorenc · 2020-08-15T14:27:11Z

teps/0010-optional-workspaces.md

+of that Task in some way.
+
+Examples of this include:
+- accepting an optional linter configuration to change the warnings/errors that are


These are really good examples.

vdemeester

Few comments, but overall LGTM 👍

vdemeester · 2020-08-17T13:01:09Z

teps/0010-optional-workspaces.md

+### Goals
+
+1. Allow Tasks to declare optional support for file-based credentials.
+2. Enable Tasks to offer support of Tekton's multiple competing credential mechanisms.


Isn't this just a benefit we get from having optional support for workspace ? (instead of a goal)

From the perspective of a catalog task author I actually think this is a desirable non-obvious goal. I think we should use it to decide if the feature is successful or not: If we got to the end of this TEP and catalog authors were stuck writing separate tasks to support workspace creds and creds-init creds I'd consider that a failure of the TEP.

teps/0010-optional-workspaces.md

pritidesai · 2020-08-20T17:39:56Z

/kind tep

bobcatfish

This looks great @sbwsg ! Thanks for the super thorough write up.

My one question is whether we actually need the bound variable or whether it would be reasonable to ask folks to check for the path being the empty string instead. bound is a bit more verbose, but it feels like it might be a bit simpler to leave it out?

Aside from that I really like it!

Gonna get the ball rolling with an:

/approve

And at the very least, a few folks have looked at it, so i think we could merge as "proposed" for sure - lemme know if you want me to lgtm as well @sbwsg

bobcatfish · 2020-08-26T19:54:03Z

teps/0010-optional-workspaces.md

+A required Workspace today already provides a `path` variable. Required Workspaces
+will be updated to also expose the following variable:
+
+- `$(workspaces.<workspace-name>.bound)`: the literal string `"true"`. A required


im torn: i like how explicit this variable is however i think this information is already available in $(workspaces.<workspace-name>.path) - if $(workspaces.<workspace-name>.path) is the empty string, this implies bound is false, im wondering if we need both?

I'm personally in favor of the explicit bound variable but it mostly comes down to readability / explicitness for me.

Relying on the path being an empty string as a signal of bound-ness feels to me like a kind of institutional knowledge: if you "know" that that's what it signifies then you'll have no problem reading a Task, but if you don't have that knowledge it will just ... look a bit odd. You'll have to go look at some docs and come back and try to figure out the meaning of this slightly idiosyncratic bit of Tekton's syntax. On the other hand, the language of workspaces already includes "bound" - it's in the type itself (WorkspaceBinding) and repeated in loads of places in the workspaces.md doc already.

In some ways I wish the fields were more explicitly named too: Tasks could include a workspaceDeclarations section and Runs could include a workspaceBindings section. Then the language would tie even more closely together. But I think I've probably missed the boat on that one.

one more counter point for bound:

if you "know" that that's what it signifies then you'll have no problem reading a Task, but if you don't have that knowledge it will just ... look a bit odd

There will be nothing to force folks to use bound when they can get the same information from the path being the empty string, so having 2 ways of representing this information might be make it even harder to read Tasks using this feature?

That's the last thing I'll say about it tho, happy to defer to your preference having said my piece :D

bobcatfish · 2020-08-26T19:56:24Z

teps/0010-optional-workspaces.md

+or other Tasks in a Pipeline.
+
+Examples of usage include:
+- Receiving test results as a file as well as printed to stdout.


this is really interesting, i like the idea of being able to control this at runtime. i wonder if there's some way this feature could be included in the task specialization proposal such that specializing tasks could provide optional workspaces 🤔 🤔 🤔

gonna think about it a bit more!

bobcatfish · 2020-08-26T20:00:28Z

teps/0010-optional-workspaces.md

+2. Validation of the file contents of the optional storage.
+3. Validation of "competing" filesystem fields used in a single TaskRun. An optional Workspace
+and an optional PipelineResource competing for the same location on the filesystem will be left
+up to the runtime to handle.


i dont totally understand this last one, maybe you could add an example?

Does it make more sense with the added knowledge that PipelineResources can already be declared optional today? So a PipelineResource declared optional and a Workspace declared optional could conceivably "fight" for the same location on disk.

(if they're both provided by a user)

yes! i didnt realize that, thanks :D

will be left up to the runtime to handle.

im not 100% clear what "the runtime" means in this case - the controller? kubernetes itself?

Ah, good catch I'll update this to be clearer. Yeah I meant "Kubernetes or whatever system is implementing the Tekton API". I guess maybe platform might be a better identifier here?

haha and I've confused myself. I do quite specifically mean Kubernetes in this case :- k8s will receive a Pod with two volumeMounts both mounting at the same place (one for the resource and one for the workspace). I'm saying it's just kinda up to kubernetes to figure out how or if that causes problems.

are you saying that in this case, the pipelines controller would try to create a pod that mounts the workspace to the specified location, and then the pipelineresource may try to mount to it as well, and itll be up to the underlying platform to decide what to do?

(if so, i wonder in this case if it would be a better experience to notice this in the pipeline controller and treat the run as invalid?)

yeah, that's i'm saying this TEP wouldn't protect the user against. I can add validation for that as a requirement and remove this.

ah kk thanks for explaining. i think it makes sense as a requirement so sgtm, but plz push back if you disagree!

Updated to include the new requirement.

bobcatfish · 2020-08-26T20:00:54Z

teps/0010-optional-workspaces.md

+a missing optional Workspace does not result in a Default TaskRun Workspace
+being injected in its place. The reason for this is that optional behaviour
+in a Task should not be switched on automatically by the Default TaskRun
+Workspace mechanism.


great requirements!

bobcatfish · 2020-08-26T20:02:28Z

teps/0010-optional-workspaces.md

+
+As a Catalog Task author I want to publish a Task that can support both Workspaces
+and PipelineResources in the same location on a Step's filesystem so that my Task's
+users can choose whether to use either field without requiring separate Tasks.


oh interesting! i hadnt thought about optional workspaces allowing this 🤔 would this work without changing PipelineResources? i think that if a task says it needs a PipelineResource, the Task won't run without it? (i.e. we'd have to make PipelineResources optional too?)

@bobcatfish we do support optional PipelineResource already 😉

@vdemeester beat me to it!

bobcatfish · 2020-08-26T20:07:05Z

teps/0010-optional-workspaces.md

+
+## Drawbacks
+
+TBD


a couple things that come to mind: (cant quite decide if these are risks or drawbacks)

a task might run extra containers that it doesnt need in the case of an optional workspace, e.g. imagine the last step in a task copies test results onto an optional workspace, and when the workspace isn't provided, the step still runs but does nothing

might be a bit more confusing to understand what a Task actually requires? (only a bit tho probably)

* a task might run extra containers that it doesnt need in the case of an optional workspace, e.g. imagine the last step in a task copies test results onto an optional workspace, and when the workspace isn't provided, the step still runs but does nothing

Is it a drawback though ? It doesn't change anything that could happen now (without this feature). You can have a step that does nothing but still runs (based on some params).

* might be a bit more confusing to understand what a Task actually requires? (only a bit tho probably)

We know what is optional for the task (optional workspace, params with default vaule), so what the task requires is whatever is not optional, nor has defaults. A UI could display that I guess.

For the second point I agree completely this design means the workspace's description field has to pull double-duty: 1) It has to describe what the workspace is for 2) It has to describe how the Task behaviour changes if it's omitted.

I've added this as a Drawback

tekton-robot · 2020-08-26T20:09:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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~~ [bobcatfish]

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

vdemeester

I tend to like the explicit bound variable as it makes it really easy to read and test (true vs a string comparison)

vdemeester · 2020-08-27T08:16:37Z

teps/0010-optional-workspaces.md

+
+## Drawbacks
+
+TBD


* a task might run extra containers that it doesnt need in the case of an optional workspace, e.g. imagine the last step in a task copies test results onto an optional workspace, and when the workspace isn't provided, the step still runs but does nothing

Is it a drawback though ? It doesn't change anything that could happen now (without this feature). You can have a step that does nothing but still runs (based on some params).

* might be a bit more confusing to understand what a Task actually requires? (only a bit tho probably)

We know what is optional for the task (optional workspace, params with default vaule), so what the task requires is whatever is not optional, nor has defaults. A UI could display that I guess.

bobcatfish · 2020-08-27T17:35:48Z

@vdemeester re

I tend to like the explicit bound variable as it makes it really easy to read and test (true vs a string comparison)

I commented above but one downside id like to add is that having 2 ways to get this information (i.e. it is represented in the empty string AND in the bound var) means folks can use either, so we actually might have folks check the empty string anyway and ignore the bound var, so imo this will make things even more confusing

Workspaces declared in Tasks are currently always required: a TaskRun cannot elect to omit a Workspace even if the data produced or consumed on that Workspace is irrelevant to the user's needs. This TEP proposes a new boolean `optional` field for Workspace declarations in Tasks. When the optional field is true a TaskRun may choose to omit a Workspace binding, resulting in no filesystem being mounted at that location. This feature would support a couple of use cases: - credentials could be optionally provided. e.g. git-clone could accept a .ssh directory for cloning private repos - catalog tasks could be written to accept either an optional pipelineresource OR an optional workspace at the same location on disk, resulting in a Task that is more flexible to user need - catalog tasks could be written to support both creds-init and explicit credentials-via-workspace. - tasks could be written to optionally take some extra configuration, like a lint config that changes some default errors into warnings - tasks could be written to generate some optional data, such as a manifest of files copied to external storage. - tasks would be able to conditionally skip some amount of work if an optional workspace is not provided by a TaskRun: there'd be no need to generate verbose test output to a file if no workspace was passed in to receive that verbose test result file.

bobcatfish · 2020-08-27T20:32:58Z

Thanks @sbwsg ! I'm happy with this as-is, anyone else want to chime in @vdemeester and other @tektoncd/core-maintainers ?

i think all open threads are currently addressed so lemme know if you want me to lgtm so we can merge with "proposed" @sbwsg

vdemeester · 2020-08-28T06:43:32Z

/lgtm

The optional workspaces TEP was pretty thoroughly edited and reviewed during the proposal phase (#169). There were no outstanding blockers or requests for more information that I'm aware of that should prevent this TEP moving to implementable. This PR moves the optional workspaces TEP into implementable state with intention to go ahead and add this feature to Tekton Pipelines.

I'm an owner on the pipelines repo and would like to help out with approvals on TEPs related to pipelines. I've contributed a bit to the community repo already with various TEPs and process-related changes: - Step & Sidecar Workspaces TEP: #225 - Optional Workspaces TEP: #169 - TEP Template updates: #268, #270, #238

tekton-robot requested review from hrishin and mnuttall August 7, 2020 16:16

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 7, 2020

ghost changed the title ~~Propose optional workspaces feature as TEP~~ TEP-0010 Propose optional workspaces feature Aug 14, 2020

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 14, 2020

dlorenc reviewed Aug 15, 2020

View reviewed changes

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 17, 2020

vdemeester reviewed Aug 17, 2020

View reviewed changes

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Aug 20, 2020

bobcatfish reviewed Aug 26, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2020

vdemeester reviewed Aug 27, 2020

View reviewed changes

tekton-robot assigned vdemeester Aug 28, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2020

tekton-robot merged commit d77c4fa into tektoncd:master Aug 28, 2020

This was referenced Sep 8, 2020

Move optional workspaces TEP to implementable #199

Merged

Optional Workspaces tektoncd/pipeline#2119

Closed

ghost mentioned this pull request Feb 26, 2021

Add sbwsg as owner on community repo #364

Merged

dlorenc pushed a commit to dlorenc/community that referenced this pull request Oct 27, 2022

update scaffolding tests to not require 1.22 (tektoncd#169)

852bc01


		## Drawbacks

		TBD


		## Drawbacks

		TBD

TEP-0010 Propose optional workspaces feature #169

TEP-0010 Propose optional workspaces feature #169

Conversation

ghost commented Aug 7, 2020

afrittoli commented Aug 14, 2020

ghost commented Aug 14, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Aug 17, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

pritidesai commented Aug 20, 2020

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Aug 26, 2020

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Aug 27, 2020

bobcatfish commented Aug 27, 2020

vdemeester commented Aug 28, 2020

ghost commented Aug 14, 2020 •

edited by ghost

Loading

ghost Aug 17, 2020 •

edited by ghost

Loading