Tasks should use bound resources from TaskRuns

Before this change, Tasks were retrieving the Resources to use by looking for PipelineResources with exactly the name that the Resource is declared with in the Task. This means that the Resource binding in Pipelines (and TaskRuns) was doing absolutely nothing. Instead, we now map bound Resources inside of TaskRuns. The name of a Resource declared in a Task is a _parameter_, which we fulfill by providing the _key_ aka parameter of the Task, and the corresponding Resource reference we should use for that _key_. This allows us to use the same Task with different Resources without having to change the Task or the Resource. This was not caught by any tests because the tests were co-incidentally using the same name for the Resources and the Resource key/name/parameter in the Task. Now the tests use different names. This came up while creating example yaml for #89 - as soon as I tried to use two different git resources (one upstream, one my fork) I realized that the mapping was not working. This also partially addresses #138 by removing the `Name` field from the bindings, which is not required. This addresses part of #64 by adding templating from outputs.
tektoncd · Oct 30, 2018 · 429e1b1 · 429e1b1
1 parent 8a2b6c3
commit 429e1b1
Show file tree

Hide file tree

Showing 22 changed files with 370 additions and 190 deletions.
diff --git a/examples/invocations/run-kritis-test.yaml b/examples/invocations/run-kritis-test.yaml
@@ -16,6 +16,7 @@ spec:
           - resourceRef:
               name: kritis-resources-git
             version: 4da79b91e8e37ea441cfe4972565e2184c1a127f
+            key: workspace
         params:
             - name: 'makeTarget'
               value: 'test'
diff --git a/examples/pipelines/guestbook.yaml b/examples/pipelines/guestbook.yaml
@@ -9,13 +9,11 @@ spec:
           taskRef:
               name: build-push
           inputSourceBindings:
-              - name: guestbookSource
-                key: workspace # bind to the name in the task
+              - key: workspace # bind to the name in the task
                 resourceRef:
                   name: guestbook-resources-git
           outputSourceBindings:
-              - name: guestbookImage 
-                key: builtImage # bind to the name in the task
+              - key: builtImage # bind to the name in the task
                 resourceRef:
                   name: guestbookstagingimage
           params:
@@ -25,13 +23,11 @@ spec:
           taskRef:
               name: build-push
           inputSourceBindings:
-              - name: redis-ocker
-                key: workspace        
+              - key: workspace
                 resourceRef:
                   name: guestbook-resources-redis-docker                    
           outputSourceBindings:
-              - name: redisImage
-                key: builtImage
+              - key: builtImage
                 resourceRef:
                   name: redisstagingimage                
           params:
@@ -41,15 +37,13 @@ spec:
           taskRef:
               name: deploy-with-kubectl
           inputSourceBindings:
-            - name: guestbookSource
-              key: workspace
+            - key: workspace
               resourceRef:
                   name: guestbook-resources-git  
               passedConstraints: 
                 - build-guestbook
                 - build-redis
-            - name: workspace
-              key: redis-docker
+            - key: redis-docker
               resourceRef:
                   name: guestbook-resources-redis-docker  
               passedConstraints:
@@ -64,8 +58,7 @@ spec:
           taskRef:
               name: integrationTestInDocker
           inputSourceBindings:
-              - name: guestbookSource
-                key: workspace
+              - key: workspace
                 resourceRef:
                   name: guestbook-resources-git  
                 passedConstraints:
@@ -77,8 +70,7 @@ spec:
           taskRef:
               name: integration-test-in-docker
           inputSourceBindings:
-              - name: guestbookSource
-                key: workspace
+              - key: workspace
                 resourceRef:
                   name: guestbook-resources-git  
                 passedConstraints:
@@ -90,8 +82,7 @@ spec:
           taskRef:
               name: deploy-with-kubectl
           inputSourceBindings:
-              - name: guestbookSource
-                key: workspace
+              - key: workspace
                 resourceRef:
                   name: guestbook-resources-git  
                 passedConstraints:

diff --git a/examples/pipelines/kritis.yaml b/examples/pipelines/kritis.yaml
@@ -9,8 +9,7 @@ spec:
         taskRef:
           name: make
         inputSourceBindings:
-          - name: kritis-app-github
-            key: workspace # bind to the name in the task
+          - key: workspace # bind to the name in the task
             resourceRef:
               name: kritis-resources-git
         params:
@@ -20,14 +19,12 @@ spec:
         taskRef:
           name: build-push
         inputSourceBindings:
-          - name: kritis-app-github
-            key: workspace # bind to the name in the task
+          - key: workspace # bind to the name in the task
             resourceRef:
               name: kritis-resources-git
             passedConstraints: [unit-test-kritis]
         outputSourceBindings:
-          - name: kritisImage
-            key: builtImage # bind to the name in the task
+          - key: builtImage # bind to the name in the task
             resourceRef:
               name: kritis-resources-image
         params:
@@ -37,8 +34,7 @@ spec:
         taskRef:
           name: deploy-with-helm
         inputSourceBindings:
-          - name: guestbookImage
-            key: workspace
+          - key: workspace
             resourceRef:
               name: kritis-resources-image
             passedConstraints: [push-kritis]
@@ -52,8 +48,7 @@ spec:
         taskRef:
           name: integration-test-in-docker
         inputSourceBindings:
-          - name: kritis-test-github
-            key: workspace
+          - key: workspace
             resourceRef:
                 name: kritis-resources-test-git
             passedConstraints: [deploy-test-env]

diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_types.go b/pkg/apis/pipeline/v1alpha1/pipeline_types.go
@@ -92,9 +92,7 @@ type ClusterBinding struct {
 // SourceBinding is used to bind a Source from a PipelineParams to a source required
 // as an input for a task.
 type SourceBinding struct {
-	// Name is the string the Task will use to identify this resource in its inputs.
-	Name string `json:"name"`
-	// Key is the string that the PipelineParams will use to identify this source.
+	// Key is the string the Task will use to identify this resource in its inputs.
 	Key string `json:"key"`
 	// The Resource this binding is referring to
 	ResourceRef PipelineResourceRef `json:"resourceRef"`

diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go
@@ -50,7 +50,6 @@ func TestPipelineSpec_Validate_Error(t *testing.T) {
 						Name: "foo",
 						InputSourceBindings: []SourceBinding{
 							{
-								Name:              "binding",
 								PassedConstraints: []string{"bar"},
 							},
 						},
@@ -102,7 +101,6 @@ func TestPipelineSpec_Validate_Valid(t *testing.T) {
 						Name: "foo",
 						InputSourceBindings: []SourceBinding{
 							{
-								Name:              "binding",
 								PassedConstraints: []string{"bar"},
 							},
 						},

diff --git a/pkg/apis/pipeline/v1alpha1/resource_types.go b/pkg/apis/pipeline/v1alpha1/resource_types.go
@@ -106,6 +106,14 @@ type PipelineResourceVersion struct {
 	Version     string              `json:"version"`
 }
 
+// TaskRunResourceVersion defines the version of the PipelineResource that
+// will be used for the Task input or output called Key.
+type TaskRunResourceVersion struct {
+	ResourceRef PipelineResourceRef `json:"resourceRef"`
+	Version     string              `json:"version"`
+	Key         string              `json:"key"`
+}
+
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 
 // PipelineResourceList contains a list of PipelineResources

diff --git a/pkg/apis/pipeline/v1alpha1/task_validation.go b/pkg/apis/pipeline/v1alpha1/task_validation.go
@@ -63,7 +63,12 @@ func (ts *TaskSpec) Validate() *apis.FieldError {
 		}
 	}
 	if ts.Outputs != nil {
-		if err := ts.Outputs.Validate("taskspec.Outputs"); err != nil {
+		for _, resource := range ts.Outputs.Resources {
+			if err := validateResourceType(resource, fmt.Sprintf("taskspec.Outputs.Resources.%s.Type", resource.Name)); err != nil {
+				return err
+			}
+		}
+		if err := checkForDuplicates(ts.Outputs.Resources, "taskspec.Outputs.Resources.Name"); err != nil {
 			return err
 		}
 	}

diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types.go b/pkg/apis/pipeline/v1alpha1/taskrun_types.go
@@ -38,8 +38,8 @@ type TaskRunSpec struct {
 	// +optional
 	Inputs TaskRunInputs `json:"inputs,omitempty"`
 	// +optional
-	Outputs Outputs `json:"outputs,omitempty"`
-	Results Results `json:"results"`
+	Outputs TaskRunOutputs `json:"outputs,omitempty"`
+	Results Results        `json:"results"`
 	// +optional
 	Generation int64 `json:"generation,omitempty"`
 	// +optional
@@ -49,7 +49,15 @@ type TaskRunSpec struct {
 // TaskRunInputs holds the input values that this task was invoked with.
 type TaskRunInputs struct {
 	// +optional
-	Resources []PipelineResourceVersion `json:"resourcesVersion,omitempty"`
+	Resources []TaskRunResourceVersion `json:"resourcesVersion,omitempty"`
+	// +optional
+	Params []Param `json:"params,omitempty"`
+}
+
+// TaskRunOutputs holds the output values that this task was invoked with.
+type TaskRunOutputs struct {
+	// +optional
+	Resources []TaskRunResourceVersion `json:"resourcesVersion,omitempty"`
 	// +optional
 	Params []Param `json:"params,omitempty"`
 }

diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go
@@ -51,7 +51,7 @@ func (ts *TaskRunSpec) Validate() *apis.FieldError {
 		return err
 	}
 
-	// check for outputs
+	// check for output resources
 	if err := ts.Outputs.Validate("spec.Outputs"); err != nil {
 		return err
 	}
@@ -80,16 +80,8 @@ func (i TaskRunInputs) Validate(path string) *apis.FieldError {
 	return validateParameters(i.Params)
 }
 
-func (o Outputs) Validate(path string) *apis.FieldError {
-	for _, source := range o.Resources {
-		if err := validateResourceType(source, fmt.Sprintf("%s.Resources.%s.Type", path, source.Name)); err != nil {
-			return err
-		}
-		if err := checkForDuplicates(o.Resources, fmt.Sprintf("%s.Resources.%s.Name", path, source.Name)); err != nil {
-			return err
-		}
-	}
-	return nil
+func (o TaskRunOutputs) Validate(path string) *apis.FieldError {
+	return checkForPipelineResourceDuplicates(o.Resources, fmt.Sprintf("%s.Resources.Name", path))
 }
 
 func (r ResultTarget) Validate(path string) *apis.FieldError {
@@ -114,12 +106,11 @@ func (r ResultTarget) Validate(path string) *apis.FieldError {
 	return nil
 }
 
-func checkForPipelineResourceDuplicates(resources []PipelineResourceVersion, path string) *apis.FieldError {
+func checkForPipelineResourceDuplicates(resources []TaskRunResourceVersion, path string) *apis.FieldError {
 	encountered := map[string]struct{}{}
 	for _, r := range resources {
-		// Check the unique combination of resource+version. Covers the use case of inputs with same resource name
-		// and different versions
-		key := fmt.Sprintf("%s%s", r.ResourceRef.Name, r.Version)
+		// We should provide only one binding for each resource required by the Task.
+		key := strings.ToLower(r.Key)
 		if _, ok := encountered[strings.ToLower(key)]; ok {
 			return apis.ErrMultipleOneOf(path)
 		}