Provide more guidance on not using PipelineResources 🚧 #2483

bobcatfish · 2020-04-23T21:35:45Z

Changes

This commit makes some updates to the migration guide:

The section on how the inputs/outputs PipelineResources spec
has changed is now at the end of the guide, since it's a bit confusing
to tell ppl to try to migrate away from them and have the new syntax
for these front and center at the same time
There is now an example of how you can make a Pipeline that does the
equivalent of what you used to get with a Task that used image and git
resources

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

[n/a] Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

vdemeester

/cc @sbwsg

vdemeester · 2020-04-24T08:16:13Z

docs/migrating-v1alpha1-to-v1beta1.md

+_More on the reasoning and what's left to do in
+[Why aren't PipelineResources in Beta?](docs/resources.md#why-arent-pipelineresources-in-beta)._
+
+In the near-term `PipelineResources` will continue to be supported by Tekton.


Do we want to use supported here ?

Until there's a definite path away / around them I kinda figure I'll be continuing to review and merge fixes for the existing types and will try to provide help for PipelineResource issues when I have the time for it. But I would push back against anyone who wants to add new PR types. There was a suggestion of an SVN PipelineResource recently and I pushed back against that, for example. supported seems right to me?

I'm updating this to try to say more of what @sbwsg said instead of "supported"

ghost · 2020-04-24T12:17:35Z

docs/migrating-v1alpha1-to-v1beta1.md

+      workspace: git-source
+  # If you want you can add a Task that uses the IMAGE_DIGEST from the kaniko task
+  # via $(tasks.build-image.results.IMAGE_DIGEST) - this was a feature we hadn't been
+  # able to fully delivery with the Image PipelineResource!


nit: delivery -> deliver

whoops thanks!

ghost · 2020-04-24T12:28:35Z

docs/migrating-v1alpha1-to-v1beta1.md

+_More on the reasoning and what's left to do in
+[Why aren't PipelineResources in Beta?](docs/resources.md#why-arent-pipelineresources-in-beta)._
+
+In the near-term `PipelineResources` will continue to be supported by Tekton.


Until there's a definite path away / around them I kinda figure I'll be continuing to review and merge fixes for the existing types and will try to provide help for PipelineResource issues when I have the time for it. But I would push back against anyone who wants to add new PR types. There was a suggestion of an SVN PipelineResource recently and I pushed back against that, for example. supported seems right to me?

bobcatfish · 2020-04-28T14:24:44Z

PTAL @vdemeester @sbwsg

ghost · 2020-04-28T14:28:37Z

Changes look good to me! Will leave it to someone else to lgtm since I already approved.

bobcatfish · 2020-04-28T14:41:59Z

===============================
==== RUNNING GOLANGCI-LINT ====
===============================
level=error msg="Running error: context loading failed: no go files to analyze"
level=error msg="Timeout exceeded: try increasing it by passing --timeout option"
============================
==== BUILD TESTS FAILED ====
============================

wat

bobcatfish · 2020-04-28T14:42:06Z

/test pull-tekton-pipeline-build-tests

afrittoli

Thanks for this.
I personally disagree with the idea of moving away from the abstraction provided by pipeline resources, but I appreciate that people may need to rely on a beta-only API.
As highlighted by the "what's still missing" chapter, we do not have functional parity yet, so personally I would welcome contribution towards improving and redesigning them.

afrittoli · 2020-04-28T15:07:50Z

docs/migrating-v1alpha1-to-v1beta1.md

+In the near-term we plan to continue to merge fixes to existing `PipelineResource` types, but
+are pushing back against adding significant new features or new `PipelineResource` types, and
+encourage people to use `Tasks` for those instead. At some point the feature may well be deprecated
+and subsequently replaced.


I continue to disagree with this general statement, I still believe that pipeline resources are a much stronger abstraction than Tasks. I think we should fix / re-design the implementation behind pipeline resources and keep the abstraction, not walk away from it.

Maybe we could change the language here to At some point the feature in its current form could be redesigned, replaced, deprecated, or removed entirely?

Okay I've updated this, let me know if the new language is still too strong @afrittoli

afrittoli · 2020-04-28T15:13:36Z

docs/migrating-v1alpha1-to-v1beta1.md

+```
+
+_Note that using `PipelineResources` with this `Task` has an immediate downside in that it is
+coupled to `git`; one can't use the same `Task` with files that were obtained some other way._


I'm not sure that this is completely - we do support optional pipeline resources - so a Task that supports an optional pipeline resource could also accept a parameter that points to the location of the data if the resource is not provided. We probably would need to improve of this to make it nice and easy to use.

Right, but this would be band-aid.

hm interesting point - I hadn't thought about optional PipelineResources! I think having the git coupling in the Task is still not the greatest, but like you said it's not actually required anymore - I'm going to just remove this sentence for now!

vdemeester · 2020-04-29T09:44:34Z

I personally disagree with the idea of moving away from the abstraction provided by pipeline resources, but I appreciate that people may need to rely on a beta-only API.
As highlighted by the "what's still missing" chapter, we do not have functional parity yet, so personally I would welcome contribution towards improving and redesigning them.

The abstraction provided by PipelineResource is nice, but we need to redisgn the concept (and part of the abstraction) — hence letting it in alpha, hence making sure users using PipelineResource may run into trouble later down the line, as we do not want to make any promises that the "new design" of PipelineResource will be compatible with the current behavior.

bobcatfish · 2020-05-11T19:55:34Z

Since we haven't actually decided to deprecate PipelineResources, I've tried to soften the language around that a bit, and also removed an inaccurate sentence that @afrittoli pointed out.

Should be ready for another look @afrittoli @vdemeester @sbwsg @sergetron !

vdemeester

/meow

vdemeester · 2020-05-12T09:02:28Z

docs/migrating-v1alpha1-to-v1beta1.md

 | `spec.inputs` | Removed from `Tasks` |
 | `spec.outputs` | Removed from `Tasks` | 
-
+|  `spec.inputs.resources` | [`spec.resources.inputs`](#changes-to-pipelineresources) |


nit: there is an extra space

whoops, should be fixed now!

tekton-robot · 2020-05-12T09:03:08Z

@vdemeester:

In response to this:

/meow

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 · 2020-05-12T09:03:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, 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:

~~OWNERS~~ [sbwsg,vdemeester]

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

bobcatfish · 2020-05-12T13:49:47Z

PTAL

bobcatfish · 2020-05-13T13:37:26Z

Re-opening to see if that will trigger check-pr-has-kind-label to run again 😅

bobcatfish · 2020-05-13T13:38:01Z

Just realized I probably could have triggered that by actually adding the kind XD

/kind documentation

This commit makes some updates to the migration guide: - The section on how the inputs/outputs PipelineResources spec has changed is now at the end of the guide, since it's a bit confusing to tell ppl to try to migrate away from them and have the new syntax for these front and center at the same time - There is now an example of how you can make a Pipeline that does the equivalent of what you used to get with a Task that used image and git resources

dibyom · 2020-05-20T21:39:43Z

/lgtm

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

tekton-robot requested review from abayer and dlorenc April 23, 2020 21:35

bobcatfish force-pushed the migration_details branch from 04f8a8e to 03317a0 Compare April 23, 2020 21:37

vdemeester approved these changes Apr 24, 2020

View reviewed changes

tekton-robot requested a review from a user April 24, 2020 08:16

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

ghost approved these changes Apr 24, 2020

View reviewed changes

bobcatfish force-pushed the migration_details branch from 03317a0 to 3ebd503 Compare April 28, 2020 14:24

afrittoli reviewed Apr 28, 2020

View reviewed changes

bobcatfish force-pushed the migration_details branch 3 times, most recently from 27d114b to 3ebd503 Compare April 29, 2020 16:56

bobcatfish force-pushed the migration_details branch 2 times, most recently from 02f8fb6 to 796317c Compare May 11, 2020 19:54

vdemeester approved these changes May 12, 2020

View reviewed changes

bobcatfish force-pushed the migration_details branch from 796317c to cb7e1d7 Compare May 12, 2020 13:49

bobcatfish closed this May 13, 2020

bobcatfish reopened this May 13, 2020

tekton-robot added the kind/documentation Categorizes issue or PR as related to documentation. label May 13, 2020

bobcatfish force-pushed the migration_details branch from cb7e1d7 to 16eea0e Compare May 20, 2020 20:36

tekton-robot assigned dibyom May 20, 2020

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

tekton-robot merged commit fd1b000 into tektoncd:master May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide more guidance on not using PipelineResources 🚧 #2483

Provide more guidance on not using PipelineResources 🚧 #2483

bobcatfish commented Apr 23, 2020

vdemeester left a comment

vdemeester Apr 24, 2020

ghost Apr 24, 2020

bobcatfish Apr 28, 2020

ghost Apr 24, 2020

bobcatfish Apr 28, 2020

ghost Apr 24, 2020

bobcatfish commented Apr 28, 2020

ghost commented Apr 28, 2020

bobcatfish commented Apr 28, 2020

bobcatfish commented Apr 28, 2020

afrittoli left a comment

afrittoli Apr 28, 2020

ghost Apr 28, 2020

bobcatfish May 11, 2020

afrittoli Apr 28, 2020

vdemeester Apr 29, 2020

bobcatfish May 11, 2020

vdemeester commented Apr 29, 2020

bobcatfish commented May 11, 2020

vdemeester left a comment

vdemeester May 12, 2020

bobcatfish May 12, 2020

tekton-robot commented May 12, 2020

tekton-robot commented May 12, 2020

bobcatfish commented May 12, 2020

bobcatfish commented May 13, 2020

bobcatfish commented May 13, 2020

dibyom commented May 20, 2020

Provide more guidance on not using PipelineResources 🚧 #2483

Provide more guidance on not using PipelineResources 🚧 #2483

Conversation

bobcatfish commented Apr 23, 2020

Changes

Submitter Checklist

Reviewer Notes

vdemeester 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

bobcatfish commented Apr 28, 2020

ghost commented Apr 28, 2020

bobcatfish commented Apr 28, 2020

bobcatfish commented Apr 28, 2020

afrittoli 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

vdemeester commented Apr 29, 2020

bobcatfish commented May 11, 2020

vdemeester left a 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 May 12, 2020

tekton-robot commented May 12, 2020

bobcatfish commented May 12, 2020

bobcatfish commented May 13, 2020

bobcatfish commented May 13, 2020

dibyom commented May 20, 2020