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-0074] Deprecate PipelineResources ☠️ #480

Merged

Conversation

bobcatfish
Copy link
Contributor

@bobcatfish bobcatfish commented Jul 16, 2021

This TEP proposes officially deprecating PipelineResources. From some
perspectives they have been unoffically deprecated for quite a while,
ever since we decided not to make them beta.

By officially making them deprecated, we can give clearer guidance to
our community on how to use Tekton Pipelines going forward instead of
being in a place where we recommend folks don't use PipelineResources
but can't clearly say what will happen next with them.

This proposal still keeps open the possibility that we can benefit from
a concept similar to PipelineResources, which could be worked on in
parallel by anyone who feels motivated.

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2021
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 16, 2021
@bobcatfish bobcatfish force-pushed the tep_deprecate_pipelineresources branch from 1270a3e to c82f79c Compare July 22, 2021 23:08
@vdemeester
Copy link
Member

Note that we are still using PipelineResources on dogfooding for CI 👼🏼

cc @afrittoli

@bobcatfish
Copy link
Contributor Author

Note that we are still using PipelineResources on dogfooding for CI 👼🏼

Good point @vdemeester - maybe I can include in here that as part of this proposal we try updating our dogfooding to use alternatives like the experimental pipeline -> taskrun custom task as a POC that this is a valid alternative suggestion? AND we get to get some use of the experimental custom task at the same time :D

@bobcatfish bobcatfish force-pushed the tep_deprecate_pipelineresources branch 2 times, most recently from 28a9445 to 40980de Compare July 26, 2021 22:30
@bobcatfish bobcatfish marked this pull request as ready for review July 26, 2021 22:31
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2021
@bobcatfish
Copy link
Contributor Author

It's ready for review!!!! 🎉

@savitaashture
Copy link
Contributor

savitaashture commented Jul 27, 2021

@bobcatfish CI failing with below error

diff --git a/teps/README.md b/teps/README.md
index d9a6514..54b8225 100644
--- a/teps/README.md
+++ b/teps/README.md
@@ -219,4 +219,4 @@ This is the complete list of Tekton teps:
 |[TEP-0070](0070-tekton-catalog-task-platform-support.md) | Platform support in Tekton catalog | proposed | 2021-06-02 |
 |[TEP-0071](0071-custom-task-sdk.md) | Custom Task SDK | proposed | 2021-06-15 |
 |[TEP-0072](0072-results-json-serialized-records.md) | Results: JSON Serialized Records | proposed | 2021-05-11 |
-|[TEP-0074](0074-deprecate-pipelineresources.md) | Deprecate PipelineResources | proposed | 2021-07-14 |
\ No newline at end of file
+|[TEP-0074](0074-deprecate-pipelineresources.md) | Deprecate PipelineResources | proposed | 2021-07-26 |

@savitaashture
Copy link
Contributor

/test pull-community-teps-lint

@vdemeester
Copy link
Member

/assign

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2021
Comment on lines 142 to 146
* When we create v1 versions of Tasks, Pipelines, TaskRuns and PipelineRuns, all alpha features should be behind
[the alpha api flag](https://github.com/tektoncd/pipeline/blob/main/docs/install.md#customizing-the-pipelines-controller-behavior)
* Our documentation and examples should clearly recommend best practices for how to use Tekton Pipelines components
* Our dogfooding should provide guidelines for how we recommend using Tekton
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to have as requirements that any TEP that is listed above (and implementing parts of what PipelineResource was providing) should be implemented and out of alpha ? (aka enabled by default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question - would you want that to be a requirement before we announce our intention to deprecate PipelineResources, or would that be a requirement for their final removal?

  1. One path could be that we approve this TEP as "implementable", announce the deprecation, add it to our v1 plans, but we make their actual removal (and therefore the v1 release!) contingent on these other TEPs making it to at least alpha. (Perhaps we include that the TEPs should at least be accepted as proposed? which TEP-0044 (decoupling scheduling) is, but the other two (dict + array support) are not)
  2. The other path would be to maintain our current state (PipelineResources not recommended but with their future somewhat ambiguous) until those other TEPs make it to at least Alpha

I feel like (1) has the disadvantage of giving our users somewhat conflicting info: this thing is deprecated, the replacement(s) are a WIP, but (2) has the disadvantage of continuing this limbo state where we're not giving users a clear idea of our plans. So my preference (and there are definitely some selfish reasons here but I'll try to be impartial XD) would be to go with (1)

Copy link
Member

Choose a reason for hiding this comment

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

+1 to option (1) - if we know this is the direction we want to go it seems reasonable to warn users ASAP so they don't adopt something we plan to remove in the near future (even if the alternative isn't as available / as friendly yet).

Copy link
Member

Choose a reason for hiding this comment

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

