Use the same logic to resolve spec vs ref in Pipelines + Tasks

In tektoncd#1324 we updated PipelineRuns to allow for embedding ResourceSpecs in PipelineRuns. This commit makes it so that the logic for resolving (i.e. deciding if PipelineResources are specified by Spec or Ref) is shared by PipelineRuns + TaskRuns. This is done by making it so that the "binding" uses the same type underneath. The only reason they can't be the exact same type is that TaskRuns additionally need the "path" attribute, which is actually only used for PVC copying, which will be removed in tektoncd#1284, and then we should be able to remove paths entirely and the type can be the same. Also added some additional comments around the use of `SelfLink`, and made sure it was well covered in the reconciler test.
bobcatfish · Oct 1, 2019 · aa4fb0a · aa4fb0a
1 parent 37d5327
commit aa4fb0a
Show file tree

Hide file tree

Showing 20 changed files with 650 additions and 337 deletions.
diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types.go b/pkg/apis/pipeline/v1alpha1/taskrun_types.go
@@ -79,16 +79,12 @@ type TaskRunInputs struct {
 }
 
 // TaskResourceBinding points to the PipelineResource that
-// will be used for the Task input or output called Name. The optional Path field
-// corresponds to a path on disk at which the Resource can be found (used when providing
-// the resource via mounted volume, overriding the default logic to fetch the Resource).
+// will be used for the Task input or output called Name.
 type TaskResourceBinding struct {
-	Name string `json:"name"`
-	// no more than one of the ResourceRef and ResourceSpec may be specified.
-	// +optional
-	ResourceRef PipelineResourceRef `json:"resourceRef,omitempty"`
-	// +optional
-	ResourceSpec *PipelineResourceSpec `json:"resourceSpec,omitempty"`
+	PipelineResourceBinding
+	// Paths will probably be removed in #1284, and then PipelineResourceBinding can be used instead.
+	// The optional Path field corresponds to a path on disk at which the Resource can be found
+	// (used when providing the resource via mounted volume, overriding the default logic to fetch the Resource).
 	// +optional
 	Paths []string `json:"paths,omitempty"`
 }

diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go
@@ -172,10 +172,12 @@ func TestInput_Validate(t *testing.T) {
 			Value: *builder.ArrayOrString("value"),
 		}},
 		Resources: []v1alpha1.TaskResourceBinding{{
-			ResourceRef: v1alpha1.PipelineResourceRef{
-				Name: "testresource",
+			PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+				ResourceRef: v1alpha1.PipelineResourceRef{
+					Name: "testresource",
+				},
+				Name: "workspace",
 			},
-			Name: "workspace",
 		}},
 	}
 	if err := i.Validate(context.Background(), "spec.inputs"); err != nil {
@@ -192,26 +194,32 @@ func TestInput_Invalidate(t *testing.T) {
 		name: "duplicate task inputs",
 		inputs: v1alpha1.TaskRunInputs{
 			Resources: []v1alpha1.TaskResourceBinding{{
-				ResourceRef: v1alpha1.PipelineResourceRef{
-					Name: "testresource1",
+				PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+					ResourceRef: v1alpha1.PipelineResourceRef{
+						Name: "testresource1",
+					},
+					Name: "workspace",
 				},
-				Name: "workspace",
 			}, {
-				ResourceRef: v1alpha1.PipelineResourceRef{
-					Name: "testresource2",
+				PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+					ResourceRef: v1alpha1.PipelineResourceRef{
+						Name: "testresource2",
+					},
+					Name: "workspace",
 				},
-				Name: "workspace",
 			}},
 		},
 		wantErr: apis.ErrMultipleOneOf("spec.Inputs.Resources.Name"),
 	}, {
 		name: "invalid task input params",
 		inputs: v1alpha1.TaskRunInputs{
 			Resources: []v1alpha1.TaskResourceBinding{{
-				ResourceRef: v1alpha1.PipelineResourceRef{
-					Name: "testresource",
+				PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+					ResourceRef: v1alpha1.PipelineResourceRef{
+						Name: "testresource",
+					},
+					Name: "resource",
 				},
-				Name: "resource",
 			}},
 			Params: []v1alpha1.Param{{
 				Name:  "name",
@@ -226,32 +234,38 @@ func TestInput_Invalidate(t *testing.T) {
 		name: "duplicate resource ref and resource spec",
 		inputs: v1alpha1.TaskRunInputs{
 			Resources: []v1alpha1.TaskResourceBinding{{
-				ResourceRef: v1alpha1.PipelineResourceRef{
-					Name: "testresource",
+				PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+					ResourceRef: v1alpha1.PipelineResourceRef{
+						Name: "testresource",
+					},
+					ResourceSpec: &v1alpha1.PipelineResourceSpec{
+						Type: v1alpha1.PipelineResourceTypeGit,
+					},
+					Name: "resource-dup",
 				},
-				ResourceSpec: &v1alpha1.PipelineResourceSpec{
-					Type: v1alpha1.PipelineResourceTypeGit,
-				},
-				Name: "resource-dup",
 			}},
 		},
 		wantErr: apis.ErrDisallowedFields("spec.Inputs.Resources.Name.ResourceRef", "spec.Inputs.Resources.Name.ResourceSpec"),
 	}, {
 		name: "invalid resource spec",
 		inputs: v1alpha1.TaskRunInputs{
 			Resources: []v1alpha1.TaskResourceBinding{{
-				ResourceSpec: &v1alpha1.PipelineResourceSpec{
-					Type: "non-existent",
+				PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+					ResourceSpec: &v1alpha1.PipelineResourceSpec{
+						Type: "non-existent",
+					},
+					Name: "resource-inv",
 				},
-				Name: "resource-inv",
 			}},
 		},
 		wantErr: apis.ErrInvalidValue("spec.type", "non-existent"),
 	}, {
 		name: "no resource ref and resource spec",
 		inputs: v1alpha1.TaskRunInputs{
 			Resources: []v1alpha1.TaskResourceBinding{{
-				Name: "resource",
+				PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+					Name: "resource",
+				},
 			}},
 		},
 		wantErr: apis.ErrMissingField("spec.Inputs.Resources.Name.ResourceRef", "spec.Inputs.Resources.Name.ResourceSpec"),
@@ -269,10 +283,12 @@ func TestInput_Invalidate(t *testing.T) {
 func TestOutput_Validate(t *testing.T) {
 	i := v1alpha1.TaskRunOutputs{
 		Resources: []v1alpha1.TaskResourceBinding{{
-			ResourceRef: v1alpha1.PipelineResourceRef{
-				Name: "testresource",
+			PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+				ResourceRef: v1alpha1.PipelineResourceRef{
+					Name: "testresource",
+				},
+				Name: "someimage",
 			},
-			Name: "someimage",
 		}},
 	}
 	if err := i.Validate(context.Background(), "spec.outputs"); err != nil {
@@ -288,23 +304,29 @@ func TestOutput_Invalidate(t *testing.T) {
 		name: "duplicated task outputs",
 		outputs: v1alpha1.TaskRunOutputs{
 			Resources: []v1alpha1.TaskResourceBinding{{
-				ResourceRef: v1alpha1.PipelineResourceRef{
-					Name: "testresource1",
+				PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+					ResourceRef: v1alpha1.PipelineResourceRef{
+						Name: "testresource1",
+					},
+					Name: "workspace",
 				},
-				Name: "workspace",
 			}, {
-				ResourceRef: v1alpha1.PipelineResourceRef{
-					Name: "testresource2",
+				PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+					ResourceRef: v1alpha1.PipelineResourceRef{
+						Name: "testresource2",
+					},
+					Name: "workspace",
 				},
-				Name: "workspace",
 			}},
 		},
 		wantErr: apis.ErrMultipleOneOf("spec.Outputs.Resources.Name"),
 	}, {
 		name: "no output resource with resource spec nor resource ref",
 		outputs: v1alpha1.TaskRunOutputs{
 			Resources: []v1alpha1.TaskResourceBinding{{
-				Name: "workspace",
+				PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+					Name: "workspace",
+				},
 			}},
 		},
 		wantErr: apis.ErrMissingField("spec.Outputs.Resources.Name.ResourceSpec", "spec.Outputs.Resources.Name.ResourceRef"),

diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go
diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go
@@ -80,7 +80,15 @@ func TestReconcile(t *testing.T) {
 			tb.PipelineRunSpec("test-pipeline",
 				tb.PipelineRunServiceAccount("test-sa"),
 				tb.PipelineRunResourceBinding("git-repo", tb.PipelineResourceBindingRef("some-repo")),
-				tb.PipelineRunResourceBinding("best-image", tb.PipelineResourceBindingRef("some-image")),
+				tb.PipelineRunResourceBinding("best-image", tb.PipelineResourceBindingResourceSpec(
+					&v1alpha1.PipelineResourceSpec{
+						Type: v1alpha1.PipelineResourceTypeImage,
+						Params: []v1alpha1.ResourceParam{{
+							Name:  "url",
+							Value: "gcr.io/sven",
+						}},
+					},
+				)),
 				tb.PipelineRunParam("bar", "somethingmorefun"),
 			),
 		),
@@ -160,11 +168,13 @@ func TestReconcile(t *testing.T) {
 			v1alpha1.PipelineResourceTypeGit,
 			tb.PipelineResourceSpecParam("url", "https://github.com/kristoff/reindeer"),
 		)),
-		tb.PipelineResource("some-image", "foo", tb.PipelineResourceSpec(
-			v1alpha1.PipelineResourceTypeImage,
-			tb.PipelineResourceSpecParam("url", "gcr.io/sven"),
-		)),
 	}
+
+	// When PipelineResources are created in the cluster, Kubernetes will add a SelfLink. We
+	// are using this to differentiate between Resources that we are referencing by Spec or by Ref
+	// after we have resolved them.
+	rs[0].SelfLink = "some/link"
+
 	d := test.Data{
 		PipelineRuns:      prs,
 		Pipelines:         ps,
@@ -211,13 +221,21 @@ func TestReconcile(t *testing.T) {
 				tb.TaskRunInputsParam("foo", "somethingfun"),
 				tb.TaskRunInputsParam("bar", "somethingmorefun"),
 				tb.TaskRunInputsParam("templatedparam", "$(inputs.workspace.revision)"),
-				tb.TaskRunInputsResource("workspace", tb.TaskResourceBindingResourceSpec(&rs[0].Spec)),
+				tb.TaskRunInputsResource("workspace", tb.TaskResourceBindingRef("some-repo")),
 			),
 			tb.TaskRunOutputs(
-				tb.TaskRunOutputsResource("image-to-use", tb.TaskResourceBindingResourceSpec(&rs[1].Spec),
+				tb.TaskRunOutputsResource("image-to-use", tb.TaskResourceBindingResourceSpec(
+					&v1alpha1.PipelineResourceSpec{
+						Type: v1alpha1.PipelineResourceTypeImage,
+						Params: []v1alpha1.ResourceParam{{
+							Name:  "url",
+							Value: "gcr.io/sven",
+						}},
+					},
+				),
 					tb.TaskResourceBindingPaths("/pvc/unit-test-1/image-to-use"),
 				),
-				tb.TaskRunOutputsResource("workspace", tb.TaskResourceBindingResourceSpec(&rs[0].Spec),
+				tb.TaskRunOutputsResource("workspace", tb.TaskResourceBindingRef("some-repo"),
 					tb.TaskResourceBindingPaths("/pvc/unit-test-1/workspace"),
 				),
 			),

diff --git a/pkg/reconciler/pipelinerun/resources/conditionresolution.go b/pkg/reconciler/pipelinerun/resources/conditionresolution.go
@@ -175,7 +175,9 @@ func (rcc *ResolvedConditionCheck) ToTaskResourceBindings() []v1alpha1.TaskResou
 
 	for name, r := range rcc.ResolvedResources {
 		tr := v1alpha1.TaskResourceBinding{
-			Name: name,
+			PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+				Name: name,
+			},
 		}
 		if r.SelfLink != "" {
 			tr.ResourceRef = v1alpha1.PipelineResourceRef{

diff --git a/pkg/reconciler/pipelinerun/resources/conditionresolution_test.go b/pkg/reconciler/pipelinerun/resources/conditionresolution_test.go
@@ -289,7 +289,9 @@ func TestResolvedConditionCheck_ToTaskResourceBindings(t *testing.T) {
 	}
 
 	expected := []v1alpha1.TaskResourceBinding{{
-		Name: "git-resource",
+		PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+			Name: "git-resource",
+		},
 	}}
 
 	if d := cmp.Diff(expected, rcc.ToTaskResourceBindings()); d != "" {

diff --git a/pkg/reconciler/pipelinerun/resources/input_output_steps.go b/pkg/reconciler/pipelinerun/resources/input_output_steps.go
@@ -28,9 +28,13 @@ func GetOutputSteps(outputs map[string]*v1alpha1.PipelineResource, taskName, sto
 
 	for name, outputResource := range outputs {
 		taskOutputResource := v1alpha1.TaskResourceBinding{
-			Name:  name,
+			PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+				Name: name,
+			},
 			Paths: []string{filepath.Join(storageBasePath, taskName, name)},
 		}
+		// SelfLink is being checked there to determine if this PipelineResource is an instance that
+		// exists in the cluster (in which case Kubernetes will populate this field) or is specified by Spec
 		if outputResource.SelfLink != "" {
 			taskOutputResource.ResourceRef = v1alpha1.PipelineResourceRef{
 				Name:       outputResource.Name,
@@ -55,8 +59,12 @@ func GetInputSteps(inputs map[string]*v1alpha1.PipelineResource, pt *v1alpha1.Pi
 
 	for name, inputResource := range inputs {
 		taskInputResource := v1alpha1.TaskResourceBinding{
-			Name: name,
+			PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
+				Name: name,
+			},
 		}
+		// SelfLink is being checked there to determine if this PipelineResource is an instance that
+		// exists in the cluster (in which case Kubernetes will populate this field) or is specified by Spec
 		if inputResource.SelfLink != "" {
 			taskInputResource.ResourceRef = v1alpha1.PipelineResourceRef{
 				Name:       inputResource.Name,