Resolve all PipelineResources first before continuing

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"`
 }

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"),

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -242,8 +242,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
 		})
 		return nil
 	}
-	providedResources, err := resources.GetResourcesFromBindings(p, pr)
-	if err != nil {
+	if err := resources.ValidateResourceBindings(p, pr); err != nil {
 		// This Run has failed, so we need to mark it as failed and stop reconciling it
 		pr.Status.SetCondition(&apis.Condition{
 			Type:   apis.ConditionSucceeded,
@@ -254,6 +253,18 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
 		})
 		return nil
 	}
+	providedResources, err := resources.GetResourcesFromBindings(pr, c.resourceLister.PipelineResources(pr.Namespace).Get)
+	if err != nil {
+		// This Run has failed, so we need to mark it as failed and stop reconciling it
+		pr.Status.SetCondition(&apis.Condition{
+			Type:   apis.ConditionSucceeded,
+			Status: corev1.ConditionFalse,
+			Reason: ReasonCouldntGetResource,
+			Message: fmt.Sprintf("PipelineRun %s can't be Run; it tries to bind Resources that don't exist: %s",
+				fmt.Sprintf("%s/%s", p.Namespace, pr.Name), err),
+		})
+		return nil
+	}
 
 	// Ensure that the parameters from the PipelineRun are overriding Pipeline parameters with the same type.
 	// Weird substitution issues can occur if this is not validated (ApplyParameters() does not verify type).
@@ -284,7 +295,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
 		func(name string) (v1alpha1.TaskInterface, error) {
 			return c.clusterTaskLister.Get(name)
 		},
-		c.resourceLister.PipelineResources(pr.Namespace).Get,
 		func(name string) (*v1alpha1.Condition, error) {
 			return c.conditionLister.Conditions(pr.Namespace).Get(name)
 		},
@@ -301,14 +311,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
 				Message: fmt.Sprintf("Pipeline %s can't be Run; it contains Tasks that don't exist: %s",
 					fmt.Sprintf("%s/%s", p.Namespace, p.Name), err),
 			})
-		case *resources.ResourceNotFoundError:
-			pr.Status.SetCondition(&apis.Condition{
-				Type:   apis.ConditionSucceeded,
-				Status: corev1.ConditionFalse,
-				Reason: ReasonCouldntGetResource,
-				Message: fmt.Sprintf("PipelineRun %s can't be Run; it tries to bind Resources that don't exist: %s",
-					fmt.Sprintf("%s/%s", p.Namespace, pr.Name), err),
-			})
 		case *resources.ConditionNotFoundError:
 			pr.Status.SetCondition(&apis.Condition{
 				Type:   apis.ConditionSucceeded,

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,22 +221,30 @@ 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"),
 				),
 			),
 		),
 	)
 
 	// ignore IgnoreUnexported ignore both after and before steps fields
-	if d := cmp.Diff(actual, expectedTaskRun, cmpopts.SortSlices(func(x, y v1alpha1.TaskResourceBinding) bool { return x.Name < y.Name })); d != "" {
-		t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRun, d)
+	if d := cmp.Diff(expectedTaskRun, actual, cmpopts.SortSlices(func(x, y v1alpha1.TaskResourceBinding) bool { return x.Name < y.Name })); d != "" {
+		t.Errorf("expected to see TaskRun %v created. Diff (-want, +got): %s", expectedTaskRun, d)
 	}
 	// test taskrun is able to recreate correct pipeline-pvc-name
 	if expectedTaskRun.GetPipelineRunPVCName() != "test-pipeline-run-success-pvc" {

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{

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 != "" {

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,