option (1) seems more appropriate indeed, and announcing the PipelineResources are deprecated is more or less what we unofficially say (at least I do 😅 ) so this would make it official. But I think we should aim for condition the "removing them" (CRD not in new release / fresh installs) to "having any feature required to support the use case PipelineResource were out of alpha" (aka available on a default install) — what weird sentence lol. If we don't have that, indeed the message is going to be weird : "you could do that before, now you have to enable alpha api to do the same".

So I vote for marking this implementable, announcing the deprecation, but adding a condition to the actual removal, which would be "the required set of feature (list of TEP) is implemented, and out-of beta for X release".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay after a big delay, I've tried to update this explicitly in the TEP in a "Requirements to mark as implementable" section!

bobcatfish added a commit to bobcatfish/plumbing that referenced this pull request Aug 13, 2021
As part of TEP-0074 (tektoncd/community#480) I'm
trying to update some of the plumbing use of PipelineResources to not
use them anymore.

I picked this yamllint pipeline as a simple example; I thought I'd need
the experimental pipeline to taskrun controller, but in this case
conditions are in the mix - when expressions are the way to update those
but they aren't supported in the pipeline to taskrun controller yet.
Also the original version would run 3 separate pods, and each would
separately clone the git resource. So in this updated version, even
though it will run 4 separate pods, it will only clone the git repo once
which seems like an optimization. And once we support when expressions
in the pipeline to taskrun controller (seems like it could be easy
enough? they execute sequentially after all!) this could all happen in
one pod.

Anyway this seems like an okay first step; next I'll tackle something
that can use the experimental pipeline to taskrun controller.

I tested this by setting up my own eventlistener with the same bindings
and trigger template on my own repo and it seems to work - I didn't have
all of the event triggering setup tho so the real test will be live :D
bobcatfish added a commit to bobcatfish/plumbing that referenced this pull request Aug 13, 2021
As part of TEP-0074 (tektoncd/community#480) I'm
trying to update some of the plumbing use of PipelineResources to not
use them anymore.

I picked this yamllint pipeline as a simple example; I thought I'd need
the experimental pipeline to taskrun controller, but in this case
conditions are in the mix - when expressions are the way to update those
but they aren't supported in the pipeline to taskrun controller yet.
Also the original version would run 3 separate pods, and each would
separately clone the git resource. So in this updated version, even
though it will run 4 separate pods, it will only clone the git repo once
which seems like an optimization. And once we support when expressions
in the pipeline to taskrun controller (seems like it could be easy
enough? they execute sequentially after all!) this could all happen in
one pod.

Anyway this seems like an okay first step; next I'll tackle something
that can use the experimental pipeline to taskrun controller.

I tested this by setting up my own eventlistener with the same bindings
and trigger template on my own repo and it seems to work - I didn't have
all of the event triggering setup tho so the real test will be live :D
@bobcatfish bobcatfish force-pushed the tep_deprecate_pipelineresources branch from 40980de to a542253 Compare August 13, 2021 16:59
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2021
@bobcatfish bobcatfish force-pushed the tep_deprecate_pipelineresources branch from a542253 to cb140e1 Compare August 13, 2021 17:24
@wlynch
Copy link
Member

wlynch commented Aug 16, 2021

/approve

LGTM. I suspect that this will be something we come back to and take another look at, but removing to simplify the v1 surface while we figure out the rest SGTM.

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2021
@jerop
Copy link
Member

jerop commented Aug 23, 2021

/assign @sbwsg

@tekton-robot tekton-robot assigned ghost Aug 23, 2021
@jerop
Copy link
Member

jerop commented Aug 23, 2021

/assign

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2021
tekton-robot pushed a commit to tektoncd/plumbing that referenced this pull request Aug 24, 2021
As part of TEP-0074 (tektoncd/community#480) I'm
trying to update some of the plumbing use of PipelineResources to not
use them anymore.

I picked this yamllint pipeline as a simple example; I thought I'd need
the experimental pipeline to taskrun controller, but in this case
conditions are in the mix - when expressions are the way to update those
but they aren't supported in the pipeline to taskrun controller yet.
Also the original version would run 3 separate pods, and each would
separately clone the git resource. So in this updated version, even
though it will run 4 separate pods, it will only clone the git repo once
which seems like an optimization. And once we support when expressions
in the pipeline to taskrun controller (seems like it could be easy
enough? they execute sequentially after all!) this could all happen in
one pod.

Anyway this seems like an okay first step; next I'll tackle something
that can use the experimental pipeline to taskrun controller.

I tested this by setting up my own eventlistener with the same bindings
and trigger template on my own repo and it seems to work - I didn't have
all of the event triggering setup tho so the real test will be live :D
@vdemeester vdemeester added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Sep 13, 2021
@ghost
Copy link

ghost commented Sep 27, 2021

I'm a bit unclear - is this TEP ready for another review or are there more changes that still need to be incorporated? FWIW this looks good to me and I'm in favor of PipelineResources' removal. Looks like it needs a non-Googler approval at this point.

bobcatfish added a commit to bobcatfish/plumbing that referenced this pull request Oct 15, 2021
As part of TEP-0074 (tektoncd/community#480) I'm trying to update some of
the plumbing use of PipelineResources to not use them anymore - and even
further I want to start dogfooding [the experimental pipeline to taskrun custom task](https://github.com/tektoncd/experimental/tree/main/pipeline-to-taskrun).
So to that end I've found a candiate Task that was using
PipelineResources and I've converted it to use a Pipeline instead. I've
also been able to run this successfully as a single pod on my own
cluster so once I have the pipeline to taskrun controller installed in
dogfooding I can come back and update this to run as a pod as well!
@bobcatfish bobcatfish force-pushed the tep_deprecate_pipelineresources branch from cb140e1 to e19b46f Compare October 15, 2021 21:31
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2021
@bobcatfish bobcatfish force-pushed the tep_deprecate_pipelineresources branch from e19b46f to 172e3a2 Compare October 15, 2021 21:36
@bobcatfish
Copy link
Contributor Author

Sorry for the delay and confusion @sbwsg and everyone!

This is finally ready for another look:

  • I've tried to be very clear about what functionality must be available at a beta level of stability (a suggested by @vdemeester ) and what the progression of events would be if this TEP is accepted as proposed (see the "Proposal" section and the "Requirements to mark as implementable" sections for a breakdown)
  • I wanted to make sure that reviewers also notice the section "New repo: tektoncd/images" to make sure that that doesn't get lost in the rest of the details

PTAL

@bobcatfish bobcatfish force-pushed the tep_deprecate_pipelineresources branch from 172e3a2 to 3327aa9 Compare October 15, 2021 21:41
Comment on lines 133 to 134
* As a user of Tekton Pipelines v1, I want to be able to:
* Understand clearly the stability level of all components I'm using
Copy link
Member

Choose a reason for hiding this comment

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

nit: If we use "Tekton Pipelines" there, it's pretty much "one component" isn't it ? (or component here isn't tektoncd/… but tektoncd/pipeline resources ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe instead of "component" explicitly saying something like "resource" or "CRD" is more clear

Comment on lines 253 to 254
* Won't be as obvious or easy how to use common CI/CD integrations such as git when looking at Tekton Pipelines docs
(will need to be directed to the catalog and/or [tektoncd/images](#new-repo-tektoncdimages))
Copy link
Member

Choose a reason for hiding this comment

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

nit: Isn't this up to us on docs side ? Nothing should prevent us from having example, walkthrough, and use-cases parts in our documentation that would "use" those images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense! I've tried to make this more clear

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, vdemeester, wlynch

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

This TEP proposes _officially_ deprecating PipelineResources. From some
perspectives they have been unoffically deprecated for quite a while,
ever since [we decided not to make them beta](https://github.com/tektoncd/pipeline/blob/e76d4132ab2ecfbedc45a964f08a01022e2d4c14/docs/resources.md#why-arent-pipelineresources-in-beta).

By officially making them deprecated, we can give clearer guidance to
our community on how to use Tekton Pipelines going forward instead of
being in a place where we recommend folks don't use PipelineResources
but can't clearly say what will happen next with them.

This proposal still keeps open the possibility that we can benefit from
a concept similar to PipelineResources, which could be worked on in
parallel by anyone who feels motivated.
@bobcatfish bobcatfish force-pushed the tep_deprecate_pipelineresources branch from 3327aa9 to 0abdffd Compare October 25, 2021 22:59
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.

I've seen PipelineResource like requirements popping up in a few places, but I agree we should not insist on keeping the existing PipelineResources around and focus on getting to v1 instead. Thanks for all the work around this and collating all the relevant info in one TEP 🙏

Since this has been already approved and discussed in the API WG, it's fine to lgtm.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2021
@tekton-robot tekton-robot merged commit dbdf8c6 into tektoncd:main Oct 26, 2021
tekton-robot pushed a commit to tektoncd/plumbing that referenced this pull request Oct 28, 2021
As part of TEP-0074 (tektoncd/community#480) I'm trying to update some of
the plumbing use of PipelineResources to not use them anymore - and even
further I want to start dogfooding [the experimental pipeline to taskrun custom task](https://github.com/tektoncd/experimental/tree/main/pipeline-to-taskrun).
So to that end I've found a candiate Task that was using
PipelineResources and I've converted it to use a Pipeline instead. I've
also been able to run this successfully as a single pod on my own
cluster so once I have the pipeline to taskrun controller installed in
dogfooding I can come back and update this to run as a pod as well!
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Proposed
Development

Successfully merging this pull request may close these issues.

7 participants