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

TEP-0038 "Generic Workspaces" #382

Merged
merged 1 commit into from Apr 1, 2021
Merged

TEP-0038 "Generic Workspaces" #382

merged 1 commit into from Apr 1, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 13, 2021

This TEP outlines a problem facing the Workspaces feature in Tekton Pipelines: there are many different
ways that people and companies want to manage Workspaces but Tekton provides only a very narrow set of
supported storage options.

This was originally prposed in #290
but closed shortly after. Re-opening in response to @afrittoli 's
comment here: #290 (comment)

@ghost
Copy link
Author

ghost commented Mar 13, 2021

/kind tep

@tekton-robot tekton-robot added kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 13, 2021
@ghost ghost changed the title Propose "Generic Workspaces" TEP TEP-0038 "Generic Workspaces" Mar 13, 2021
@chhsia0
Copy link
Contributor

chhsia0 commented Mar 15, 2021

/assign @bobcatfish

@chhsia0
Copy link
Contributor

chhsia0 commented Mar 15, 2021

/assign @afrittoli

Copy link
Contributor

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few minor comments from me - i think the problem statement makes sense, im not sure im 100% sure that im convinced it should be addressed, i.e. i think it depends on what the possible solutions are (i think im falling into the trap where im having trouble agreeing with the problem statement without knowing what the solution might look like) but anyway im happy to merge as proposed!

/approve

@afrittoli still to review:
/hold

teps/0038-generic-workspaces.md Outdated Show resolved Hide resolved
- A Platform Builder can't limit the set of supported volume types to only those they want to allow.
- A Platform Builder can't expand the set of supported volume types to add those they manage themselves.
- A Platform Builder can expect the set of supported Workspace types to grow as support is added
for new Kubernetes types (like `ProjectedVolume`) and must plan around those potential changes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think im being pedantic so feel free to ignore me, but im wondering if 'platform developers' are the folks who have this set of concerns, or if that's more tool developers? (i think the thing tripping me up is that im not sure why platform builders really care about systems other than the one they're building)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i think the thing tripping me up is that im not sure why platform builders really care about systems other than the one they're building)

My take is they care in the sense that they want to interoperate with those systems via the Tekton API. Resources from one exported and then imported to another. If the Tekton API is that locus of integration then this is the list of responsibilities that falls on the platform developer's shoulders.

Does that interpretation make sense or do you still think these concerns are first and foremost those of Tool Developers? Happy to change it if so.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've trimmed the list back a bit here in response to the feedback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kk sounds fine to me! maybe we could add tool developers as well? im thinking something like "as a tool developer I want to develop tools that will work with any k8s system" - for example the CLI can run Pipelines, but maybe it only knows about PVCs and emptyDirs as possible volumes, and doesn't know how to make a Gluster volume - id like my tool to be able to work with even systems that require Gluster volumes

(i think im just trying to polish something that's already in great shape)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point - makes total sense that this is also a concern of folks writing interoperable tools. Updated!

teps/0038-generic-workspaces.md Outdated Show resolved Hide resolved
@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2021
@tekton-robot
Copy link
Contributor

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

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2021
This TEP outlines a problem facing the Workspaces feature in Tekton Pipelines: there are many different
ways that people and companies want to manage Workspaces but Tekton provides only a very narrow set of
supported storage options.

This was originally prposed in #290
but closed shortly after. Re-opening in response to @afrittoli's
comment here: #290 (comment)
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good and thorough!
/lgtm

Comment on lines +32 to +33
The Pipelines API Spec is intentionally even more limited: only support for `emptyDir` is required
to conform to the v1beta1 API. All other workspace types are optional. This is done to reduce
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point :)

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2021
@afrittoli
Copy link
Member

Since the hold was waiting on my review, removing it now.
/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2021
@tekton-robot tekton-robot merged commit 6b2c855 into tektoncd:main Apr 1, 2021
@ghost ghost deleted the propose-generic-workspaces-again branch April 1, 2021 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
Status: Proposed
Development

Successfully merging this pull request may close these issues.

4 participants