Allow Pipelines to be used with different Resources #248

bobcatfish · 2018-11-21T02:01:48Z

This change makes it possible to reuse a Pipeline with different Resources without having to change the Pipeline itself. It does this by moving the linking of the Resources a Task requires from the Pipeline into the PipelineRun.

The relationship between Resources and the Tasks that are expected to be executed on them is still expressed in the Pipeline (via providedBy).

ResourceBindings move from Pipeline to PipelineRun and become ResourceDependencies. No longer calling these "bindings" in the API or using the term "source" fixes + the additional docs in using.md fixes #139.

Removing version b/c we had been hoping to use version to cover two cases:

To allow a user to override the version of the Resource they are using (e.g. for a particular git resource, use a different revision)
For the case where a Task modifies a resource then provides it to the next Task, we wanted to express the modification in the version

But now a user can "change" a Resource by creating a new one and specifying the new one in teh PipelineRun, and they can do the same thing via a TaskRun.

For the second case, in #124 @shashwathi is designing how the output of one task will be passed to the next, and it is likely going to be via PVCs and will not need the version field.

This interface is still probably not in its final form and hopefully we can iterate on it!

Fixes #200

bobcatfish · 2018-11-21T02:02:25Z

pkg/reconciler/v1alpha1/pipelinerun/validate.go

-			rr, err := c.resourceLister.PipelineResources(ns).Get(source.ResourceRef.Name)
+	for _, r := range inputs {
+		inputMapping[r.Name] = ""
+		// TODO(#213): if this is the empty string should it be an error? or maybe let the lookup fail?


note: i plan to follow up on #213 next!

nader-ziada

amazing changes, the yaml files are so much better now, but only one concern with the version information missing for the resources

nader-ziada · 2018-11-21T03:14:12Z

pkg/apis/pipeline/v1alpha1/git_resource.go

-// GetVersion returns the revision of the resource, See
-// https://git-scm.com/docs/gitrevisions#_specifying_revisions for
-// more details what the revison in github is
-func (s GitResource) GetVersion() string {


we still need a version for the git resource so it knows what to clone, or is this information somewhere else now?

Well the git resource fortunately still has the "revision" field, so as long as we are okay with having logic specific to Git (which I think we'd need anyway) I think we are still good

https://github.com/knative/build-pipeline/blob/0c9529db60cc053f9bcd14fc89a8e1454b873746/pkg/apis/pipeline/v1alpha1/git_resource.go#L33

nader-ziada · 2018-11-21T14:37:42Z

pkg/apis/pipeline/v1alpha1/image_resource.go

@@ -61,11 +61,6 @@ func (s ImageResource) GetType() PipelineResourceType {
 	return PipelineResourceTypeImage
 }

-// GetVersion returns the version of the resource
-func (s ImageResource) GetVersion() string {


same for image, we still need to know which digest to pull, or where is that information coming from? I can't find it

image has a digest field:

https://github.com/knative/build-pipeline/blob/0c9529db60cc053f9bcd14fc89a8e1454b873746/pkg/apis/pipeline/v1alpha1/image_resource.go#L51

It does mean the logic to interact with these types needs to know what type it's dealing with

(I'm thinking that we could have something kind of cool if in #238 we go the route of using Resources as an extension point, and use duck-typing for resources, then a Resource's controller can take care of itself, so it can know about the Resource's functionality specific attributes like an image's digest or a git resource's commitish)

hmm... that sounds interesting

We need to also know the type to handle passing these resources for further tasks. It is tough to come up universal solution for all resource types.

nader-ziada · 2018-11-21T14:46:46Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go

@@ -71,9 +71,6 @@ func AddInputResource(
 				if err != nil {
 					return nil, fmt.Errorf("task %q invalid Pipeline Resource: %q", task.Name, boundResource.ResourceRef.Name)
 				}


we need to provide to the git clone command the revision or it will always get master I believe

i think it's okay b/c the revision field is used

https://github.com/knative/build-pipeline/blob/0c9529db60cc053f9bcd14fc89a8e1454b873746/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go#L74

I'll make sure tho that the tests have a least one case with a custom revision!

looks like there isn't one yet, i'll add it!

yeah, that works.

bobcatfish · 2018-11-21T18:49:05Z

@pivotal-nader-ziada I re-added the test case that I (shouldn't have) removed for the branch case, now updated to get the branch from the resource instead, PTAL

bobcatfish · 2018-11-21T18:53:07Z

@pivotal-nader-ziada I re-added the test case that I (shouldn't have) removed for the branch case, now updated to get the branch from the resource instead, PTAL

I should make sure this is clear in the docs too!

bobcatfish · 2018-11-21T19:12:23Z

Okay added some more docs :D

bobcatfish · 2018-11-22T00:29:49Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go:439:18: undefined: "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1".TaskRunResourceVersion

whoops! should have ran the tests after rebasing :D

vdemeester

LGTM 🦁
Just need a small rebase 😝

knative-prow-robot · 2018-11-22T15:23:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, 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~~ [bobcatfish]

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

This change makes it possible to reuse a Pipeline with different Resources without having to change the Pipeline itself. It does this by moving the linking of the Resources a Task requires from the Pipeline into the PipelineRun. The relationship between Resources and the Tasks that are expected to be executed on them is still expressed in the Pipeline (via `providedBy`). ResourceBindings move from Pipeline to PipelineRun and become ResourceDependencies. No longer calling these "bindings" in the API or using the term "source" fixes + the additional docs in using.md fixes tektoncd#139. The most difficult part of this change was updating validatePipelineTaskAndTask - hoping to refactor this in a follow up (which would also address tektoncd#213). This interface is still probably not in its final form and hopefully we can iterate on it! Fixes tektoncd#200

@shashwathi

We had been hoping to use `version` to cover two cases: 1. To allow a user to override the version of the Resource they are using (e.g. for a particular git resource, use a different revision) 2. For the case where a Task modifies a resource then provides it to the next Task, we wanted to express the modification in the version In the previous commit, we made it so that a user can change a Resource by creating a new one and specifying the new one in teh PipelineRun, and they can do the same thing via a TaskRun. For the second case, in #124 @shashwathi is designing how the output of one task will be passed to the next, and it is likely going to be via PVCs and will not need the version field. Fixes tektoncd#200

After removing `version` field, trying to make sure it is clear how to use specific commits/digests/branches/forks without the `version` field. Also fixed `/workspace` docs which hadn't been updated to include multiple input changes #123

knative-metrics-robot · 2018-11-22T17:51:04Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/cluster_resource.go	59.4%	61.3%	1.9
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	81.7%	82.7%	1.0
pkg/reconciler/v1alpha1/pipelinerun/validate.go	100.0%	96.8%	-3.2

nader-ziada · 2018-11-22T18:05:27Z

/lgtm

/meow space

knative-prow-robot · 2018-11-22T18:05:28Z

@pivotal-nader-ziada:

In response to this:

/lgtm

/meow space

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.

Readd nop and move knative-images to tekton-images

This will emove the binding field we have deprecated in 0.2 in favours of bindings Fix tektoncd#248

knative-prow-robot requested review from imjasonh and nader-ziada November 21, 2018 02:01

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 21, 2018

bobcatfish commented Nov 21, 2018

View reviewed changes

nader-ziada reviewed Nov 21, 2018

View reviewed changes

bobcatfish force-pushed the goodbye_version branch from 0c9529d to 1963376 Compare November 21, 2018 18:48

bobcatfish force-pushed the goodbye_version branch from d03458b to 8ebf1f5 Compare November 22, 2018 00:26

vdemeester approved these changes Nov 22, 2018

View reviewed changes

bobcatfish added 5 commits November 22, 2018 09:43

Fix markdown linting issues

8333cb0

Remove kubebuilder make reminders about regenerating code

7e2c673

bobcatfish force-pushed the goodbye_version branch from 8ebf1f5 to 7e2c673 Compare November 22, 2018 17:48

knative-prow-robot assigned nader-ziada Nov 22, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2018

knative-prow-robot merged commit 9aa252b into tektoncd:master Nov 22, 2018

mchmarny unassigned nader-ziada Mar 7, 2019

chmouel pushed a commit to chmouel/tektoncd-pipeline that referenced this pull request Dec 4, 2019

Merge pull request tektoncd#248 from chmouel/move-knative-to-tekton

a348f58

Readd nop and move knative-images to tekton-images

pradeepitm12 pushed a commit to pradeepitm12/pipeline that referenced this pull request Jan 28, 2021

Remove deprecated binding

1fe51c1

This will emove the binding field we have deprecated in 0.2 in favours of bindings Fix tektoncd#248

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Pipelines to be used with different Resources #248

Allow Pipelines to be used with different Resources #248

bobcatfish commented Nov 21, 2018 •

edited

Loading

bobcatfish Nov 21, 2018

nader-ziada left a comment

nader-ziada Nov 21, 2018

bobcatfish Nov 21, 2018

bobcatfish Nov 21, 2018

nader-ziada Nov 21, 2018

bobcatfish Nov 21, 2018

nader-ziada Nov 21, 2018

shashwathi Nov 21, 2018 •

edited

Loading

nader-ziada Nov 21, 2018

bobcatfish Nov 21, 2018

bobcatfish Nov 21, 2018

nader-ziada Nov 21, 2018

bobcatfish commented Nov 21, 2018

bobcatfish commented Nov 21, 2018

bobcatfish commented Nov 21, 2018

bobcatfish commented Nov 22, 2018

vdemeester left a comment

knative-prow-robot commented Nov 22, 2018

knative-metrics-robot commented Nov 22, 2018

nader-ziada commented Nov 22, 2018

knative-prow-robot commented Nov 22, 2018

Allow Pipelines to be used with different Resources #248

Allow Pipelines to be used with different Resources #248

Conversation

bobcatfish commented Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

nader-ziada 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

shashwathi Nov 21, 2018 • edited Loading

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 Nov 21, 2018

bobcatfish commented Nov 21, 2018

bobcatfish commented Nov 21, 2018

bobcatfish commented Nov 22, 2018

vdemeester left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 22, 2018

knative-metrics-robot commented Nov 22, 2018

nader-ziada commented Nov 22, 2018

knative-prow-robot commented Nov 22, 2018

bobcatfish commented Nov 21, 2018 •

edited

Loading

shashwathi Nov 21, 2018 •

edited

Loading